-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[CIR] Initial implementation of CIR-to-LLVM IR lowering pass #125260
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.
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
Changes from 1 commit
4801886
6a2570c
ce2c8b0
410665a
d0c2866
0d20709
71729bd
4558b4a
bb74dff
660788f
bd73580
db4798b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -12,7 +12,9 @@ | |
| #ifndef CLANG_CIR_LOWERTOLLVM_H | ||
| #define CLANG_CIR_LOWERTOLLVM_H | ||
|
|
||
| #include "mlir/Pass/Pass.h" | ||
| #include "mlir/Dialect/LLVMIR/LLVMDialect.h" | ||
| #include "mlir/Transforms/DialectConversion.h" | ||
| #include "clang/CIR/Dialect/IR/CIRDialect.h" | ||
|
|
||
| #include <memory> | ||
|
|
||
|
|
@@ -31,6 +33,24 @@ namespace direct { | |
| std::unique_ptr<llvm::Module> | ||
| lowerDirectlyFromCIRToLLVMIR(mlir::ModuleOp mlirModule, | ||
| llvm::LLVMContext &llvmCtx); | ||
|
|
||
| class CIRToLLVMGlobalOpLowering | ||
| : public mlir::OpConversionPattern<cir::GlobalOp> { | ||
| mlir::DataLayout const &dataLayout; | ||
|
||
|
|
||
| public: | ||
| CIRToLLVMGlobalOpLowering(const mlir::TypeConverter &typeConverter, | ||
| mlir::MLIRContext *context, | ||
| mlir::DataLayout const &dataLayout) | ||
| : OpConversionPattern(typeConverter, context), dataLayout(dataLayout) { | ||
| setHasBoundedRewriteRecursion(); | ||
| } | ||
|
|
||
| mlir::LogicalResult | ||
| matchAndRewrite(cir::GlobalOp op, OpAdaptor adaptor, | ||
| mlir::ConversionPatternRewriter &rewriter) const override; | ||
| }; | ||
|
|
||
| } // namespace direct | ||
| } // namespace cir | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -12,7 +12,19 @@ | |||||
|
|
||||||
| #include "clang/CIR/LowerToLLVM.h" | ||||||
|
|
||||||
| #include "mlir/Conversion/LLVMCommon/TypeConverter.h" | ||||||
| #include "mlir/Dialect/DLTI/DLTI.h" | ||||||
| #include "mlir/Dialect/Func/IR/FuncOps.h" | ||||||
| #include "mlir/Dialect/LLVMIR/LLVMDialect.h" | ||||||
| #include "mlir/IR/BuiltinDialect.h" | ||||||
| #include "mlir/IR/BuiltinOps.h" | ||||||
| #include "mlir/Pass/Pass.h" | ||||||
| #include "mlir/Pass/PassManager.h" | ||||||
| #include "mlir/Target/LLVMIR/Dialect/Builtin/BuiltinToLLVMIRTranslation.h" | ||||||
| #include "mlir/Target/LLVMIR/Dialect/LLVMIR/LLVMToLLVMIRTranslation.h" | ||||||
| #include "mlir/Target/LLVMIR/Export.h" | ||||||
| #include "mlir/Transforms/DialectConversion.h" | ||||||
| #include "clang/CIR/Dialect/IR/CIRDialect.h" | ||||||
| #include "llvm/IR/Module.h" | ||||||
| #include "llvm/Support/TimeProfiler.h" | ||||||
|
|
||||||
|
|
@@ -22,13 +34,127 @@ using namespace llvm; | |||||
| namespace cir { | ||||||
| namespace direct { | ||||||
|
|
||||||
| struct ConvertCIRToLLVMPass | ||||||
| : public mlir::PassWrapper<ConvertCIRToLLVMPass, | ||||||
| mlir::OperationPass<mlir::ModuleOp>> { | ||||||
| void getDependentDialects(mlir::DialectRegistry ®istry) const override { | ||||||
| registry.insert<mlir::BuiltinDialect, mlir::DLTIDialect, | ||||||
| mlir::LLVM::LLVMDialect, mlir::func::FuncDialect>(); | ||||||
| } | ||||||
| void runOnOperation() final; | ||||||
|
|
||||||
| StringRef getDescription() const override { | ||||||
| return "Convert the prepared CIR dialect module to LLVM dialect"; | ||||||
| } | ||||||
|
|
||||||
| StringRef getArgument() const override { return "cir-flat-to-llvm"; } | ||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what does 'flat' mean here?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When control flow and scopes implemented in CIR, there will be nested regions describing that structure. When that happens another pass will be run before this one that flattens the CIR by inlining the nested regions to create a situation where all blocks in a function belong to the parent region. Here's a simple example showing what that looks like: https://godbolt.org/z/Ta5dTMfPn
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks! Wouldn't mind a comment somewhere explaining this is for 'flat' CIR only, as it is a bit of a sub-dialect it sounds?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yea, a comment might be good here since cir-flat hasn't been introduced just yet |
||||||
| }; | ||||||
|
|
||||||
| mlir::LogicalResult CIRToLLVMGlobalOpLowering::matchAndRewrite( | ||||||
| cir::GlobalOp op, OpAdaptor adaptor, | ||||||
| mlir::ConversionPatternRewriter &rewriter) const { | ||||||
|
|
||||||
| // Fetch required values to create LLVM op. | ||||||
| const mlir::Type cirSymType = op.getSymType(); | ||||||
|
|
||||||
| // This is the LLVM dialect type | ||||||
|
||||||
| // This is the LLVM dialect type | |
| // This is the LLVM dialect type. |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // These defaults are just here until the equivalent attributes are | |
| // FIXME: These default values are placeholders until the the equivalent attributes are available on cir.global ops. |
^^ Plus word-wrap.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps extract alignment, addrspace, and threadlocal here too?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds reasonable. I haven't dug into all the cases that will be handled here, so there might be some cases where that doesn't work, but I can at least put placeholders here for now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might be useful to introduce the MissingFeatures.h header to upstream so we can start tracking these little missing details.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /*alignment*/ 0, /*addrSpace*/ 0, /*dsoLocal*/ isDsoLocal, | |
| /*alignment=*/ 0, /*addrSpace=*/ 0, isDsoLocal, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Clang typically uses paramname=. Also, don't need a comment if the variable name tells you what it is.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already have 'float' types implemented in the Clang->CIR bit, could we perhaps add float here too if it is only a handful of lines? Would be nice to see what this is going to look like some day.
Also, I find myself wondering if this should be some sort of visitor pattern instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure. I have that implemented and was going to save it for the next patch along with some other types that are less straightforward, but I can bring in float types here if you like.
I'll think about whether a visitor pattern would work here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the else arm we already emit an error for not implemented things (like float in this example), IMO it should be fine as an incremental addition later on.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if (auto intAttr = mlir::dyn_cast<cir::IntAttr>(init.value())) { | |
| if (const auto *intAttr = mlir::dyn_cast<cir::IntAttr>(init.value())) { |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| /*addrSpace*/ 0, /*dsoLocal*/ isDsoLocal, /*threadLocal*/ false, | |
| /*addrSpace*/ 0, isDsoLocal, /*threadLocal*/ false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same other suggestions on comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its a shame there isn't an addConversions variadic version of this that we could just call all of these in 1 place. I see this comes from MLIR unfortunately, but perhaps something to suggest upstream elsewhere.
erichkeane marked this conversation as resolved.
Show resolved
Hide resolved
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| prepareTypeConverter(converter, dl); // , lowerModule.get()); | |
| prepareTypeConverter(converter, dl); |
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This variable is harming readability to me i think. The name isn't clear which direction it means, plus the double-not is REALLY confusing here :D I suspect just if (mlir::failed(pm.run(mlirModule)) would be most readable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I expect reporting a fatal error here is also not what we want long term. I'll do something to clean it up.
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| mlirModule, llvmCtx, moduleName ? *moduleName : "CIRToLLVMModule"); | |
| mlirModule, llvmCtx, moduleName.value_or("CIRToLLVMModule")); |
Better yet, make moduleName above a StringRef, and just do mlirModule.getName().value_or("CIRToLLVMModule"));
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,63 @@ | ||
| // Global variables of intergal types | ||
| // RUN: %clang_cc1 -std=c++20 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o - | FileCheck %s | ||
|
|
||
| // Note: Currently unsupported features include default zero-initialization | ||
| // and alignment. The fact that "external" is only printed for globals | ||
| // without an initializer is a quirk of the LLVM AsmWriter. | ||
|
|
||
| char c; | ||
| // CHECK: @c = external dso_local global i8 | ||
|
|
||
| signed char sc; | ||
| // CHECK: @sc = external dso_local global i8 | ||
|
|
||
| unsigned char uc; | ||
| // CHECK: @uc = external dso_local global i8 | ||
|
|
||
| short ss; | ||
| // CHECK: @ss = external dso_local global i16 | ||
|
|
||
| unsigned short us = 100; | ||
| // CHECK: @us = dso_local global i16 100 | ||
|
|
||
| int si = 42; | ||
| // CHECK: @si = dso_local global i32 42 | ||
|
|
||
| unsigned ui; | ||
| // CHECK: @ui = external dso_local global i32 | ||
|
|
||
| long sl; | ||
| // CHECK: @sl = external dso_local global i64 | ||
|
|
||
| unsigned long ul; | ||
| // CHECK: @ul = external dso_local global i64 | ||
|
|
||
| long long sll; | ||
| // CHECK: @sll = external dso_local global i64 | ||
|
|
||
| unsigned long long ull = 123456; | ||
| // CHECK: @ull = dso_local global i64 123456 | ||
|
|
||
| __int128 s128; | ||
| // CHECK: @s128 = external dso_local global i128 | ||
|
|
||
| unsigned __int128 u128; | ||
| // CHECK: @u128 = external dso_local global i128 | ||
|
|
||
| wchar_t wc; | ||
| // CHECK: @wc = external dso_local global i32 | ||
|
|
||
| char8_t c8; | ||
| // CHECK: @c8 = external dso_local global i8 | ||
|
|
||
| char16_t c16; | ||
| // CHECK: @c16 = external dso_local global i16 | ||
|
|
||
| char32_t c32; | ||
| // CHECK: @c32 = external dso_local global i32 | ||
|
|
||
| _BitInt(20) sb20; | ||
| // CHECK: @sb20 = external dso_local global i20 | ||
|
|
||
| unsigned _BitInt(48) ub48; | ||
| // CHECK: @ub48 = external dso_local global i48 |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,8 +1,10 @@ | ||
| // Smoke test for ClangIR-to-LLVM IR code generation | ||
| // RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o - | FileCheck %s | ||
|
|
||
| // TODO: Add checks when proper lowering is implemented. | ||
| // For now, we're just creating an empty module. | ||
| // CHECK: ModuleID | ||
| int a; | ||
|
|
||
| void foo() {} | ||
erichkeane marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| // CHECK: @a = external dso_local global i32 | ||
|
|
||
| int b = 2; | ||
|
|
||
| // CHECK: @b = dso_local global i32 2 | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is originally in
clang/lib/CIR/Lowering/DirectToLLVM/LowerToLLVM.h. Any reason you're promoting it to theincludetop level dir?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was an accident. This is a different file with the same name but different location. I think I missed that detail when I was moving this bit of code over from the incubator project.