Skip to content

[MachineBasicBlock] Fix SlotIndexUpdater for insertion order #69424

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

Merged
merged 2 commits into from
Oct 21, 2023

Conversation

perlfu
Copy link
Contributor

@perlfu perlfu commented Oct 18, 2023

Follow up fix for #68786 to address that MachineFunction handleInsertion is actually called before a new instruction has been inserted into the block. Hence new instructions must be recorded and SlotIndex updates performed after the delegate call.

Follow up fix for llvm#68786 to address that MachineFunction
handleInsertion is actually called before a new instruction has
been inserted into the block. Hence new instructions must be
recorded and SlotIndex updates performed after the delegate call.
@perlfu perlfu requested review from jayfoad and qcolombet October 18, 2023 06:49
Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Was there a test case for the original bug?

@qcolombet
Copy link
Collaborator

Was there a test case for the original bug?

+1, please add a test case if possible.

@perlfu
Copy link
Contributor Author

perlfu commented Oct 19, 2023

Test case is in #69429 -- needs associated PHI Elimination changes.
Unless you are suggesting I add a unit test?

@jayfoad
Copy link
Contributor

jayfoad commented Oct 19, 2023

Test case is in #69429 -- needs associated PHI Elimination changes.

So if you tried to apply #69429 without this patch, then we would see failures in some asan buildbots? If so, I think this patch is fine as-is.

Unless you are suggesting I add a unit test?

I wasn't suggesting that.

@perlfu perlfu merged commit 386f390 into llvm:main Oct 21, 2023
Guzhu-AMD pushed a commit to GPUOpen-Drivers/llvm-project that referenced this pull request Oct 26, 2023
Local branch amd-gfx 124a34d Merged main:a3937c46d0ea into amd-gfx:100685c57eaa
Remote branch main 386f390 [MachineBasicBlock] Fix SlotIndexUpdater for insertion order (llvm#69424)
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.

3 participants