-
Notifications
You must be signed in to change notification settings - Fork 104
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][CIRGen] Support Lambda capturing this
object
#1213
Conversation
this
object
Seems like a unrecheable NYI case to me, if you disagree please elaborate |
I was thinking about to create an issue to see if anybody from community wants to work on it. :-) |
Good intent, thanks! I don't understand why being NYI would preclude open source contribution or filing issues though. Missing features are usually used for things that we can't afford to NYI at a certain moment because otherwise everything would fail on that path. This isn't the case for a C++23 we don't yet support.
NYI |
Sure, just removed the missing feature. Thanks for clarification about NYI and Missing feature. |
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.
Which unreachable is tracking the c++23 feature you mention? I don't see anything specific here nor comments that can help someone take that over.
Made changes according to review comments. I didn't change some of the |
clang/test/CIR/CodeGen/lambda.cpp
Outdated
// LLVM: [[THIS_ADDR:%.*]] = alloca ptr, i64 1, align 8 | ||
// LLVM: store ptr [[THIS]], ptr [[THIS_ADDR]], align 8 | ||
// LLVM: store ptr {{%.*}}, ptr [[THIS_ADDR]], align 8 |
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 see now, you were not using LABEL because you were matching the argument, my bad!
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.
You can keep THIS
for the case were you are matching the arg and use LABEL for the other ones (like I see you did below). Sorry for the churn
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 don't have the full context here, but if the problem is that CHECK-LABEL
can't contain variable references, a common pattern I've seen for that, assuming the variable references occur after a label bit, is to use CHECK-LABEL
for the label bit and then CHECK-SAME
for the variable references. May or may not be worth it depending on the situation though.
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.
Also works
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'll use the SAME
directive then, Another reason I want to check the argument is (in future) we should check attributes to argument(that's what the COM: line is about), This test seems to be a good case for that.
clang/test/CIR/CodeGen/lambda.cpp
Outdated
// CHECK: [[CLS_ANNO7:%.*]] = cir.load [[ARG_ADDR]] : !cir.ptr<!cir.ptr<!ty_anon2E7_>>, !cir.ptr<!ty_anon2E7_> | ||
// CHECK: [[STRUCT_A:%.*]] = cir.get_member [[CLS_ANNO7]][0] {name = "this"} : !cir.ptr<!ty_anon2E7_> -> !cir.ptr<!ty_A> | ||
// CHECK: [[a:%.*]] = cir.get_member [[STRUCT_A]][0] {name = "a"} : !cir.ptr<!ty_A> -> !cir.ptr<!s32i> loc(#loc70) | ||
// CHECK: cir.return {{%.*}} : !s32i |
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 return value here is important, please check it
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, will do
clang/test/CIR/CodeGen/lambda.cpp
Outdated
// CHECK: [[STRUCT_A_PTR:%.*]] = cir.get_member [[CLS_ANNO8]][0] {name = "this"} : !cir.ptr<!ty_anon2E8_> -> !cir.ptr<!cir.ptr<!ty_A>> | ||
// CHECK: [[STRUCT_A:%.*]] = cir.load [[STRUCT_A_PTR]] : !cir.ptr<!cir.ptr<!ty_A>>, !cir.ptr<!ty_A> | ||
// CHECK: [[a:%.*]] = cir.get_member [[STRUCT_A]][0] {name = "a"} : !cir.ptr<!ty_A> -> !cir.ptr<!s32i> loc(#loc70) | ||
// CHECK: cir.return {{%.*}} : !s32i |
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 return value here is important, please check it
The PR should help us to get rid of NYI
NYI UNREACHABLE executed at clang/lib/CIR/CodeGen/CIRGenExprAgg.cpp:899
Relevant OG code here:
I put
HasExplicitObjectParameter
support as a missing feature, which is a new C++23 feature.