[CIR] Cleanup cir.scopes with a single cir.yield operation #482
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This diverges from the original codegen by tracking all members of a union, instead of just the largest one. This is necessary to support type-checking at the MLIR level when accessing union members. It also preserves more information about the source code, which might be useful. Fixes llvm#224 ghstack-source-id: 8a975426d077a66c49f050741d7362da3c102fed Pull Request resolved: llvm#229
Converts a union to a struct containing only its largest element. GetMemberOp for unions is lowered as bitcasts instead of GEPs, since union members share the same address space. ghstack-source-id: 744ac312675b8f3225ccc459fcd09474bcfcfe81 Pull Request resolved: llvm#230
…lvm#241) Similar with the previous ctor support, I'm bringing up the dtor support for global var initialization. This change only contains the CIR early codegen work. The upcoming lowering prepare work will be in a separate change.
This change adds lowering support for global variable definition with
destructor.
For example:
```
cir.global "private" internal @_ZL8__ioinit = ctor : !ty_22Init22 {
%0 = cir.get_global @_ZL8__ioinit : cir.ptr <!ty_22Init22>
%1 = cir.const(#true) : !cir.bool
cir.call @_ZN4InitC1Eb(%0, %1) : (!cir.ptr<!ty_22Init22>, !cir.bool) -> ()
} dtor {
%0 = cir.get_global @_ZL8__ioinit : cir.ptr <!ty_22Init22>
cir.call @_ZN4InitD1Ev(%0) : (!cir.ptr<!ty_22Init22>) -> ()
}
```
is now lowered to
```
cir.func internal private @__cxx_global_var_init() {
%0 = cir.get_global @_ZL8__ioinit : cir.ptr <!ty_22Init22>
%1 = cir.const(#true) : !cir.bool
cir.call @_ZN4InitC1Eb(%0, %1) : (!cir.ptr<!ty_22Init22>, !cir.bool) -> ()
%2 = cir.get_global @_ZL8__ioinit : cir.ptr <!ty_22Init22>
%3 = cir.get_global @_ZN4InitD1Ev : cir.ptr <!cir.func<!void (!cir.ptr<!ty_22Init22>)>>
%4 = cir.cast(bitcast, %3 : !cir.ptr<!cir.func<!void (!cir.ptr<!ty_22Init22>)>>), !cir.ptr<!cir.func<!void (!cir.ptr<!void>)>>
%5 = cir.cast(bitcast, %2 : !cir.ptr<!ty_22Init22>), !cir.ptr<!void>
%6 = cir.get_global @__dso_handle : cir.ptr <i8>
cir.call @__cxa_atexit(%4, %5, %6) : (!cir.ptr<!cir.func<!void (!cir.ptr<!void>)>>, !cir.ptr<!void>, !cir.ptr<i8>) -> ()
cir.return
}
```
Note that instead calling the destructor in the global initializer
function, a registration with `__cxa_atexit` is done instead so that the
destructor will be called right before the program is shut down.
Add two new CIR ops: cir.alloc_exception and cir.throw, they are higher level representations for their __cxa_* counterparts. Add CIRGen for a simple `throw <expr>` example, which requires using the above added operations. Rethrow mechanism (plain `throw`) is still NYI. For LLVM lowering: LoweringPrepare work needs to be done next, breaking these ops back to functions calls.
Add an assert while we don't handle the available externally.
Add lots of necessary boilerplate for personality functions, catch block tracking and building catch scope stack. We enter the try but no catching handling yet. New paths all guarded with asserts, therefore NFC.
In order to support exceptions, skeleton for unwind detection.
…call attributes soon
Populate regions and basic blocks for handling try-catch! No testcases just yet, new path guarded with unreachable, so effectively this is NFC.
This PR removes the method `hasBooleanRepresentation` as was discussed in [PR#233](llvm#233) Briefly, the `cir.bool` has the same representation in memory and after load. The LLVM IR differs, that's why the check is there, in the origin `CodeGen`. Also, in order to trigger the path and make the implementation to be conform with the original `CodeGen`, there are changes in the `CIRGenExprScalar`: call the common `buildFromLValue` instead of manaul `load` creation. Also, a couple of tests for the bool load/store added
…n. (llvm#253) Introducing `cir.ConstPtrAttr` to represent arbitrary absolute pointer value initializations. Also incorporating previous `cir.nullptr` effort into this work.
Following discussion in llvm#237 this adds support for `fabs` builtins which are used extensively in llvm-test-suite.
Temporary workaround until we patch the codegen for record types.
This patch brings up the basic support for C++ virtual inheritance. VTT (virtual table table) now can be laid out as expected for simple program with single virtual inheritance. RTTI support is on the way. This patch does not include LLVM lowering support.
This PR introduces bitfelds support. This now works:
```
#include <stdio.h>
typedef struct {
int a1 : 4;
int a2 : 28;
int a3 : 16;
int a4 : 3;
int a5 : 17;
int a6 : 25;
} A;
void init(A* a) {
a->a1 = 1;
a->a2 = 321;
a->a3 = 15;
a->a4 = -2;
a->a5 = -123;
a->a6 = 1234;
}
void print(A* a) {
printf("%d %d %d %d %d %d\n",
a->a1,
a->a2,
a->a3,
a->a4,
a->a5,
a->a6
);
}
int main() {
A a;
init(&a);
print(&a);
return 0;
}
```
the output is:
`1 321 15 -2 -123 1234`
- Introduces `CIR/Interfaces/ASTAttrInterfaces` which model API of clang AST nodes, but allows to plugin custom attribute, making `CIR` dialect AST independent. - Extends hierarchy of `DeclAttr`s to model `Decl` attributes more faithfully. - Notably all `CIRASTAttr`s are now created uniformly using `makeAstDeclAttr` which builds corresponding Attribute based on `clang::Decl`.
…lvm#263) There is a bug in the code generation: the field index for the `GetMemberOp` is taken from the `FieldDecl`, with no respect to the record layout. One of the manifestation of the bug is the wrong index generated for a field in a derived class that does not take into the account the instance of the base class (that has index 0). You can take a look at the example in `test/CIR/CodeGen/derived-to-base.cpp`, i.e. the current test is not the correct one ``` // CHECK-DAG: !ty_22C23A3ALayer22 = !cir.struct<class "C2::Layer" {!ty_22C13A3ALayer22, !cir.ptr<!ty_22C222> // CHECK-DAG: !ty_22C33A3ALayer22 = !cir.struct<struct "C3::Layer" {!ty_22C23A3ALayer22 // CHECK: cir.func @_ZN2C35Layer10InitializeEv // CHECK: cir.scope { // CHECK: %2 = cir.base_class_addr(%1 : cir.ptr <!ty_22C33A3ALayer22>) -> cir.ptr <!ty_22C23A3ALayer22> // CHECK: %3 = cir.get_member %2[0] {name = "m_C1"} : !cir.ptr<!ty_22C23A3ALayer22> -> !cir.ptr<!cir.ptr<!ty_22C222>> ``` As one can see, the result (of ptr type to `!ty_22C222` ) must have the index `1` in the corresponded struct `ty_22C23A3ALayer22`. Basically the same is done in the `clang/CodeGen/CGExpr.cpp`, so we don't invent something new here. Note, this fix doesn't affect anything related to the usage of `buildPreserveStructAccess` where the `field->getFieldIndex()` is used.
This PR adds MLIR lowering of `cir.scope`. I also notice that the MLIR unit tests still uses old integer types. I will fix those in a separate PR.
Recent work on exceptions broke an internal testcase, free the path back while I work on a holistic solution.
The change is taken from the original llvm codegen.
Originally those are only used for debug info generation, so get a bit more specific on what's missing here.
When replacing the no-proto functions with it's real definition, codegen assumes that only `cir.call` operation may use the replaced function. Such behaviour leads to compilation error because of the `cir.get_global` op can also use the function to get pointer to function. This PR adds handle the case with `cir.get_global` operation and fixes the issue.
This PR adds missing case to lowerCirAttrAsValue.
…r.try
The final destination here is to support cir.try_calls that are not within a
`try {}` statement in C++. This only affect untested paths that will
assert a bit later than before, testcase coming soon.
The second part of the job started in llvm#412 , now about local structures. As it was mentioned previously, sometimes the layout for structures with bit fields inited with constants differ from the originally created in `CIRRecordLayoutBuilder` and it cause `storeOp` verification fail due to different structure type was used to allocation. This PR fix it. An example: ``` typedef struct { int a : 4; int b : 5; int c; } D; void bar () { D d = {1,2,3}; } ``` Well, I can't say I'm proud of these changes - it seems like a type safety violation, but looks like it's the best we can do here. The original codegen doesn't have this problem at all, there is just a `memcpy` there, I provide LLVM IR just for reference: ``` %struct.D = type { i16, i32 } @__const.bar.d = private unnamed_addr constant { i8, i8, i32 } { i8 33, i8 0, i32 3 }, align 4 ; Function Attrs: noinline nounwind optnone uwtable define dso_local void @bar() #0 { entry: %d = alloca %struct.D, align 4 call void @llvm.memcpy.p0.p0.i64(ptr align 4 %d, ptr align 4 @__const.bar.d, i64 8, i1 false) ret void } ```
In llvm#426 we confirmed that CIR needs a `cir.unreachable` operation to mark unreachable program points [(discussion)](llvm#426 (comment)). This PR adds it.
This PR adds support for evaluating constants in member exprs. The change is taken from original codegen.
- Add it to functions but not yet on calls. - Add more skeleton for tagging function attributes. - Testcases One more incremental step towards cir.try_call outside cir.try scopes.
The next step in inline-assembly support: we add instruction operands! Nothing interesting, just some copy-pasta from the `codegen` with some sort of simplifications for now. Well, I'm not sure `functional-type` is the best way to print operands though it's used in mlir's `InlineAsmOp`. But anyways, maybe you have a better idea. There are two or three steps ahead, so we are not that far from being able to run something!
One more tiny step!
This a tiny PR that adds clobbers to constraint string.
Note, that `~{dirflag},~{fpsr},~{flags}` is a
[X86](https://github.com/llvm/clangir/blob/main/clang/lib/Basic/Targets/X86.h#L281)
dependent clobbers.
Basically, the next things remain:
- lowering
- store the results of the `cir.asm`
You can test this locally with the following command:git-clang-format --diff b9cd201198ee61b57cfbdfced69c9fe2f3710cf5 a984329e4fcbc220ec681e6ed1b9ee2aad20c487 -- clang/lib/CIR/Dialect/Transforms/MergeCleanups.cppView the diff from clang-format here.diff --git a/clang/lib/CIR/Dialect/Transforms/MergeCleanups.cpp b/clang/lib/CIR/Dialect/Transforms/MergeCleanups.cpp
index b12f4c9f08..42dcb6e965 100644
--- a/clang/lib/CIR/Dialect/Transforms/MergeCleanups.cpp
+++ b/clang/lib/CIR/Dialect/Transforms/MergeCleanups.cpp
@@ -59,10 +59,10 @@ struct RemoveEmptyScope : public OpRewritePattern<ScopeOp> {
LogicalResult match(ScopeOp op) const final {
return success(op.getRegion().empty() ||
- (op.getRegion().getBlocks().size() == 1 &&
- op.getRegion().front().empty()) ||
- (op.getRegion().front().getOperations().size() == 1 &&
- isa<YieldOp>(&op.getRegion().front().front())));
+ (op.getRegion().getBlocks().size() == 1 &&
+ op.getRegion().front().empty()) ||
+ (op.getRegion().front().getOperations().size() == 1 &&
+ isa<YieldOp>(&op.getRegion().front().front())));
}
void rewrite(ScopeOp op, PatternRewriter &rewriter) const final {
|
|
Thanks, this also needs a testcase! |
|
Thanks for this comment. Just to clarify, I should add the test case to clangir/tree/main/clang/test/CIR/Transforms/merge-cleanups.cir, right? |
That'd be a good place for it |
Lancern
reviewed
Feb 24, 2024
| op.getRegion().front().empty())); | ||
| (op.getRegion().getBlocks().size() == 1 && | ||
| op.getRegion().front().empty()) || | ||
| (std::distance(op.getRegion().front().begin(), op.getRegion().front().end()) == 1 && |
There was a problem hiding this comment.
To test a block has only one operation, you can use:
op.getRegion().front().getOperations().size() == 1Which is clearer.
Expected behavior is that scope blocks with only a single yield op are cleaned up.
lanza
force-pushed
the
main
branch
2 times, most recently
from
ed3955a
to
8b7417c
Compare
|
Any plans to continue the work here? |
lanza
force-pushed
the
main
branch
2 times, most recently
from
c4db6d0
to
e197d4e
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR adds an extra condition to the match pattern in the RemoveEmptyScope cleanup pass to consider the case where there is a single operation in the block and that it is YieldOp.
Issue #455
Related to #436