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.
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][Lowering] Add support for attribute annotate #804
[CIR][CIRGen][Lowering] Add support for attribute annotate #804
Changes from all commits
ded7c37
68c76a3
8f25b3d
0fa5fb8
7052d16
7dbb5c0
f4dd9e4
92f9541
2f064d0
5ba1a52
b13e3ee
bb08f29
c192b0b
03fc0aa
77beec4
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Please add an example in CIR of how this looks 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.
While debugging some core dump in this function, I wonder what happens here when e is not a
clang::ConstantExpr
?@ghehg an idea about the missing comments here? 😄
No need for some
IgnoreParenCasts()
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.
I am still diving into it. Please ignore my comments since I got the inheritance wrong.
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.
It crashes on my use-case when
e
is aConstantExpr
which is not handled. I can handle it with a PR but I need to understand the neighborhood first.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.
OK. Actually everything is a
ConstantExpr
here. Just that I have anenum
which is not lowered to its underlying type.Thinking about 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.
Thank you for looking into this! As you might notice that the logic is actually from clang codegen.
clangir/clang/lib/CodeGen/CodeGenModule.cpp
Line 3284 in d00c3a3
I think the assumption is that as argument to annotation, everything should be ConstantExpr, so I guess we just trust clang front end wouldn't shoot its own foot by generating other types of AST expr for annotation arg values.
But you could find an example that breaks the assumption, what I'd do is to use clang code gen (removing -fclangir from opt list), and if you can reproduce it. That's an issue we should open at upstream, and of course, we could fix it there to be ahead of upstream :-)
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 "normal" clang is able to evaluate the
ConstantExp
beyond just literals.I have made #874 for integers.