-
Notifications
You must be signed in to change notification settings - Fork 156
[CIR][CodeGen] Introduce CIR CXXSpecialMember attribute #1711
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
base: main
Are you sure you want to change the base?
Conversation
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 adding more of the building blocks here. In the future it'd be nice to have a representation for struct/class more closer to C++ and it will probably be easier to extract this type of information, but to prevent over engineering early, it feels right to incrementally add pieces that can enable us to better analyze C++ code and make transformations easier to write.
Can you look into changing LifetimeChecker.cpp to use this attribute instead of the current AST approach?
(@andykaylor @erichkeane @dkolsen-pgi in case you have any extra thoughts 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.
Do we also want to set this attribute on cir.func definitions/declarations?
68ff6ef
to
8c7235c
Compare
β With the latest revision this PR passed the C/C++ code formatter. |
A few updates:
|
77c8851
to
d4c51f3
Compare
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.
1 comment, else seems reasonable, I'm happy when the others are.
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 the update, one more round of suggestions
ab65415
to
c1f8a62
Compare
c1f8a62
to
f6aaa4d
Compare
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 the updates, one more round!
a5f2b05
to
c40a840
Compare
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 patience, more comments!
if (auto oa = parser.parseOptionalAttribute(ctorAttr); oa.has_value()) | ||
state.addAttribute(cxxSpecialMemberAttr, ctorAttr); | ||
if (auto oa = parser.parseOptionalAttribute(dtorAttr); oa.has_value()) | ||
state.addAttribute(cxxSpecialMemberAttr, dtorAttr); |
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 oa = parser.parseOptionalAttribute(ctorAttr); oa.has_value()) | |
state.addAttribute(cxxSpecialMemberAttr, ctorAttr); | |
if (auto oa = parser.parseOptionalAttribute(dtorAttr); oa.has_value()) | |
state.addAttribute(cxxSpecialMemberAttr, dtorAttr); | |
if (auto oa = parser.parseOptionalAttribute(ctorAttr)) | |
state.addAttribute(cxxSpecialMemberAttr, ctorAttr); | |
if (auto oa = parser.parseOptionalAttribute(dtorAttr)) | |
state.addAttribute(cxxSpecialMemberAttr, dtorAttr); |
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.
parser.parseOptionalAttribute
returns mlir::OptionalParseResult
, so the oa.has_value()
part is necessary to make the condition boolean.
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 are not using oa
, so might as well do if (parser.parseOptionalAttribute(dtorAttr).has_value())
?
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 are not using
oa
, so might as well doif (parser.parseOptionalAttribute(dtorAttr).has_value())
?
right, updated!
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 pending last discussion item
I think this one is self-explanatory, so I will not write much πβ
Adding this attribute helps in optimizations like #1653, and using the attribute it's easy to create operations like
cir.std.vector.ctor
/cir.std.vector.dtor
by just modifyingIdiomRecognizer
a bit. I believe it will also be useful for future optimizations. Finally, I updated quite a number of tests so they now reflect this attribute.Please, let me know if you see any issues.