-
Notifications
You must be signed in to change notification settings - Fork 1
[HeterogeneousDwarf] Support translation of DIOp-based DIExpressions #9
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
Conversation
Fixed by AMD-Lightning-Internal/llvm-project #1229
These expressions exist downstream on amd-staging and are necessary for debugging on device. Fixes SWDEV-445691.
afb36e5
to
370168f
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.
Took me a moment to get up-to-speed on the SPIRV representation, so I may have missed something, but aside from a few nits this LGTM!
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'm not sure why the default is to just link to the GV and not look for the GVE, but I suppose we only really care about the NewElements case anyway for working symbolic debug info, so this LGTM!
(Maybe I answered my question there, and the point is to actively avoid using faulty expressions for the non-heterogeneous case?)
@epilk something I had not considered, but is important, is that this breaks compatibility with upstream AFAICT (for example the DIExprOps header is not available), and we would like the fork to still build with upstream. Perhaps a |
Ah, okay I didn't realize that the downstream translator would need to work with upstream llvm. Using |
Thank you and apologies for the inconvenience. I spaced out on this when I reviewed. |
No problem, thanks for spotting the issue! #14 |
These expressions exist downstream on amd-staging and are necessary for
debugging on device. Fixes SWDEV-445691.