[CIR] Refactor StructType with TableGen
#1504
Open
+166
−219
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 patch currently causes the test struct Foo {
void m1(int);
virtual void m2(int);
virtual void m3(int);
};
void call(Foo* obj, void (Foo::*func)(int), int arg)
{
(obj->*func)(arg);
}I have been debugging for some time, but haven't been able to resolve this issue. Any help would be appreciated. The backtrace is attached below: |
|
This should be considered a bug in the clangir/clang/include/clang/CIR/Dialect/IR/CIROps.td Lines 2897 to 2949 in dec9a17 Simply put, the introduction of To mitigate this problem, preserve the old definition of - def CIR_StructType : Type<CPred<"::mlir::isa<::cir::StructType>($_self)">,
- "CIR struct type">;
+ def Struct : Type<CPred<"::mlir::isa<::cir::StructType>($_self)">, "CIR struct type">;And then update the definition of - let arguments = (ins CIR_StructType:$record, IndexAttr:$index_attr);
+ let arguments = (ins Struct:$record, IndexAttr:$index_attr);Btw, you could build the code with |
Thank you for your assistance. I noticed that |
Yes. |
|
Amazing catch @Lancern |
Before renaming to |
|
I see unrelated changes in this PR (e.g. changes to STD ops), perhaps some bad merging on your end? You need to fix your branch somehow so that the diff we see only has the relevant bits. |
Lower vcales_f32
This PR adds an insertion guard for the try body scope for try-catch.
Currently, the following code snippet fails during CodeGen:
```
void foo() {
int r = 1;
try {
++r;
return;
} catch (...) {
}
}
```
The insertion point doesn't get reset properly and the cleanup is being
ran for a wrong/deleted block causing a segmentation fault. I also added
a test.
The comments suggested that we should use TableGen to generate the recognizing functions. However, I think templates might be more suitable for generating them -- and I can't find any existing TableGen backends that let us generate arbitrary functions. My choice of design is to offer a template to match standard library functions: ```cpp // matches std::find with 3 arguments, and raise it into StdFindOp StdRecognizer<3, StdFindOp, StdFuncsID::Find> ``` I have to use a TableGen'd enum to map names to IDs, as we can't pass string literals to template arguments easily in C++17. This also constraints design of future `StdXXXOp`s: they must take operands the same way of StdFindOp, where the first one is the original function, and the rest are function arguments. I'm not sure if this approach is the best way. Please tell me if you have concerns or any alternative ways.
…was set explicitly (llvm#1482) This is backported from a change made in llvm/llvm-project#131181 --------- Co-authored-by: Morris Hafner <mhafner@nvidia.com>
…R attribute. (llvm#1467) Started decorating CUDA shadow variables with the shadow_name CIR attribute which will be used for registering the globals.
Lower neon vcaltd_f64
… target was set explicitly" (llvm#1509) Reverts llvm#1482 @mmha this is crashing on macos on asserts build: ``` FAIL: Clang :: CIR/Tools/cir-translate/warn-default-triple.cir (472 of 552) ******************** TEST 'Clang :: CIR/Tools/cir-translate/warn-default-triple.cir' FAILED ******************** Exit Code: 134 Command Output (stdout): -- Assertion failed: (!DataLayoutString.empty() && "Uninitialized DataLayout!"), function getDataLayoutString, file TargetInfo.h, line 1282. ``` Perhaps besides picking a default you maybe need to do some missing datalayout init?
el-ev
force-pushed
the
cir_structtype_td
branch
from
b35d074 to
fc3b818
Compare
el-ev
commented
Mar 25, 2025
bcardosolopes
approved these changes
Mar 25, 2025
| } | ||
|
|
||
| // Note CIRStructType is used instead of CIR_StructType because of tablegen conflicts | ||
| def CIRStructType : Type<CPred<"::mlir::isa<::cir::StructType>($_self)">, "CIR struct type">; |
There was a problem hiding this comment.
One last nit: we should be bound to 80-col in tablegen files.
There was a problem hiding this comment.
Have you confirmed the rest of the diff is also bound? I see some suspect entries
There was a problem hiding this comment.
My apologies, I didn't check carefully enough... Everything should be in bounds now.
edit: oops, i forgot to push
el-ev
force-pushed
the
cir_structtype_td
branch
from
03f8fa3 to
0a7facc
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.
Closes #1367