[AMDGPU][CodeGen] Incrementally update reserved regs for SIPreAllocateWWMRegs pass in RegisterClassInfo - #212201
[AMDGPU][CodeGen] Incrementally update reserved regs for SIPreAllocateWWMRegs pass in RegisterClassInfo#212201nkotikal wants to merge 10 commits into
Conversation
… bitvector without invalidating full object
|
@llvm/pr-subscribers-llvm-regalloc @llvm/pr-subscribers-backend-amdgpu Author: Nikhil Kotikalapudi (nkotikal) ChangesBased on this TODO comment in SIPreAllocateWWMRegs.cpp: 76 // TODO: Update RCI with the additional reserved registers the pass sets.
77 AU.addRequired<MachineRegisterClassInfoWrapperPass>();Creates function updateReservedRegs in RegisterClassInfo.cpp which takes the BitVector containing reserved register information without requiring any unnecessary recomputations of the entire RegisterClassInfo object.
Also drops the unconditional compute(RC) in computePSetLimit, which was redundant since getNumAllocatableRegs already recomputes stale entries. This is within the scope of my PR because it would unnecessarily rebuild an already-valid entry, rendering the incremental update pointless. Full diff: https://github.com/llvm/llvm-project/pull/212201.diff 5 Files Affected:
diff --git a/llvm/include/llvm/CodeGen/RegisterClassInfo.h b/llvm/include/llvm/CodeGen/RegisterClassInfo.h
index 256277832db24..53e7faba12555 100644
--- a/llvm/include/llvm/CodeGen/RegisterClassInfo.h
+++ b/llvm/include/llvm/CodeGen/RegisterClassInfo.h
@@ -98,6 +98,16 @@ class RegisterClassInfo {
LLVM_ABI void runOnMachineFunction(const MachineFunction &MF,
bool Rev = false);
+ /// allows modification of current reserved register vector
+ /// without invalidating RCI and triggering recomputation when possible
+ /// prereqs for use:
+ /// RCI already initialized,
+ /// the caller updated MRI's reserved vector
+ /// note: target information, callee-saved regs, cost, and alloc order
+ /// must not change.
+ /// input: MRI's current frozen vector
+ LLVM_ABI void updateReservedRegs(const BitVector &ReservedInput);
+
LLVM_ABI bool invalidate(MachineFunction &, const PreservedAnalyses &PA,
MachineFunctionAnalysisManager::Invalidator &) {
auto PAC = PA.getChecker<MachineRegisterClassAnalysis>();
diff --git a/llvm/lib/CodeGen/RegisterClassInfo.cpp b/llvm/lib/CodeGen/RegisterClassInfo.cpp
index f4b9e8d9b1704..fcb2f6f8d65e7 100644
--- a/llvm/lib/CodeGen/RegisterClassInfo.cpp
+++ b/llvm/lib/CodeGen/RegisterClassInfo.cpp
@@ -123,6 +123,81 @@ void RegisterClassInfo::runOnMachineFunction(const MachineFunction &mf,
}
}
+void RegisterClassInfo::updateReservedRegs(const BitVector &ReservedInput) {
+ assert(MF && TRI && RegClass &&
+ "RegisterClassInfo must be initialized before updating reserved regs");
+ assert(ReservedInput.size() == Reserved.size() &&
+ "Reserved register bit vectors must have the same size");
+ if (ReservedInput == Reserved)
+ return;
+
+ // Cached orders cannot regain unreserved registers; recompute them lazily.
+ bool OnlyNewReservations = Reserved.subsetOf(ReservedInput);
+
+ // subtracts reserved set from input set to get newly reserved regs
+ BitVector NewReservations = ReservedInput;
+ NewReservations.reset(Reserved);
+
+ Reserved = ReservedInput;
+
+ // Pressure limits depend on the number of allocatable registers.
+ std::fill_n(PSetLimits.get(), TRI->getNumRegPressureSets(), 0);
+
+ // NumRegs may hide entries beyond the stress limit, so those orders cannot
+ // safely be compacted using only their visible prefix.
+ if (!OnlyNewReservations || StressRA) {
+ ++Tag;
+ return;
+ }
+
+ for (const TargetRegisterClass &RC : TRI->regclasses()) {
+ RCInfo &Info = RegClass[RC.getID()];
+
+ // skip if class info is out of date
+ if (Info.Tag != Tag)
+ continue;
+
+ // Recomputed below, once every order has been narrowed.
+ Info.ProperSubClass = false;
+
+ unsigned NewNumRegs = 0;
+ uint8_t MinCost = uint8_t(~0u);
+ uint8_t LastCost = uint8_t(~0u);
+ unsigned LastCostChange = 0;
+
+ for (unsigned I = 0; I != Info.NumRegs; ++I) {
+ MCPhysReg PhysReg = Info.Order[I];
+ if (NewReservations.test(PhysReg))
+ continue;
+
+ uint8_t Cost = RegCosts[PhysReg];
+ MinCost = std::min(MinCost, Cost);
+ if (Cost != LastCost)
+ LastCostChange = NewNumRegs;
+
+ Info.Order[NewNumRegs++] = PhysReg;
+ LastCost = Cost;
+ }
+
+ Info.NumRegs = NewNumRegs;
+ Info.MinCost = MinCost;
+ Info.LastCostChange = LastCostChange;
+ }
+
+ // ProperSubClass depends on both this class and its superclass counts, so
+ // calculate it only after all valid orders have been compacted.
+ for (const TargetRegisterClass &RC : TRI->regclasses()) {
+ RCInfo &Info = RegClass[RC.getID()];
+ if (Info.Tag != Tag)
+ continue;
+
+ if (const TargetRegisterClass *Super =
+ TRI->getLargestLegalSuperClass(&RC, *MF))
+ if (Super != &RC && getNumAllocatableRegs(Super) > Info.NumRegs)
+ Info.ProperSubClass = true;
+ }
+}
+
/// compute - Compute the preferred allocation order for RC with reserved
/// registers filtered out. Volatile registers come first followed by CSR
/// aliases ordered according to the CSR order specified by the target.
@@ -206,7 +281,6 @@ void RegisterClassInfo::compute(const TargetRegisterClass *RC) const {
unsigned RegisterClassInfo::computePSetLimit(unsigned Idx) const {
const TargetRegisterClass *RC = TRI->getLargestRegClassForRegPressureSet(Idx);
assert(RC && "Failed to find register class");
- compute(RC);
unsigned NAllocatableRegs = getNumAllocatableRegs(RC);
unsigned RegPressureSetLimit = TRI->getRegPressureSetLimit(*MF, Idx);
// If all the regs are reserved, return raw RegPressureSetLimit.
diff --git a/llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp b/llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp
index bf484cef98da4..c7ed1b0d02e6b 100644
--- a/llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp
+++ b/llvm/lib/Target/AMDGPU/SIPreAllocateWWMRegs.cpp
@@ -17,13 +17,11 @@
#include "MCTargetDesc/AMDGPUMCTargetDesc.h"
#include "SIMachineFunctionInfo.h"
#include "llvm/ADT/PostOrderIterator.h"
-#include "llvm/CodeGen/LiveDebugVariables.h"
#include "llvm/CodeGen/LiveIntervals.h"
#include "llvm/CodeGen/LiveRegMatrix.h"
#include "llvm/CodeGen/MachineFrameInfo.h"
#include "llvm/CodeGen/MachineFunctionPass.h"
#include "llvm/CodeGen/RegisterClassInfo.h"
-#include "llvm/CodeGen/SlotIndexes.h"
#include "llvm/CodeGen/VirtRegMap.h"
#include "llvm/InitializePasses.h"
@@ -45,7 +43,7 @@ class SIPreAllocateWWMRegs {
LiveIntervals *LIS;
LiveRegMatrix *Matrix;
VirtRegMap *VRM;
- const RegisterClassInfo &RegClassInfo;
+ RegisterClassInfo &RegClassInfo;
std::vector<unsigned> RegsToRewrite;
#ifndef NDEBUG
@@ -56,7 +54,7 @@ class SIPreAllocateWWMRegs {
public:
SIPreAllocateWWMRegs(LiveIntervals *LIS, LiveRegMatrix *Matrix,
- VirtRegMap *VRM, const RegisterClassInfo &RCI)
+ VirtRegMap *VRM, RegisterClassInfo &RCI)
: LIS(LIS), Matrix(Matrix), VRM(VRM), RegClassInfo(RCI) {}
bool run(MachineFunction &MF);
};
@@ -73,14 +71,8 @@ class SIPreAllocateWWMRegsLegacy : public MachineFunctionPass {
AU.addRequired<LiveIntervalsWrapperPass>();
AU.addRequired<VirtRegMapWrapperLegacy>();
AU.addRequired<LiveRegMatrixWrapperLegacy>();
- // TODO: Update RCI with the additional reserved registers the pass sets.
AU.addRequired<MachineRegisterClassInfoWrapperPass>();
- AU.setPreservesCFG();
- AU.addPreserved<LiveIntervalsWrapperPass>();
- AU.addPreserved<SlotIndexesWrapperPass>();
- AU.addPreserved<VirtRegMapWrapperLegacy>();
- AU.addPreserved<LiveRegMatrixWrapperLegacy>();
- AU.addPreserved<LiveDebugVariablesWrapperLegacy>();
+ AU.setPreservesAll();
MachineFunctionPass::getAnalysisUsage(AU);
}
};
@@ -175,8 +167,10 @@ void SIPreAllocateWWMRegs::rewriteRegs(MachineFunction &MF) {
RegsToRewrite.clear();
- // Update the set of reserved registers to include WWM ones.
+ // Update the set of reserved registers to include WWM ones
+ // without unnecessarily invalidating RegClassInfo
MRI->freezeReservedRegs();
+ RegClassInfo.updateReservedRegs(MRI->getReservedRegs());
}
#ifndef NDEBUG
@@ -208,7 +202,7 @@ bool SIPreAllocateWWMRegsLegacy::runOnMachineFunction(MachineFunction &MF) {
auto *LIS = &getAnalysis<LiveIntervalsWrapperPass>().getLIS();
auto *Matrix = &getAnalysis<LiveRegMatrixWrapperLegacy>().getLRM();
auto *VRM = &getAnalysis<VirtRegMapWrapperLegacy>().getVRM();
- const auto &RCI = getAnalysis<MachineRegisterClassInfoWrapperPass>().getRCI();
+ auto &RCI = getAnalysis<MachineRegisterClassInfoWrapperPass>().getRCI();
return SIPreAllocateWWMRegs(LIS, Matrix, VRM, RCI).run(MF);
}
@@ -280,12 +274,7 @@ SIPreAllocateWWMRegsPass::run(MachineFunction &MF,
auto *LIS = &MFAM.getResult<LiveIntervalsAnalysis>(MF);
auto *Matrix = &MFAM.getResult<LiveRegMatrixAnalysis>(MF);
auto *VRM = &MFAM.getResult<VirtRegMapAnalysis>(MF);
- const auto &RCI = MFAM.getResult<MachineRegisterClassAnalysis>(MF);
+ auto &RCI = MFAM.getResult<MachineRegisterClassAnalysis>(MF);
SIPreAllocateWWMRegs(LIS, Matrix, VRM, RCI).run(MF);
- // The pass reserves WWM registers, invalidating RegisterClassInfo's
- // allocation order, so it cannot be preserved (see the legacy
- // getAnalysisUsage above).
- PreservedAnalyses PA = PreservedAnalyses::all();
- PA.abandon<MachineRegisterClassAnalysis>();
- return PA;
+ return PreservedAnalyses::all();
}
diff --git a/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-invalidate-rci.mir b/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-invalidate-rci.mir
deleted file mode 100644
index 6571294bac741..0000000000000
--- a/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-invalidate-rci.mir
+++ /dev/null
@@ -1,32 +0,0 @@
-# RUN: llc -mtriple=amdgpu7.00-amd-amdhsa -passes="require<machine-register-class-info>,si-pre-allocate-wwm-regs,require<machine-register-class-info>" -debug-pass-manager -filetype=null %s 2>&1 | FileCheck %s
-
-# INFO: Test that MachineRegisterClassInfo is not preserved in WWM preallocation
-
-# CHECK: Running analysis: MachineRegisterClassAnalysis on test_wwm_reserved
-# CHECK: Running pass: SIPreAllocateWWMRegsPass on test_wwm_reserved
-# CHECK: Invalidating analysis: MachineRegisterClassAnalysis on test_wwm_reserved
-# CHECK: Running analysis: MachineRegisterClassAnalysis on test_wwm_reserved
-
----
-name: test_wwm_reserved
-tracksRegLiveness: true
-frameInfo:
- maxAlignment: 4
-stack:
- - { id: 0, type: spill-slot, size: 4, alignment: 4, stack-id: sgpr-spill }
-machineFunctionInfo:
- isEntryFunction: false
- scratchRSrcReg: '$sgpr0_sgpr1_sgpr2_sgpr3'
- stackPtrOffsetReg: '$sgpr32'
- frameOffsetReg: '$sgpr33'
- hasSpilledSGPRs: true
-body: |
- bb.0:
- liveins: $sgpr4, $vgpr2_vgpr3
- SI_SPILL_S32_SAVE killed $sgpr4, %stack.0, implicit $exec, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit $sgpr32
- S_NOP 0
- renamable $sgpr4 = SI_SPILL_S32_RESTORE %stack.0, implicit $exec, implicit $sgpr0_sgpr1_sgpr2_sgpr3, implicit $sgpr32
- %0:vgpr_32 = V_MOV_B32_e32 20, implicit $exec
- GLOBAL_STORE_DWORD $vgpr2_vgpr3, %0:vgpr_32, 0, 0, implicit $exec
- SI_RETURN
-...
diff --git a/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-preserve-rci.mir b/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-preserve-rci.mir
new file mode 100644
index 0000000000000..9b2793795e35c
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/si-pre-allocate-wwm-regs-preserve-rci.mir
@@ -0,0 +1,27 @@
+# RUN: llc -mtriple=amdgpu9.0a -passes="require<machine-register-class-info>,si-pre-allocate-wwm-regs,require<machine-register-class-info>" -debug-pass-manager -filetype=null %s 2>&1 | FileCheck %s
+# RUN: llc -mtriple=amdgpu9.0a -passes=si-pre-allocate-wwm-regs -o - %s | FileCheck %s --check-prefix=MIR
+
+# INFO: Test that WWM preallocation updates MachineRegisterClassInfo in place
+# instead of invalidating it, so the analysis is not recomputed afterwards.
+
+# CHECK: Running analysis: MachineRegisterClassAnalysis on test_wwm_reserved
+# CHECK: Running pass: SIPreAllocateWWMRegsPass on test_wwm_reserved
+# CHECK-NOT: Invalidating analysis: MachineRegisterClassAnalysis on test_wwm_reserved
+# CHECK-NOT: Running analysis: MachineRegisterClassAnalysis on test_wwm_reserved
+
+# MIR: wwmReservedRegs:
+# MIR-NEXT: - '$vgpr0'
+
+---
+name: test_wwm_reserved
+tracksRegLiveness: true
+body: |
+ bb.0:
+ liveins: $sgpr1
+ %0:vgpr_32 = IMPLICIT_DEF
+ renamable $sgpr4_sgpr5 = ENTER_STRICT_WWM -1, implicit-def $exec, implicit-def $scc, implicit $exec
+ %1:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
+ %2:vgpr_32 = V_MOV_B32_dpp %1, %0, 323, 12, 15, 0, implicit $exec
+ $exec = EXIT_STRICT_WWM killed renamable $sgpr4_sgpr5
+ %3:vgpr_32 = COPY %0
+...
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
c69e285 to
5d78fe8
Compare
| @@ -98,6 +98,16 @@ class RegisterClassInfo { | |||
| LLVM_ABI void runOnMachineFunction(const MachineFunction &MF, | |||
| bool Rev = false); | |||
|
|
|||
| /// allows modification of current reserved register vector | |||
There was a problem hiding this comment.
Format comments according to the coding standards. They should be full sentences with capital letter and full stop. Avoid needless abbreviations like "prereqs" and "alloc" in the text. Also I don't understand "without invalidating RCI and triggering recomputation when possible".
How about adding a restriction that you can only use this method to add registers to the reserved set, not to remove any?
There was a problem hiding this comment.
How about adding a restriction that you can only use this method to add registers to the reserved set, not to remove any?
We do have a potential use case for removing reserved registers. Fundamentally it shouldn't be a problem to remove from the set
There was a problem hiding this comment.
It's tricky because reserved regs is a list of registers not regunits. If you remove a register, how do you efficiently work out which of its aliases are still reserved?
There was a problem hiding this comment.
I want to stop tracking reservations in terms of registers and move to using regunits
There was a problem hiding this comment.
Fixed the comments, thank you.
Initially, I did implement that restricting when I asserted the RCI's reserved snapshot was a subset of the input, but llvm/test/CodeGen/AMDGPU/llvm.sponentry.ll shows that it's possible this will not always be the case for PreAllocateWWMRegs, so I replaced the assertion with the OnlyNewReservations boolean, where as you can see it does fall back to enabling recomputation.
Are there any specific changes I should make now regarding this?
1577876 to
f3290df
Compare
f3290df to
de9ab11
Compare
| if (ReservedInput == Reserved) | ||
| return; | ||
|
|
||
| // Cached orders cannot regain unreserved registers; recompute them lazily. |
There was a problem hiding this comment.
For AMDGPU we do have uses for re-introducing reserved registers, but it would require writing a new optimization pass. We have to reserve registers to manage spilling, but if we know after RA there are no spills, we can free those up and make use of them
There was a problem hiding this comment.
We can reserve that for a future PR then right? Are there any specific changes I need to make here?
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | ||
| # RUN: -passes="require<machine-register-class-info>,si-pre-allocate-wwm-regs,require<machine-register-class-info>" \ | ||
| # RUN: -debug-pass-manager -o /dev/null %s 2>&1 | FileCheck %s | ||
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | ||
| # RUN: -passes=si-pre-allocate-wwm-regs -o - %s | FileCheck %s --check-prefix=MIR | ||
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | ||
| # RUN: -passes="si-pre-allocate-wwm-regs,greedy<vgpr>,virt-reg-rewriter" \ | ||
| # RUN: -o - %s | FileCheck %s --check-prefix=ALIAS | ||
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | ||
| # RUN: -passes="greedy<vgpr>,virt-reg-rewriter" \ | ||
| # RUN: -o - %s | FileCheck %s --check-prefix=NO-WWM |
There was a problem hiding this comment.
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | |
| # RUN: -passes="require<machine-register-class-info>,si-pre-allocate-wwm-regs,require<machine-register-class-info>" \ | |
| # RUN: -debug-pass-manager -o /dev/null %s 2>&1 | FileCheck %s | |
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | |
| # RUN: -passes=si-pre-allocate-wwm-regs -o - %s | FileCheck %s --check-prefix=MIR | |
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | |
| # RUN: -passes="si-pre-allocate-wwm-regs,greedy<vgpr>,virt-reg-rewriter" \ | |
| # RUN: -o - %s | FileCheck %s --check-prefix=ALIAS | |
| # RUN: llc -mtriple=amdgcn-amd-amdhsa -mcpu=gfx90a \ | |
| # RUN: -passes="greedy<vgpr>,virt-reg-rewriter" \ | |
| # RUN: -o - %s | FileCheck %s --check-prefix=NO-WWM | |
| # RUN: llc -mtriple=amdgpu9.0a-amd-amdhsa \ | |
| # RUN: -passes="require<machine-register-class-info>,si-pre-allocate-wwm-regs,require<machine-register-class-info>" \ | |
| # RUN: -debug-pass-manager -filetype=nulll %s 2>&1 | FileCheck %s | |
| # RUN: llc -mtriple=amdgpu9.0a-amd-amdhsa \ | |
| # RUN: -passes=si-pre-allocate-wwm-regs -o - %s | FileCheck %s --check-prefix=MIR | |
| # RUN: llc -mtriple=amdgpu9.0a-amd-amdhsa \ | |
| # RUN: -passes="si-pre-allocate-wwm-regs,greedy<vgpr>,virt-reg-rewriter" \ | |
| # RUN: -o - %s | FileCheck %s --check-prefix=ALIAS | |
| # RUN: llc -mtriple=amdgpu9.0a-amd-amdhsa \ | |
| # RUN: -passes="greedy<vgpr>,virt-reg-rewriter" \ | |
| # RUN: -o - %s | FileCheck %s --check-prefix=NO-WWM |
There was a problem hiding this comment.
thanks, changed locally
| // ProperSubClass depends on both this class and its superclass counts, so | ||
| // calculate it only after all valid orders have been compacted. |
There was a problem hiding this comment.
Isn't this implied by the register class order?
There was a problem hiding this comment.
True, merging into one loop
| # RUN: -passes="si-pre-allocate-wwm-regs,greedy<vgpr>,virt-reg-rewriter" \ | ||
| # RUN: -o - %s | FileCheck %s --check-prefix=ALIAS | ||
| # RUN: llc -mtriple=amdgpu9.0a-amd-amdhsa \ | ||
| # RUN: -passes="greedy<vgpr>,virt-reg-rewriter" \ |
There was a problem hiding this comment.
The greedy run will freshly compute a new instance, not really sure what the point is.
I think to comprehensively assert that recompute == incremental probably requires a unit test
6d5057c to
0c9e7be
Compare
Based on this TODO comment in SIPreAllocateWWMRegs.cpp:
Creates function updateReservedRegs in RegisterClassInfo.cpp which takes the BitVector containing reserved register information without requiring any unnecessary recomputations of the entire RegisterClassInfo object.
It:
SIPreAllocateWWMRegsto preserveRegisterClassInfoafter reserving WWM registers.Also drops the unconditional compute(RC) in computePSetLimit, which was redundant since getNumAllocatableRegs already recomputes stale entries. This is within the scope of my PR because it would unnecessarily rebuild an already-valid entry, rendering the incremental update pointless.