Skip to content

[CIR] Add framework for CIR to LLVM IR lowering #124650

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 4 commits into from
Jan 30, 2025
Merged

[CIR] Add framework for CIR to LLVM IR lowering #124650

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
41546e2
[CIR] Add framework for CIR to LLVM IR lowering
andykaylor Jan 27, 2025
a9ea2d8
Formatting and style changes
andykaylor Jan 28, 2025
dc69628
Make CIRGenerator::mlirContext a shared pointer
andykaylor Jan 28, 2025
1b0e5c6
Address review feedback
andykaylor Jan 28, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions clang/include/clang/CIR/CIRGenerator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ class CIRGenerator : public clang::ASTConsumer {
const clang::CodeGenOptions &codeGenOpts;

protected:
std::unique_ptr<mlir::MLIRContext> mlirContext;
std::shared_ptr<mlir::MLIRContext> mlirContext;
Copy link
Collaborator

@erichkeane erichkeane Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm... I don't think the shared pointer is the way here, we should have one of them own, and the other observe. What are the lifetimes of these two objects? Is there one of these that has a lifetime that is clearly larger than the other that can be the owner, and the other be a reference to it?

@bcardosolopes

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I thought shared pointer was what you were suggesting.

Here's what I'm seeing. CIRGenerator::Initialize creates the mlirContext and uses it. That function also passes it, as a raw pointer, to CIRGenModule, which keeps a copy of the pointer throughout its lifetime. CIRGenerator doesn't use it again, and was previously handing it off in CIRGenConsumer::handleTranslationUnit() via takeContext(). CIRGenConsumer::handleTranslationUnit(), in turn, passes it to a few calls as a raw pointer (in the incubator, not yet here) before handing it off, as a unique pointer, to lowerFromCIRToLLVMIR() which hands it, as a unique pointer, to lowerFromCIRToMLIRToLLVMIR() in the incubator implementation. There it is used as a raw pointer.

All of the uses after the call to takeContext() have lifetimes that should be limited to the scope of CIRGenConsumer::handleTranslationUnit(), so I think we should be able to just use it as a raw pointer here instead of taking it and leaving CIRGenerator with a null pointer, but CIRGenerator shouldn't need it again either.

@bcardosolopes Have I misunderstood anything here? What's your recommendation?

Copy link
Contributor Author

@andykaylor andykaylor Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps it's also relevant to note that I don't seem to need the context here in the current patch. It's only used as a unique pointer in the path where we're lowering CIR to other MLIR dialects before generating LLVM IR. So, I could remove this part of the change and defer the decision until it is needed.

Copy link
Collaborator

@erichkeane erichkeane Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could CIRGenerator own this and just share it with lowerFromCIRToLLVMIR/etc via reference? CIRGenerator seems to be the entry point into all of CIR, so it makes sense for me for it to 'own' the lifetime.

I don't see why handleTranslationUnit needs to take ownership of it. I also wonder if that taking of ownership is problematic, since of course, clang can in some configurations handle multiple translation units, which this could potentially lose (though IDK if we ended up enabling that functionality).

Copy link
Member

@lanza lanza Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could CIRGenerator own this and just share it with lowerFromCIRToLLVMIR/etc via reference? CIRGenerator seems to be the entry point into all of CIR, so it makes sense for me for it to 'own' the lifetime.

I think hoisting it to the CIRGenConsumer and just letting the CIRGenerator get a handle to it makes sense.

Alternatively, the CIRGenerator is done at this point. We could have some sort of destruction of the CIRGenerator here and have it give up it's state contents to the CIRGenConsumer. cc @bcardosolopes

Copy link
Member

@bcardosolopes bcardosolopes Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We run passes post CIRGen (LLVM lowering is one example) and at some point we were trying to build CIR in Sema and hand it over to codegen if already produced, so this probably dates back to that. Right now CIRGenerator holds the whole lifetime it's totally fine to go for a more clean solution here.

Btw, If you want to double check, you could run the cir tests with a ASAN enabled clang build, I usually use that from time to time to make sure the incubator isn't leaking.

std::unique_ptr<clang::CIRGen::CIRGenModule> cgm;

public:
Expand All @@ -55,8 +55,8 @@ class CIRGenerator : public clang::ASTConsumer {
void Initialize(clang::ASTContext &astContext) override;
bool HandleTopLevelDecl(clang::DeclGroupRef group) override;
mlir::ModuleOp getModule() const;
std::unique_ptr<mlir::MLIRContext> takeContext() {
return std::move(mlirContext);
std::shared_ptr<mlir::MLIRContext> getContext() {
Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a const member function?

return mlirContext;
};
};

Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/CIR/LowerToLLVM.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,4 +33,4 @@ lowerDirectlyFromCIRToLLVMIR(mlir::ModuleOp M, llvm::LLVMContext &Ctx);
} // namespace direct
} // namespace cir

#endif // CLANG_CIR_LOWERTOLLVM_H
#endif // CLANG_CIR_LOWERTOLLVM_H
2 changes: 1 addition & 1 deletion clang/lib/CIR/CodeGen/CIRGenerator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,7 @@ void CIRGenerator::Initialize(ASTContext &astContext) {

this->astContext = &astContext;

mlirContext = std::make_unique<mlir::MLIRContext>();
mlirContext = std::make_shared<mlir::MLIRContext>();
mlirContext->loadDialect<cir::CIRDialect>();
cgm = std::make_unique<clang::CIRGen::CIRGenModule>(
*mlirContext.get(), astContext, codeGenOpts, diags);
Expand Down
8 changes: 4 additions & 4 deletions clang/lib/CIR/FrontendAction/CIRGenAction.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ getBackendActionFromOutputType(CIRGenAction::OutputType Action) {

static std::unique_ptr<llvm::Module> lowerFromCIRToLLVMIR(
const clang::FrontendOptions &FEOpts, mlir::ModuleOp MLIRModule,
std::unique_ptr<mlir::MLIRContext> MLIRCtx, llvm::LLVMContext &LLVMCtx) {
std::shared_ptr<mlir::MLIRContext> MLIRCtx, llvm::LLVMContext &LLVMCtx) {
return direct::lowerDirectlyFromCIRToLLVMIR(MLIRModule, LLVMCtx);
}

Expand Down Expand Up @@ -72,7 +72,7 @@ class CIRGenConsumer : public clang::ASTConsumer {
void HandleTranslationUnit(ASTContext &C) override {
Gen->HandleTranslationUnit(C);
mlir::ModuleOp MlirModule = Gen->getModule();
auto MLIRCtx = Gen->takeContext();
std::shared_ptr<mlir::MLIRContext> MLIRCtx = Gen->getContext();
switch (Action) {
case CIRGenAction::OutputType::EmitCIR:
if (OutputStream && MlirModule) {
Expand All @@ -84,7 +84,7 @@ class CIRGenConsumer : public clang::ASTConsumer {
case CIRGenAction::OutputType::EmitLLVM: {
llvm::LLVMContext LLVMCtx;
auto LLVMModule = lowerFromCIRToLLVMIR(CI.getFrontendOpts(), MlirModule,
Copy link
Collaborator

@AaronBallman AaronBallman Jan 28, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please spell out the type instead of using auto.

std::move(MLIRCtx), LLVMCtx);
MLIRCtx, LLVMCtx);

BackendAction BEAction = getBackendActionFromOutputType(Action);
emitBackendOutput(
Expand Down Expand Up @@ -135,4 +135,4 @@ EmitCIRAction::EmitCIRAction(mlir::MLIRContext *MLIRCtx)

void EmitLLVMAction::anchor() {}
EmitLLVMAction::EmitLLVMAction(mlir::MLIRContext *MLIRCtx)
: CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {}
: CIRGenAction(OutputType::EmitLLVM, MLIRCtx) {}
2 changes: 1 addition & 1 deletion clang/lib/CIR/Lowering/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1 +1 @@
add_subdirectory(DirectToLLVM)
add_subdirectory(DirectToLLVM)
Loading