-
Notifications
You must be signed in to change notification settings - Fork 54
Local variable debug info deduplication #222
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
|
@jiel-nv Many thanks for the PR! I'll add it to my queue to review, but note I'm OOO until next Tuesday, so will have to pick it up after then. |
| and ( | ||
| name not in self._singly_assigned_vars | ||
| or self._disable_sroa_like_opt | ||
| ) | ||
| and not name.startswith("$") |
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 I comment out all these checks then none of the debuginfo tests fail. It's not immediately obvious to me what these conditions are for (I was commenting them out to try to learn what they did by what breaks when I do that) - are they really needed? If so, what for? Are there additional tests needed to check for the conditions these handle?
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.
Ah, I see now that the first two conditions are used in the superclass storevar() and suppresses storing the variable when they are true.
I think the last is because we don't emit debuginfo for internal names (those beginning with a $).
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.
Yes, you're right.
gmarkall
left a comment
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 think this generally looks good - there is one suggestion on the diff which I think will save time for future readers of the code (it took me quite a while to work out).
Adding comments explaining the conditions. Co-authored-by: Graham Markall <[email protected]>
gmarkall
left a comment
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! I think this is good to merge now but will wait until after the CI fixes (#238) are done.
- Local variable debug info deduplication (NVIDIA#222) - Fix package installation for wheels CI (NVIDIA#238) - Fix Invalid NVVM IR emitted when lowering shfl_sync APIs (NVIDIA#231) - Add Bfloat16 Low++ Bindings (NVIDIA#166) - Fix cuda.jit decorator inline (NVIDIA#181) - Feature: cuda specific make_attribute_wrapper (NVIDIA#193) - return a none tuple if no libdevice path is found (NVIDIA#234)
- Local variable debug info deduplication (#222) - Fix package installation for wheels CI (#238) - Fix Invalid NVVM IR emitted when lowering shfl_sync APIs (#231) - Add Bfloat16 Low++ Bindings (#166) - Fix cuda.jit decorator inline (#181) - Feature: cuda specific make_attribute_wrapper (#193) - return a none tuple if no libdevice path is found (#234)
This PR improves the debug info for local variables.
Fixes nvbug#5027648, nvbug#5009771, nvbug#5120628.