-
Notifications
You must be signed in to change notification settings - Fork 638
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
[Util] Fix an assert getting reached for certain nested loops in HoistIntoGlobals
#19576
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: zjgarvey <[email protected]>
A question: I'm not familiar with the heuristics for when it is advantageous to hoist into globals. The lit test I submitted here (although it is kind of silly) seems consteval-able, but does not get hoisted. Is this something that would be useful to look into? |
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.
(will take a look at the logic whenever I wake up - it's NYE and my anniversary :)
|
||
// ----- | ||
|
||
// Prior to this patch, a bug caused %3#0 to be considered an escaping producer for %1. |
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.
use issue numbers when referencing things - "this patch" won't mean much after this lands :)
the description here is a bit too much for a test - it encodes assumptions/situations that are very specific to the current implementation of the code and will get out of date fast - it's useful to include comments as to what a test is verifying but it doesn't need the whole back story and prior behavior, just what's expected - if the test ever starts failing as someone is changing code they care about what situation they are trying to make work, not how it did/didn't work previously
we generally want to avoid "it doesn't crash" tests as it doesn't help anyone coming along working on the code - "not crashing" is a weak test that doesn't prove the behavior does anything but not crash - those are better for large bulk test corpuses or indirectly via e2e tests - if adding a test and fixing code then a test should be added for the behavior being modified/fixed/etc. here, for example, whatever ops or attributes caused the crash need to be CHECKed.
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, Ben. I'll create an issue to link with and look for a different way to test these changes. I found it difficult to add a test since I'm essentially only changing the analysis phase of this pass: the actual behavior on IR should not be affected.
Testing a batched aten.multinomial
op e2e would be sufficient to cover this, but that might also need some changes from #19563 and #19556 to pass.
I was trying to compile a batched binary search program (which has an unused return on a while loop), and noticed it reached an assert during
HoistIntoGlobals
. I narrowed down what was happening, and decided to make two changes to address the failure:Despite the fact that 2. alone would resolve the issue, I decided to keep the changes mentioned in 1., since the early continue is more efficient.