-
Notifications
You must be signed in to change notification settings - Fork 100
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
[CIR][Dialect] Introduce StdInitializerListOp to represent high-level semantics of C++ initializer list #1121
base: main
Are you sure you want to change the base?
Conversation
f04fa03
to
90c2cca
Compare
cc @smeenai |
Thanks for looking at this! I think the high-level idea is right here, but I'm wondering if we want to lift up the InitListExpr node instead of the CXXStdInitializerListExpr node. From what I can see in this change, we're still constructing the init list via a sequence of stores, and then have the higher-level op to get an initializer_list from that init list. That's definitely an improvement over the status quo, but having the init list itself be represented directly would be even better. You raised the question of struct vs array inits in #777, and I think we can keep them separate. One other interesting difference is for InitListExprs that directly initialize an array vs. become part of an initializer_list; the former are stored in constant memory and the latter constructed via stores on the stack (https://godbolt.org/z/Ee47xcoaM). Have you looked into what causes that difference? Understanding why the codegen for those two cases differs would help us figure out the appropriate high-level design. |
I agree with you. I will try to re-design
And then "stored in constant memory" or "constructed via stores on the stack" can be determined during the lowering WDYT? |
@smeenai Is this something you describe in origin PR? I don't do store in constant memory when lowering LLVM now but it should be possible. |
✅ With the latest revision this PR passed the C/C++ code formatter. |
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.
Thank you! This will be a very nice improvement :)
At a high level, I think we want the op to look like (see also https://llvm.github.io/clangir/Dialect/cir-style-guide.html):
%0 = cir.std_initializer_list %elem1, %elem2, %elem3 : !s32i -> !ty_std3A3Ainitializer_list3Cint3E
It'll probably be simpler to decompose that into lower-level ops inside LoweringPrepare instead of lowering it to LLVM directly.
In the future we may want to lift up other forms of init lists as well, but just tackling std::initializer_list
in this PR is a good starting point (and then we can incrementally build upon that in the future).
@@ -4900,6 +4900,12 @@ def ClearCacheOp : CIR_Op<"clear_cache", [AllTypesMatch<["begin", "end"]>]> { | |||
}]; | |||
} | |||
|
|||
def InitListCtor : CIR_Op<"initlist.ctor"> { | |||
let summary = "Starts a variable argument list"; |
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 needs updating.
@@ -4900,6 +4900,12 @@ def ClearCacheOp : CIR_Op<"clear_cache", [AllTypesMatch<["begin", "end"]>]> { | |||
}]; | |||
} | |||
|
|||
def InitListCtor : CIR_Op<"initlist.ctor"> { | |||
let summary = "Starts a variable argument list"; | |||
let arguments = (ins CIR_PointerType:$initlist, |
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 it'd be better if the argument was just the arg list, and the op produced a result of the appropriate type. That way you can add the SameTypeOperands
trait to verify that all the operand types are the same. You'll also want to verify that the result type is consistent with the argument type; LoadOp
uses the TypesMatchWith
trait for that, but our logic is a bit more complex, so you might need to implement a custom verifier (https://mlir.llvm.org/docs/DefiningDialects/Operations/#custom-verifier-code).
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 we do like this, we need to treat std::initializer_list
as scalar instead of aggregate. I don't think it is a good way just for simplify the verify CIR.
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.
Let's push this discussion further and materialize our conclusion on it into the doc of this new op.
If we do like this, we need to treat
std::initializer_list
as scalar instead of aggregate. I don't think it is a good way just for simplify the verify CIR.
From my understanding, "as scalar instead of aggregate" means treating initializer_list
in high-level CIR as an value, or an pointer to value.
Let me humbly summarize both of your opinions here:
- A new pure SSA value for
initializer_list
, which probably introduces several new op with no extraordinary benefits, is of course not what we want. - But we can fuse
cir.alloca
and currentcir.initializer_list.ctor
op intocir.initializer_list
, which has result type!cir.ptr<!tyInitList>
. That way we may get cleaner argument list, without evident IR design issue.- This benefit not only the verification, but also assembly format and possibly passes that touch the high-level op.
- But I'm not sure that implicit
alloca
semantic won't cause any issue. Assumptions about memory operations should be taken care of.
Basically I think @HerrCai0907's current design is nice enough. Given our goal is to capture the list of values in initializer_list
, only abstracting the constructor of it sounds reasonable to me. @smeenai Wdyt?
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.
My original idea was that std::initializer_list
is just a struct, and we can treat that struct as an SSA value. Clang doesn't usually do so because it always performs ABI lowering, but we perform ABI lowering as a separate transform. Taking https://godbolt.org/z/dz88MrfWW as an example, note the lines:
%17 = cir.load %2 : !cir.ptr<!ty_std3A3Ainitializer_list3Cint3E>, !ty_std3A3Ainitializer_list3Cint3E
cir.call @_ZNSt6vectorIiEC1ESt16initializer_listIiE(%1, %17) : (!cir.ptr<!ty_std3A3Avector3Cint3E>, !ty_std3A3Ainitializer_list3Cint3E) -> ()
%17
is an SSA value of type std::initializer_list<int>
(which is just a cir.struct
type with a particular format) being passed by value to a function, and I was thinking this op could behave similarly. Basically, right now the op usage looks like (in pseudo-CIR):
%init.alloc = cir.alloca std::initializer_list<int>
cir.initializer_list.ctor %init.alloc(1, 2, 3)
// somewhere later
%init.load = cir.load %init.alloc
cir.call some_function(%init.load)
We could instead theoretically have:
%init = cir.initializer_list.ctor 1, 2, 3
// somewhere later
cir.call some_function(%init)
We'd end up with the same result after lowering prepare, but the CIR before that would look nicer IMO. I haven't thought through all the implications of that though (in particular for cases like https://godbolt.org/z/PsK7d8PrG, which unfortunately runs into an assertion failure right now but where we'd still need the alloca), and I'm still pretty new to the project myself, so I might be missing something obvious :) I'm also fine with the current design.
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.
the process of codegen is like this, we check the expression type, if it a scalar, we want a op which return a value. if it an aggregate type, we ensure the memory and init it according to ptr.
It looks weird to do
if type.isRecord:
if type.name == std::initializer_list:
return scalar
else:
return aggregate
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.
Got it, your implementation is the way to go then.
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.
Thanks for your efforts to delay its lowering to LoweringPrepare! The approach looks good to me (of course please continue the discussion with @smeenai in the previous comment). Just some suggestions on engineering~
//===----------------------------------------------------------------------===// | ||
|
||
def StdInitializerListOp : CIR_Op<"std_initializer_list.ctor"> { | ||
let summary = "create std::initializer_list"; |
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.
More docs with example needed.
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.
LGTM except for some nits. I recommend you not to solve them until you push the discussion further about the extra argument pointer of init list.
And we should update the doc of operation after that.
}]; | ||
let arguments = (ins StructPtr:$initList, Variadic<CIR_AnyType>:$args); | ||
let assemblyFormat = [{ | ||
$initList `:` type($initList) `(` ($args^ `:` type($args))? `)` attr-dict |
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.
The assembly format is a bit confusing:
cir.std_initializer_list.ctor %0 : !cir.ptr<!init_list>(%2, %4 : !cir.ptr<!s8i>, !cir.ptr<!s8i>)
- This intuitively mimics function call syntax, but it's the struct type of init list that is right before the left parenthesis.
- Redundantly presenting a list of values with types, which also occurs in
cir.vec.create
syntax is a bit confusing #541 .
Not a critical problem, you can improve it any later if you like : )
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 agree, it's a bit confusing. Let's decide what the op will look like below and then we can figure out the assembly format based on that.
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 short term, I think just moving the initList
type to the end is good enough for me. In the long term, it'd be nice if we could omit the types of the args entirely (since they can be inferred from the initlist type), but I don't have a sense of how involved that'd be, so it doesn't need to be part of this diff. Using {}
delimiters instead of ()
would be more initializer-list like, but that can also be confused with regions, so I'm not sure if it's actually better. The discussion in #541 looks super relevant, thanks for linking it.
```cpp | ||
initializer_list<int> v{1,2,3}; // initialize v with 1, 2, 3 | ||
``` |
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.
Thanks, a C++ example is nice. But it would be nicer to further show how this line of code is modelled in ClangIR with your new operation.
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.
Yes, please also add that!
Apologies on the delay, I'm also taking a look at this soon! |
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 looks great to me. I'd like for seven-mile or Bruno to do one more pass over it though, since they have much more experience here. Thanks for the work and bearing with all the comments!
}]; | ||
let arguments = (ins StructPtr:$initList, Variadic<CIR_AnyType>:$args); | ||
let assemblyFormat = [{ | ||
$initList `:` type($initList) `(` ($args^ `:` type($args))? `)` attr-dict |
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 short term, I think just moving the initList
type to the end is good enough for me. In the long term, it'd be nice if we could omit the types of the args entirely (since they can be inferred from the initlist type), but I don't have a sense of how involved that'd be, so it doesn't need to be part of this diff. Using {}
delimiters instead of ()
would be more initializer-list like, but that can also be confused with regions, so I'm not sure if it's actually better. The discussion in #541 looks super relevant, thanks for linking it.
auto builder = CGF.getBuilder(); | ||
assert(llvm::isa<MaterializeTemporaryExpr>(E->getSubExpr())); | ||
auto *subExpr = | ||
llvm::cast<MaterializeTemporaryExpr>(E->getSubExpr())->getSubExpr(); |
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.
Minor: cast
will fail if the type doesn't match, so I don't think the isa
assertion gets you anything additional (here and below).
auto resultType = mlir::dyn_cast_if_present<cir::StructType>( | ||
mlir::cast<cir::PointerType>(getInitList().getType()).getPointee()); | ||
if (resultType == nullptr) | ||
return emitOpError("std::initialize_list must be '!cir.struct'"); |
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.
Typo (in a couple of other places too).
return emitOpError("std::initialize_list must be '!cir.struct'"); | |
return emitOpError("std::initializer_list must be '!cir.struct'"); |
} | ||
|
||
// FIXME(cir): better handling according to different field type. [ptr ptr], | ||
// [ptr size], [size ptr]. |
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.
What's this FIXME referring to? You're handling both [ptr ptr]
and [ptr size]
below, and I don't think [size ptr]
is used. What improvements did you have in mind?
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.
According to standard, it does not define the member of std::initializer_list
. So it can be [ptr ptr], [ptr size]. But it is also possible to put the size as first member and ptr as second member. I keep it here as FIXME because I don't want to change the behavior a lot with the old CG to make reviewing easier.
The loop was erasing the user of a value while iterating on the value's users, which results in a use after free. We're already assuming (and asserting) that there's only one user, so we can just access it directly instead. CIR/Transforms/Target/x86_64/x86_64-call-conv-lowering-pass.cpp was failing with ASAN before this change. We're now ASAN-clean except for llvm#829 (which is also in progress).
Reland llvm#638 This was reverted due to llvm#655. I tried to address the problem in the newest commit. The changes of the PR since the last landed one includes: - Move the definition of `cir::CIRGenConsumer` to `clang/include/clang/CIRFrontendAction/CIRGenConsumer.h`, and leave its `HandleTranslationUnit` interface is left empty. So that `cir::CIRGenConsumer` won't need to depend on CodeGen any more. - Change the old definition of `cir::CIRGenConsumer` in `clang/lib/CIR/FrontendAction/CIRGenAction.cpp` and to `CIRLoweringConsumer`, inherited from `cir::CIRGenConsumer`, which implements the original `HandleTranslationUnit` interface. I feel this may improve the readability more even without my original patch.
This PR fixes the lowering for multi dimensional arrays. Consider the following code snippet `test.c`: ``` void foo() { char arr[4][1] = {"a", "b", "c", "d"}; } ``` When ran with `bin/clang test.c -Xclang -fclangir -Xclang -emit-llvm -S -o -`, It produces the following error: ``` ~/clangir/llvm/include/llvm/Support/Casting.h:566: decltype(auto) llvm::cast(const From&) [with To = mlir::ArrayAttr; From = mlir::Attribute]: Assertion `isa<To>(Val) && "cast<Ty>() argument of incompatible type!"' failed. ``` The bug can be traced back to `LoweringHelpers.cpp`. It considers the values in the array as integer types, and this causes an error in this case. This PR updates `convertToDenseElementsAttrImpl` when the array contains string attributes. I have also added one more similar test. Note that in the tests I used a **literal match** to avoid matching as regex, so `!dbg` is useful.
Support expressions at the top level such as const unsigned int n = 1234; const int &r = (const int&)n; Reviewers: bcardosolopes Pull Request: llvm#857
This is to match clang CodeGen
As title. Also introduced buildAArch64NeonCall skeleton, which is partially the counterpart of OG's EmitNeonCall. And this could be use for many other neon intrinsics. --------- Co-authored-by: Guojin He <[email protected]>
See the test for example.
This PR adds aarch64 big endian support. Basically the support for aarch64_be itself is expressed only in two extra cases for the switch statement and changes in the `CIRDataLayout` are needed to prove that we really support big endian. Hence the idea for the test - I think the best way for proof is something connected with bit-fields, so we compare the results of the original codegen and ours.
This PR splits the old `cir-simplify` pass into two new passes, namely `cir-canonicalize` and `cir-simplify` (the new `cir-simplify`). The `cir-canonicalize` pass runs transformations that do not affect CIR-to-source fidelity much, such as operation folding and redundant operation elimination. On the other hand, the new `cir-simplify` pass runs transformations that may significantly change the code and break high-level code analysis passes, such as more aggresive code optimizations. This PR also updates the CIR-to-CIR pipeline to fit these two new passes. The `cir-canonicalize` pass is moved to the very front of the pipeline, while the new `cir-simplify` pass is moved to the back of the pipeline (but still before lowering prepare of course). Additionally, the new `cir-simplify` now only runs when the user specifies a non-zero optimization level on the frontend. Also fixed some typos and resolved some `clang-tidy` complaints along the way. Resolves llvm#827 .
…1125) Currently, the final `target triple` in LLVM IR is set in `CIRGenAction`, which is not executed by cir tools like `cir-translate`. This PR delay its assignment to LLVM lowering, enabling sharing the emitting of `target triple` between different invoking paths.
…bsOp to take vector input (llvm#1099) Extend AbsOp to take vector of int input. With it, we can support __builtin_elementwise_abs. We should in the next PR extend FpUnaryOps to support vector type input so we won't have blocker to implement all elementwise builtins completely. Now just temporarily have missingFeature `fpUnaryOPsSupportVectorType`. Currently, int type UnaryOp support vector type. FYI: [clang's documentation about elementwise builtins](https://clang.llvm.org/docs/LanguageExtensions.html#vector-builtins)
…vm#1102) This is a NFC patch that moves declaration from LowerToLLVM.cpp. The motivation of the patch is, we hope we can use the abilities from MLIR's standard dialects without lowering **ALL** clangir operation to MLIR's standard dialects. For example, currently we have 86 operations in LowerToLLVM.cpp but only 45 operations under though MLIR. It won't be easy to add proper lowering for all operation to **different** dialects. I think the solution may be to allow **mixed** IR. So that we can lowering CIR to MLIR's standard dialects partially and we can use some existing analysis and optimizations in MLIR and then we can lower all of them (the MLIR dialects and unlowered clangir) to LLVM IR. The hybrid IR is one of the goals of MLIR as far as I know. NOTE: I completely understand that the DirectlyLLVM pipeline is the tier-1 pipeline that we want to support. The idea above won't change this. I just want to offer some oppotunities for the downstream projects and finally some chances to improve the overall ecosystem.
…, neon_splatq_lane and neon_splatq_laneq (llvm#1126)
This is going to be raised in follow up work, which is hard to do in one go because createBaseClassAddr goes of the OG skeleton and ideally we want ApplyNonVirtualAndVirtualOffset to work naturally. This also doesn't handle null checks, coming next.
Now that we fixed the dep on VBase, clean up the rest of the function.
…e BaseClassAddrOp
It was always the intention for `cir.cmp` operations to return bool result. Due to missing constraints, a bug in codegen has slipped in which created `cir.cmp` operations with result type that matches the original AST expression type. In C, as opposed to C++, boolean expression types are "int". This resulted with extra operations being codegened around boolean expressions and their usage. This commit both enforces `cir.cmp` in the op definition and fixes the mentioned bug.
This is the first patch to support TBAA, following the discussion at llvm#1076 (comment) - add skeleton for CIRGen, utilizing `decorateOperationWithTBAA` - add empty implementation in `CIRGenTBAA` - introduce `CIR_TBAAAttr` with empty body - attach `CIR_TBAAAttr` to `LoadOp` and `StoreOp` - no handling of vtable pointer - no LLVM lowering
) The title describes the purpose of the PR. It adds initial support for structures with padding to the call convention lowering for AArch64. I have also _initial support_ for the missing feature [FinishLayout](https://github.com/llvm/clangir/blob/5c5d58402bebdb1e851fb055f746662d4e7eb586/clang/lib/AST/RecordLayoutBuilder.cpp#L786) for records, and the logic is gotten from the original codegen. Finally, I added a test for verification.
…m#1152) The function `populateCIRToLLVMConversionPatterns` contains a spaghetti of LLVM dialect conversion patterns, which results in merge conflicts very easily. Besides, a few patterns are even registered for more than once, possibly due to careless resolution of merge conflicts. This PR attempts to mitigate this problem. Pattern names now are sorted in alphabetical order, and each source code line now only lists exactly one pattern name to reduce potential merge conflicts.
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.
Thanks a bunch for working on this, some minor nits
```cpp | ||
initializer_list<int> v{1,2,3}; // initialize v with 1, 2, 3 | ||
``` |
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.
Yes, please also add that!
// StdInitializerListOp | ||
//===----------------------------------------------------------------------===// | ||
|
||
def StdInitializerListOp : CIR_Op<"std_initializer_list.ctor"> { |
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.
To match the other std op, I'd expect this to be instead named std.initializer_list.ctor
, and I don't think we need to put ctor in the name (maybe in the future if there's another related op we can then come up with a way to differentiate, but it doesn't seem to be one?). I suggest std.init.list
or std.init_list
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.
FWIW, I prefer std.initializer_list
to std.init_list
, just to make the correspondence with std::initializer_list
super clear.
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.
sounds good
auto resultType = mlir::dyn_cast_if_present<cir::StructType>( | ||
mlir::cast<cir::PointerType>(getInitList().getType()).getPointee()); | ||
if (resultType == nullptr) | ||
return emitOpError("std::initializer_list must be '!cir.struct'"); |
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.
Because you already added the constraint to tablegen in StructPtr
you could just a direct cast, no need to check for nullptr here
if (resultType.getMembers().size() != 2) | ||
return emitOpError( | ||
"std::initializer_list must be '!cir.struct' with two fields"); | ||
auto memberPtr = mlir::dyn_cast<cir::PointerType>(resultType.getMembers()[0]); |
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.
similar here, the result is also constrained by StructPtr
, so no need for the dyn_cast, a regular cast will do it 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.
Here we cast the first member of result type to pointer type. It does not be defined in tablegen.
"'!cir.ptr', but provided ") | ||
<< resultType.getMembers()[0]; | ||
auto expectedType = memberPtr.getPointee(); | ||
for (mlir::Value const &arg : getArgs()) |
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.
move the const
to before mlir::Value
…ltinExpr (llvm#1133) This PR is a NFC as we just NYI every builtID of neon SISD. We will implement them in subsequent PRs.
…CFG (llvm#1147) This PR implements NYI in CIRScopeOpFlattening. It seems to me the best way is to let results of ScopeOp forwarded as block arguments of the last block split from the cir.scope block.
b3aa5a1
to
17e66b3
Compare
17e66b3
to
280029b
Compare
I don't finish all work about
cir.initlist
. But I want to get some feedback about the cir design to make sure I am in correct way.Fixed: #777