-
Notifications
You must be signed in to change notification settings - Fork 15.4k
[AMDGPU][NPM] Preserve analyses in AMDGPURewriteAGPRCopyMFMA for NPM #170130
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
[AMDGPU][NPM] Preserve analyses in AMDGPURewriteAGPRCopyMFMA for NPM #170130
Conversation
preserve liveIntervals, VirtRegMapAnalysis and LiveRegMatrix analysis for AMDGPURewriteAGPRCopyMFMAPass (as in legacy) specifically not preserving VirtRegMap causes it to be invalidated and thus VirtRegRewriter has invalid info next in pipeline
…yMFMA for NPM AMDGPURewriteAGPRCopyMFMAPass claimed to preserve LiveIntervalsAnalysis but did not preserve its dependency, SlotIndexesAnalysis. Under NPM, this causes LiveIntervals to be invalidated and recomputed. Fix by explicitly preserving SlotIndexesAnalysis.
|
@llvm/pr-subscribers-backend-amdgpu Author: Prasoon Mishra (PrasoonMishra) ChangesThe pass preserved LiveStacksAnalysis but failed to preserve LiveIntervalsAnalysis, LiveRegMatrixAnalysis, VirtRegMapAnalysis, and SlotIndexesAnalysis under NPM. This caused these analyses to be invalidated and recomputed, leading to incorrect behavior in subsequent passes like VirtRegRewriter. Fix by explicitly preserving all required analyses in the NPM version, matching the legacy pass manager behavior. Full diff: https://github.com/llvm/llvm-project/pull/170130.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp b/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
index 89c16dadb4b41..095f23ad22d97 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
+++ b/llvm/lib/Target/AMDGPU/AMDGPURewriteAGPRCopyMFMA.cpp
@@ -32,6 +32,7 @@
#include "llvm/CodeGen/LiveStacks.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
+#include "llvm/CodeGen/SlotIndexes.h"
#include "llvm/CodeGen/VirtRegMap.h"
#include "llvm/InitializePasses.h"
@@ -659,7 +660,11 @@ AMDGPURewriteAGPRCopyMFMAPass::run(MachineFunction &MF,
if (!Impl.run(MF))
return PreservedAnalyses::all();
auto PA = getMachineFunctionPassPreservedAnalyses();
- PA.preserveSet<CFGAnalyses>();
- PA.preserve<LiveStacksAnalysis>();
+ PA.preserveSet<CFGAnalyses>()
+ .preserve<LiveStacksAnalysis>()
+ .preserve<VirtRegMapAnalysis>()
+ .preserve<SlotIndexesAnalysis>()
+ .preserve<LiveIntervalsAnalysis>()
+ .preserve<LiveRegMatrixAnalysis>();
return PA;
}
|
|
Without this patch, the following lit test fail when NPM is enabled: This patch aligns the NPM behavior with the legacy pass manager (see AMDGPURewriteAGPRCopyMFMA.cpp Line 607-610). |
| PA.preserveSet<CFGAnalyses>() | ||
| .preserve<LiveStacksAnalysis>() | ||
| .preserve<VirtRegMapAnalysis>() | ||
| .preserve<SlotIndexesAnalysis>() |
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 guess SlotIndex is not directly used here, but invoked inside LiveIntervals?
It doesn't seem good to me to explicitly preserve the analyses that aren't explicitly used in a pass. Rather, the pass manager should have some way to preserve them. In this case LiveIntervals analysis is preserved here. The pass-manager infrastructure should preserve the SlitIndex too. Anyway, it doesn't work that way at the moment.
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 pointing that out. I agree the current approach isn’t ideal, but given the limitations of the pass manager right now, I’ll leave it as is.
…lvm#170130) The pass preserved LiveStacksAnalysis but failed to preserve LiveIntervalsAnalysis, LiveRegMatrixAnalysis, VirtRegMapAnalysis, and SlotIndexesAnalysis under NPM. This caused these analyses to be invalidated and recomputed, leading to incorrect behavior in subsequent passes like VirtRegRewriter. Fix by explicitly preserving all required analyses in the NPM version, matching the legacy pass manager behavior. --------- Co-authored-by: vikhegde <vikram.hegde@amd.com>
The pass preserved LiveStacksAnalysis but failed to preserve LiveIntervalsAnalysis, LiveRegMatrixAnalysis, VirtRegMapAnalysis, and SlotIndexesAnalysis under NPM. This caused these analyses to be invalidated and recomputed, leading to incorrect behavior in subsequent passes like VirtRegRewriter.
Fix by explicitly preserving all required analyses in the NPM version, matching the legacy pass manager behavior.