Skip to content
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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

zjgarvey
Copy link
Contributor

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:

  1. Skip iterating through all results of an operation when the first result is not hoistable. This was my original fix for the crash, but it didn't address the fact that too many operations inside of these loops were still getting added to the analysis worklist (which seemed unintentional from the code).
  2. Don't consider values nested even deeper under a parent to be "escaping producers". This addressed the issue discussed in 1.

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.

@zjgarvey zjgarvey requested a review from benvanik as a code owner December 31, 2024 21:17
@zjgarvey
Copy link
Contributor Author

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?

Copy link
Collaborator

@benvanik benvanik left a 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.
Copy link
Collaborator

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.

Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants