[AMDGPU][NPM] Fix CFG invalidation detection in insertSimulatedTrap #169290
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When SIMULATED_TRAP is at the end of a block with no successors, insertSimulatedTrap incorrectly returns the original MBB despite adding HaltLoopBB to the CFG. EmitInstrWithCustomInserter detects CFG changes by comparing the returned MBB with the original. When they match, it assumes no modification occurred and skips MachineLoopInfo invalidation. This causes stale loop information in subsequent passes. Fix: Return HaltLoopBB to properly signal the CFG modification.
Member
|
@llvm/pr-subscribers-backend-amdgpu Author: Prasoon Mishra (PrasoonMishra) ChangesWhen SIMULATED_TRAP is at the end of a block with no successors, insertSimulatedTrap incorrectly returns the original MBB despite adding HaltLoopBB to the CFG. EmitInstrWithCustomInserter detects CFG changes by comparing the returned MBB with the original. When they match, it assumes no modification occurred and skips MachineLoopInfo invalidation. This causes stale loop information in subsequent passes. Fix: Return HaltLoopBB to properly signal the CFG modification. Full diff: https://github.com/llvm/llvm-project/pull/169290.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
index a7333e3373f38..9b05d99e265e6 100644
--- a/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInstrInfo.cpp
@@ -1963,6 +1963,10 @@ MachineBasicBlock *SIInstrInfo::insertSimulatedTrap(MachineRegisterInfo &MRI,
BuildMI(MBB, MI, DL, get(AMDGPU::S_CBRANCH_EXECNZ)).addMBB(TrapBB);
MF->push_back(TrapBB);
MBB.addSuccessor(TrapBB);
+ } else {
+ // Since we're adding HaltLoopBB and modifying the CFG, we must return a
+ // different block to signal the change.
+ ContBB = HaltLoopBB;
}
// Start with a `s_trap 2`, if we're in PRIV=1 and we need the workaround this
|
PrasoonMishra
changed the title
Contributor
Author
|
This bug was discovered while enabling the NPM pipeline for AMDGPU. Without this patch, the existing test trap-abis.ll fails under NPM. The Expected: This happens only when using NPM pipeline because legacy pass manager conservatively recomputes analyses, so MachineLoopInfo is refreshed before AsmPrinter, masking the bug. NPM relies on accurate invalidation signals hence when insertSimulatedTrap incorrectly signals “no CFG change,” MachineLoopInfo remains stale, causing missing loop headers. This fix resolves the bug. |
cdevadas
approved these changes
Nov 24, 2025
Collaborator
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/116/builds/21528 Here is the relevant piece of the build log for the reference |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When SIMULATED_TRAP is at the end of a block with no successors, insertSimulatedTrap incorrectly returns the original MBB despite adding HaltLoopBB to the CFG.
EmitInstrWithCustomInserter detects CFG changes by comparing the returned MBB with the original. When they match, it assumes no modification occurred and skips MachineLoopInfo invalidation. This causes stale loop information in subsequent passes, particularly when using the NPM which relies on accurate invalidation signals.
Fix: Return HaltLoopBB to properly signal the CFG modification.