[AMDGPU] Fix GCNUpwardRPTracker. (WIP) #71186
Merged
Conversation
This file contains 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
|
@llvm/pr-subscribers-backend-amdgpu Author: Valery Pykhtin (vpykhtin) ChangesThis is an early-preview, this patch is going to fix:
Interface has changed, moveMaxPressure becomes deprecated and getMaxPressure, resetMaxPressure functions are added. reset function seem now more consistent. GCNUpwardRPTracker interface may seem a bit strange but I would like to clarify what I had in mind when created it. The idea was to have a tracker that can work on non-implemented tentative schedules, that is:
For this reason it has no functions to advance to next instruction, it should not use dead/kill flags. It only uses LiveIntervals information to track usage of a register with subregs and to calculate starting live register set. The idea is that lane bitmask of a reg's use should be the same for any schedule. Full diff: https://github.com/llvm/llvm-project/pull/71186.diff 4 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/GCNIterativeScheduler.cpp b/llvm/lib/Target/AMDGPU/GCNIterativeScheduler.cpp
index d89c9b1febded0f..cdc9de7f65e3e50 100644
--- a/llvm/lib/Target/AMDGPU/GCNIterativeScheduler.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNIterativeScheduler.cpp
@@ -251,7 +251,7 @@ GCNIterativeScheduler::getRegionPressure(MachineBasicBlock::iterator Begin,
assert(UPTracker.isValid() ||
(dbgs() << "Tracked region ",
printRegion(dbgs(), Begin, End, LIS), false));
- return UPTracker.moveMaxPressure();
+ return UPTracker.getMaxPressureAndReset();
}
// returns max pressure for a tentative schedule
@@ -272,7 +272,7 @@ GCNIterativeScheduler::getSchedulePressure(const Region &R,
for (auto I = Schedule.end(), B = Schedule.begin(); I != B;) {
RPTracker.recede(*getMachineInstr(*--I));
}
- return RPTracker.moveMaxPressure();
+ return RPTracker.getMaxPressureAndReset();
}
void GCNIterativeScheduler::enterRegion(MachineBasicBlock *BB, // overridden
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
index a04c470b7b9762f..f191f3f08c56c6d 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.cpp
@@ -166,66 +166,62 @@ static LaneBitmask getDefRegMask(const MachineOperand &MO,
MRI.getTargetRegisterInfo()->getSubRegIndexLaneMask(MO.getSubReg());
}
-static LaneBitmask getUsedRegMask(const MachineOperand &MO,
- const MachineRegisterInfo &MRI,
- const LiveIntervals &LIS) {
- assert(MO.isUse() && MO.isReg() && MO.getReg().isVirtual());
-
- if (auto SubReg = MO.getSubReg())
- return MRI.getTargetRegisterInfo()->getSubRegIndexLaneMask(SubReg);
-
- auto MaxMask = MRI.getMaxLaneMaskForVReg(MO.getReg());
- if (SIRegisterInfo::getNumCoveredRegs(MaxMask) > 1) // cannot have subregs
- return MaxMask;
-
- // For a tentative schedule LIS isn't updated yet but livemask should remain
- // the same on any schedule. Subreg defs can be reordered but they all must
- // dominate uses anyway.
- auto SI = LIS.getInstructionIndex(*MO.getParent()).getBaseIndex();
- return getLiveLaneMask(MO.getReg(), SI, LIS, MRI);
-}
-
-static SmallVector<RegisterMaskPair, 8>
-collectVirtualRegUses(const MachineInstr &MI, const LiveIntervals &LIS,
+static void
+collectVirtualRegUses(SmallVectorImpl<RegisterMaskPair> &RegMaskPairs,
+ const MachineInstr &MI, const LiveIntervals &LIS,
const MachineRegisterInfo &MRI) {
- SmallVector<RegisterMaskPair, 8> Res;
+ SlotIndex InstrSI;
for (const auto &MO : MI.operands()) {
if (!MO.isReg() || !MO.getReg().isVirtual())
continue;
if (!MO.isUse() || !MO.readsReg())
continue;
- auto const UsedMask = getUsedRegMask(MO, MRI, LIS);
+ Register Reg = MO.getReg();
+ auto I = llvm::find_if(RegMaskPairs, [Reg](const RegisterMaskPair &RM) {
+ return RM.RegUnit == Reg;
+ });
+ if (I != RegMaskPairs.end())
+ continue;
+
+ LaneBitmask UseMask;
+ auto &LI = LIS.getInterval(Reg);
+ if (!LI.hasSubRanges())
+ UseMask = MRI.getMaxLaneMaskForVReg(Reg);
+ else {
+ // For a tentative schedule LIS isn't updated yet but livemask should
+ // remain the same on any schedule. Subreg defs can be reordered but they
+ // all must dominate uses anyway.
+ if (!InstrSI)
+ InstrSI = LIS.getInstructionIndex(*MO.getParent()).getBaseIndex();
+ UseMask = getLiveLaneMask(LI, InstrSI, MRI);
+ }
- auto Reg = MO.getReg();
- auto I = llvm::find_if(
- Res, [Reg](const RegisterMaskPair &RM) { return RM.RegUnit == Reg; });
- if (I != Res.end())
- I->LaneMask |= UsedMask;
- else
- Res.push_back(RegisterMaskPair(Reg, UsedMask));
+ RegMaskPairs.emplace_back(Reg, UseMask);
}
- return Res;
}
///////////////////////////////////////////////////////////////////////////////
// GCNRPTracker
-LaneBitmask llvm::getLiveLaneMask(unsigned Reg,
- SlotIndex SI,
+LaneBitmask llvm::getLiveLaneMask(unsigned Reg, SlotIndex SI,
const LiveIntervals &LIS,
const MachineRegisterInfo &MRI) {
+ return getLiveLaneMask(LIS.getInterval(Reg), SI, MRI);
+}
+
+LaneBitmask llvm::getLiveLaneMask(const LiveInterval &LI, SlotIndex SI,
+ const MachineRegisterInfo &MRI) {
LaneBitmask LiveMask;
- const auto &LI = LIS.getInterval(Reg);
if (LI.hasSubRanges()) {
for (const auto &S : LI.subranges())
if (S.liveAt(SI)) {
LiveMask |= S.LaneMask;
- assert(LiveMask < MRI.getMaxLaneMaskForVReg(Reg) ||
- LiveMask == MRI.getMaxLaneMaskForVReg(Reg));
+ assert(LiveMask < MRI.getMaxLaneMaskForVReg(LI.reg()) ||
+ LiveMask == MRI.getMaxLaneMaskForVReg(LI.reg()));
}
} else if (LI.liveAt(SI)) {
- LiveMask = MRI.getMaxLaneMaskForVReg(Reg);
+ LiveMask = MRI.getMaxLaneMaskForVReg(LI.reg());
}
return LiveMask;
}
@@ -261,15 +257,11 @@ void GCNRPTracker::reset(const MachineInstr &MI,
MaxPressure = CurPressure = getRegPressure(*MRI, LiveRegs);
}
-void GCNUpwardRPTracker::reset(const MachineInstr &MI,
- const LiveRegSet *LiveRegsCopy) {
- GCNRPTracker::reset(MI, LiveRegsCopy, true);
-}
-
void GCNUpwardRPTracker::reset(const MachineRegisterInfo &MRI_,
const LiveRegSet &LiveRegs_) {
MRI = &MRI_;
LiveRegs = LiveRegs_;
+ LastTrackedMI = nullptr; // TODO: LastTrackedMI isnt' used, remove?
MaxPressure = CurPressure = getRegPressure(MRI_, LiveRegs_);
}
@@ -281,38 +273,49 @@ void GCNUpwardRPTracker::recede(const MachineInstr &MI) {
if (MI.isDebugInstr())
return;
- auto const RegUses = collectVirtualRegUses(MI, LIS, *MRI);
-
- // calc pressure at the MI (defs + uses)
- auto AtMIPressure = CurPressure;
- for (const auto &U : RegUses) {
- auto LiveMask = LiveRegs[U.RegUnit];
- AtMIPressure.inc(U.RegUnit, LiveMask, LiveMask | U.LaneMask, *MRI);
- }
- // update max pressure
- MaxPressure = max(AtMIPressure, MaxPressure);
-
- for (const auto &MO : MI.all_defs()) {
- if (!MO.getReg().isVirtual() || MO.isDead())
- continue;
-
- auto Reg = MO.getReg();
+ auto DecrementDef = [this](const MachineOperand &MO) {
+ Register Reg = MO.getReg();
auto I = LiveRegs.find(Reg);
if (I == LiveRegs.end())
- continue;
- auto &LiveMask = I->second;
- auto PrevMask = LiveMask;
+ return;
+
+ LaneBitmask &LiveMask = I->second;
+ LaneBitmask PrevMask = LiveMask;
LiveMask &= ~getDefRegMask(MO, *MRI);
CurPressure.inc(Reg, PrevMask, LiveMask, *MRI);
if (LiveMask.none())
LiveRegs.erase(I);
+ };
+
+ // Decrement non-early-clobber defs.
+ SmallVector<const MachineOperand *, 2> EarlyClobberDefs;
+ for (const MachineOperand &MO : MI.all_defs()) {
+ if (!MO.getReg().isVirtual())
+ continue;
+ if (!MO.isEarlyClobber())
+ DecrementDef(MO);
+ else
+ EarlyClobberDefs.push_back(&MO);
}
- for (const auto &U : RegUses) {
- auto &LiveMask = LiveRegs[U.RegUnit];
- auto PrevMask = LiveMask;
+
+ // Increment uses.
+ SmallVector<RegisterMaskPair, 8> RegUses;
+ collectVirtualRegUses(RegUses, MI, LIS, *MRI);
+ for (const RegisterMaskPair &U : RegUses) {
+ LaneBitmask &LiveMask = LiveRegs[U.RegUnit];
+ LaneBitmask PrevMask = LiveMask;
LiveMask |= U.LaneMask;
CurPressure.inc(U.RegUnit, PrevMask, LiveMask, *MRI);
}
+
+ // Point of maximum pressure: non-early-clobber defs are decremented and uses
+ // are incremented.
+ MaxPressure = max(CurPressure, MaxPressure);
+
+ // Now decrement early clobber defs.
+ for (const MachineOperand *MO : EarlyClobberDefs)
+ DecrementDef(*MO);
+
assert(CurPressure == getRegPressure(*MRI, LiveRegs));
}
@@ -562,15 +565,15 @@ bool GCNRegPressurePrinter::runOnMachineFunction(MachineFunction &MF) {
} else {
GCNUpwardRPTracker RPT(LIS);
RPT.reset(MRI, MBBEndSlot);
- RPT.moveMaxPressure(); // Clear max pressure.
LiveOut = RPT.getLiveRegs();
RPAtMBBEnd = RPT.getPressure();
for (auto &MI : reverse(MBB)) {
+ RPT.resetMaxPressure();
RPT.recede(MI);
if (!MI.isDebugInstr())
- RP.emplace_back(RPT.getPressure(), RPT.moveMaxPressure());
+ RP.emplace_back(RPT.getPressure(), RPT.getMaxPressure());
}
LiveIn = RPT.getLiveRegs();
diff --git a/llvm/lib/Target/AMDGPU/GCNRegPressure.h b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
index c750fe74749e2b3..732ed33337d24dc 100644
--- a/llvm/lib/Target/AMDGPU/GCNRegPressure.h
+++ b/llvm/lib/Target/AMDGPU/GCNRegPressure.h
@@ -128,7 +128,7 @@ class GCNRPTracker {
void clearMaxPressure() { MaxPressure.clear(); }
- GCNRegPressure getPressure() const { return CurPressure; }
+ const GCNRegPressure &getPressure() const { return CurPressure; }
// returns MaxPressure, resetting it
decltype(MaxPressure) moveMaxPressure() {
@@ -149,24 +149,46 @@ class GCNUpwardRPTracker : public GCNRPTracker {
public:
GCNUpwardRPTracker(const LiveIntervals &LIS_) : GCNRPTracker(LIS_) {}
- // reset tracker to the point just below MI
- // filling live regs upon this point using LIS
- void reset(const MachineInstr &MI, const LiveRegSet *LiveRegs = nullptr);
-
// reset tracker and set live register set to the specified value.
void reset(const MachineRegisterInfo &MRI_, const LiveRegSet &LiveRegs_);
// reset tracker at the specified slot index.
- void reset(const MachineRegisterInfo &MRI_, SlotIndex SI) {
- reset(MRI_, llvm::getLiveRegs(SI, LIS, MRI_));
+ void reset(const MachineRegisterInfo &MRI, SlotIndex SI) {
+ reset(MRI, llvm::getLiveRegs(SI, LIS, MRI));
+ }
+
+ // reset tracker to the end of the MBB.
+ void reset(const MachineBasicBlock &MBB) {
+ reset(MBB.getParent()->getRegInfo(),
+ LIS.getSlotIndexes()->getMBBEndIdx(&MBB));
+ }
+
+ // reset tracker to the point just after MI (in program order).
+ void reset(const MachineInstr &MI) {
+ reset(MI.getMF()->getRegInfo(), LIS.getInstructionIndex(MI).getDeadSlot());
}
- // move to the state just above the MI
+ // move to the state just before the MI (in program order).
void recede(const MachineInstr &MI);
// checks whether the tracker's state after receding MI corresponds
- // to reported by LIS
+ // to reported by LIS.
bool isValid() const;
+
+ // deprecated.
+ decltype(MaxPressure) moveMaxPressure() = delete;
+
+ const GCNRegPressure &getMaxPressure() const { return MaxPressure; }
+
+ void resetMaxPressure() {
+ MaxPressure = CurPressure;
+ }
+
+ GCNRegPressure getMaxPressureAndReset() {
+ GCNRegPressure RP = MaxPressure;
+ resetMaxPressure();
+ return RP;
+ }
};
class GCNDownwardRPTracker : public GCNRPTracker {
@@ -209,6 +231,13 @@ LaneBitmask getLiveLaneMask(unsigned Reg,
const LiveIntervals &LIS,
const MachineRegisterInfo &MRI);
+LaneBitmask getLiveLaneMask(const LiveInterval &LI, SlotIndex SI,
+ const MachineRegisterInfo &MRI);
+
+GCNRPTracker::LiveRegSet getLiveRegs(SlotIndex SI,
+ const LiveIntervals &LIS,
+ const MachineRegisterInfo &MRI);
+
/// creates a map MachineInstr -> LiveRegSet
/// R - range of iterators on instructions
/// After - upon entry or exit of every instruction
diff --git a/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir b/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
index d53050167e98bef..bb889e48aa16895 100644
--- a/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
+++ b/llvm/test/CodeGen/AMDGPU/regpressure_printer.mir
@@ -136,54 +136,28 @@ body: |
name: upward_problem_lis_subregs_mismatch
tracksRegLiveness: true
body: |
- ; RPU-LABEL: name: upward_problem_lis_subregs_mismatch
- ; RPU: bb.0:
- ; RPU-NEXT: Live-in:
- ; RPU-NEXT: SGPR VGPR
- ; RPU-NEXT: 0 0
- ; RPU-NEXT: 0 1 undef %0.sub0:vreg_64 = V_MOV_B32_e32 42, implicit $exec
- ; RPU-NEXT: 0 1
- ; RPU-NEXT: 0 2 undef %1.sub1:vreg_64 = V_MOV_B32_e32 33, implicit $exec
- ; RPU-NEXT: 0 2
- ; RPU-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
- ; RPU-NEXT: bb.1:
- ; RPU-NEXT: Live-in: %0:0000000000000003 %1:000000000000000C
- ; RPU-NEXT: SGPR VGPR
- ; RPU-NEXT: 0 2
- ; RPU-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
- ; RPU-NEXT: bb.2:
- ; RPU-NEXT: Live-in: %0:000000000000000F %1:000000000000000F
- ; RPU-NEXT: mis LIS: %0:0000000000000003 %1:000000000000000C
- ; RPU-NEXT: %0 masks doesn't match: LIS reported 0000000000000003, tracked 000000000000000F
- ; RPU-NEXT: %1 masks doesn't match: LIS reported 000000000000000C, tracked 000000000000000F
- ; RPU-NEXT: SGPR VGPR
- ; RPU-NEXT: 0 4
- ; RPU-NEXT: 0 4 S_NOP 0, implicit %0:vreg_64, implicit %1:vreg_64
- ; RPU-NEXT: 0 0
- ; RPU-NEXT: Live-out:
- ;
- ; RPD-LABEL: name: upward_problem_lis_subregs_mismatch
- ; RPD: bb.0:
- ; RPD-NEXT: Live-in:
- ; RPD-NEXT: SGPR VGPR
- ; RPD-NEXT: 0 0
- ; RPD-NEXT: 0 1 undef %0.sub0:vreg_64 = V_MOV_B32_e32 42, implicit $exec
- ; RPD-NEXT: 0 1
- ; RPD-NEXT: 0 2 undef %1.sub1:vreg_64 = V_MOV_B32_e32 33, implicit $exec
- ; RPD-NEXT: 0 2
- ; RPD-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
- ; RPD-NEXT: bb.1:
- ; RPD-NEXT: Live-in: %0:0000000000000003 %1:000000000000000C
- ; RPD-NEXT: SGPR VGPR
- ; RPD-NEXT: 0 2
- ; RPD-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
- ; RPD-NEXT: bb.2:
- ; RPD-NEXT: Live-in: %0:0000000000000003 %1:000000000000000C
- ; RPD-NEXT: SGPR VGPR
- ; RPD-NEXT: 0 2
- ; RPD-NEXT: 0 2 S_NOP 0, implicit %0:vreg_64, implicit %1:vreg_64
- ; RPD-NEXT: 0 0
- ; RPD-NEXT: Live-out:
+ ; RP-LABEL: name: upward_problem_lis_subregs_mismatch
+ ; RP: bb.0:
+ ; RP-NEXT: Live-in:
+ ; RP-NEXT: SGPR VGPR
+ ; RP-NEXT: 0 0
+ ; RP-NEXT: 0 1 undef %0.sub0:vreg_64 = V_MOV_B32_e32 42, implicit $exec
+ ; RP-NEXT: 0 1
+ ; RP-NEXT: 0 2 undef %1.sub1:vreg_64 = V_MOV_B32_e32 33, implicit $exec
+ ; RP-NEXT: 0 2
+ ; RP-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
+ ; RP-NEXT: bb.1:
+ ; RP-NEXT: Live-in: %0:0000000000000003 %1:000000000000000C
+ ; RP-NEXT: SGPR VGPR
+ ; RP-NEXT: 0 2
+ ; RP-NEXT: Live-out: %0:0000000000000003 %1:000000000000000C
+ ; RP-NEXT: bb.2:
+ ; RP-NEXT: Live-in: %0:0000000000000003 %1:000000000000000C
+ ; RP-NEXT: SGPR VGPR
+ ; RP-NEXT: 0 2
+ ; RP-NEXT: 0 2 S_NOP 0, implicit %0:vreg_64, implicit %1:vreg_64
+ ; RP-NEXT: 0 0
+ ; RP-NEXT: Live-out:
bb.0:
undef %0.sub0:vreg_64 = V_MOV_B32_e32 42, implicit $exec
undef %1.sub1:vreg_64 = V_MOV_B32_e32 33, implicit $exec
@@ -217,13 +191,13 @@ body: |
; RPU-NEXT: 0 7
; RPU-NEXT: 0 8 %4.sub0:vreg_64 = V_MOV_B32_e32 111, implicit $exec
; RPU-NEXT: 0 8
- ; RPU-NEXT: 0 10 %5:vreg_64 = COPY %2:vreg_64
+ ; RPU-NEXT: 0 9 %5:vreg_64 = COPY %2:vreg_64
; RPU-NEXT: 0 9
; RPU-NEXT: 0 9 undef %6.sub0:vreg_64 = V_ADD_F32_e32 %1.sub0:vreg_64, %5.sub0:vreg_64, implicit $mode, implicit $exec
; RPU-NEXT: 0 8
; RPU-NEXT: 0 8 dead %6.sub1:vreg_64 = V_ADD_F32_e32 %1.sub1:vreg_64, %5.sub0:vreg_64, implicit $mode, implicit $exec
; RPU-NEXT: 0 7
- ; RPU-NEXT: 0 8 %7:vgpr_32 = GLOBAL_LOAD_DWORD %5:vreg_64, 0, 0, implicit $exec
+ ; RPU-NEXT: 0 7 %7:vgpr_32 = GLOBAL_LOAD_DWORD %5:vreg_64, 0, 0, implicit $exec
; RPU-NEXT: 0 6
; RPU-NEXT: 0 7 %8:vreg_64 = IMPLICIT_DEF
; RPU-NEXT: 0 7
@@ -247,16 +221,16 @@ body: |
; RPU-NEXT: 0 20
; RPU-NEXT: 0 21 %18:vgpr_32 = V_MOV_B32_e32 0, implicit $exec
; RPU-NEXT: 0 21
- ; RPU-NEXT: 0 22 undef %19.sub0:vreg_64 = V_ADD_F32_e32 %7:vgpr_32, %2.sub0:vreg_64, implicit $mode, implicit $exec
+ ; RPU-NEXT: 0 21 undef %19.sub0:vreg_64 = V_ADD_F32_e32 %7:vgpr_32, %2.sub0:vreg_64, implicit $mode, implicit $exec
; RPU-NEXT: 0 20
- ; RPU-NEXT: 0 21 %19.sub1:vreg_64 = V_ADD_F32_e32 %3:vgpr_32, %3:vgpr_32, implicit $mode, implicit $exec
+ ; RPU-NEXT: 0 20 %19.sub1:vreg_64 = V_ADD_F32_e32 %3:vgpr_32, %3:vgpr_32, implicit $mode, implicit $exec
; RPU-NEXT: DBG_VALUE
; RPU-NEXT: 0 20
; RPU-NEXT: 0 20 GLOBAL_STORE_DWORDX2 %19:vreg_64, %4:vreg_64, 32, 0, implicit $exec
; RPU-NEXT: 0 16
- ; RPU-NEXT: 0 17 %11.sub0:vreg_64 = GLOBAL_LOAD_DWORD %9:vreg_64, 0, 0, implicit $exec
+ ; RPU-NEXT: 0 16 %11.sub0:vreg_64 = GLOBAL_LOAD_DWORD %9:vreg_64, 0, 0, implicit $exec
; RPU-NEXT: 0 15
- ; RPU-NEXT: 0 16 %8.sub0:vreg_64 = GLOBAL_LOAD_DWORD %10:vreg_64, 0, 0, implicit $exec
+ ; RPU-NEXT: 0 15 %8.sub0:vreg_64 = GLOBAL_LOAD_DWORD %10:vreg_64, 0, 0, implicit $exec
; RPU-NEXT: 0 14
; RPU-NEXT: 0 14 dead %20:vgpr_32 = GLOBAL_LOAD_DWORD %11:vreg_64, 0, 0, implicit $exec
; RPU-NEXT: DBG_VALUE
@@ -266,7 +240,7 @@ body: |
; RPU-NEXT: 0 10
; RPU-NEXT: 0 10 dead %22:vgpr_32 = GLOBAL_LOAD_DWORD %15:vreg_64, 0, 0, implicit $exec
; RPU-NEXT: 0 10
- ; RPU-NEXT: 0 11 %23:vreg_64 = V_LSHLREV_B64_e64 2, %8:vreg_64, implicit $exec
+ ; RPU-NEXT: 0 10 %23:vreg_64 = V_LSHLREV_B64_e64 2, %8:vreg_64, implicit $exec
; RPU-NEXT: 0 9
; RPU-NEXT: 0 9 S_NOP 0, implicit %13:vgpr_32, implicit %23.sub0:vreg_64, implicit %12:vgpr_32, implicit %17:vgpr_32
; RPU-NEXT: 0 5
@@ -459,4 +433,55 @@ body: |
S_NOP 0, implicit %16
S_ENDPGM 0
...
-
+---
+name: test_early_clobber_trivial
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; RP-LABEL: name: test_early_clobber_trivial
+ ; RP: Live-in:
+ ; RP-NEXT: SGPR VGPR
+ ; RP-NEXT: 0 0
+ ; RP-NEXT: 0 1 %0:vgpr_32 = V_MOV_B32_e32 42, implicit $exec
+ ; RP-NEXT: 0 1
+ ; RP-NEXT: 0 2 early-clobber %1:vgpr_32 = V_MOV_B32_e32 %0:vgpr_32, implicit $exec
+ ; RP-NEXT: 0 1
+ ; RP-NEXT: 0 1 S_NOP 0, implicit %1:vgpr_32
+ ; RP-NEXT: 0 0
+ ; RP-NEXT: Live-out:
+ %0:vgpr_32 = V_MOV_B32_e32 42, implicit $exec
+ early-clobber %1:vgpr_32 = V_MOV_B32_e32 %0, implicit $exec
+ S_NOP 0, implicit %1
+...
+---
+name: test_not_early_clobber_trivial
+tracksRegLiveness: true
+body: |
+ bb.0:
+ ; RPU-LABEL: name: test_not_early_clobber_trivial
+ ; RPU: Live-in:
+ ; RPU-NEXT: SGPR VGPR
+ ; RPU-NEXT: 0 0
+ ; RPU-NEXT: 0 1 %0:vgpr_32 = V_MOV_B32_e32 42, implicit $exec
+ ; RPU-NEXT: 0 1
+ ; RPU-NEXT: 0 1 %1:vgpr_32 = V_MOV_B32_e32 %0:vgpr_32, implicit $exec
+ ; RPU-NEXT: 0 1
+ ; RPU-NEXT: 0 1 S_NOP 0, implicit %1:vgpr_32
+ ; RPU-NEXT: 0 0
+ ; RPU-NEXT: Live-out:
+ ;
+ ; RPD-LABEL: name: test_not_early_clobber_trivial
+ ; RPD: Live-in:
+ ; RPD-NEXT: SGPR VGPR
+ ; RPD-NEXT: 0 0
+ ; RPD-NEXT: 0 1 %0:vgpr_32 = V_MOV_B32_e32 42, implicit $exec
+ ; RPD-NEXT: 0 1
+ ; RPD-NEXT: 0 2 %1:vgpr_32 = V_MOV_B32_e32 %0:vgpr_32, implicit $exec
+ ; RPD-NEXT: 0 1
+ ; RPD-NEXT: 0 1 S_NOP 0, implicit %1:vgpr_32
+ ; RPD-NEXT: 0 0
+ ; RPD-NEXT: Live-out:
+ %0:vgpr_32 = V_MOV_B32_e32 42, implicit $exec
+ %1:vgpr_32 = V_MOV_B32_e32 %0, implicit $exec
+ S_NOP 0, implicit %1
+...
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Thanks. That is useful information. I can understand how the upward tracker can work like that, but how does the downward tracker work? When it sees a use operand, it does not know if it is the last use of that register. |
Downward tracker is different, it wasn't supposed to be used like that, and therefore it has different interface. |
|
Thanks - I can confirm the patch fixes two issues I observed recently, where the pressure was over-reported. |
arsenm
reviewed
Nov 7, 2023
jayfoad
reviewed
Nov 7, 2023
| void GCNUpwardRPTracker::reset(const MachineRegisterInfo &MRI_, | ||
| const LiveRegSet &LiveRegs_) { | ||
| MRI = &MRI_; | ||
| LiveRegs = LiveRegs_; | ||
| LastTrackedMI = nullptr; // TODO: LastTrackedMI isnt' used, remove? |
There was a problem hiding this comment.
I haven't noticed it's used with the base class accessor, so I decided to leave it and delete TODO comment.
|
I assume this can be submitted if there are no objections, and I'm going to fix downward tracker next. |
jayfoad
approved these changes
Nov 9, 2023
Thank you Jay, let's review those details, I find both points above valid and will fix them. |
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
Fixed: 1. Maximum register pressure calculation at the instruction level. Previously max RP included both def and use of registers of an instruction. Now maximum RP includes _uses_ and _early-clobber defs_. 2. Uses were incorrectly tracked and this resulted in a mismatch of live-in set reported by LiveIntervals and tracked live reg set when the beginning of the block is reached. Interface has changed, moveMaxPressure becomes deprecated and getMaxPressure, resetMaxPressure functions are added. reset function seem now more consistent.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This is an early-preview, this patch is going to fix:
Maximum register pressure calculation at the instruction level.
Previously max RP included both def and use of registers of an instruction. Now maximum RP includes uses and early-clobber defs.
Uses were incorrectly tracked and this resulted in a mismatch of live-in set reported by LiveIntervals and tracked live reg set when the beginning of the block is reached.
Interface has changed, moveMaxPressure becomes deprecated and getMaxPressure, resetMaxPressure functions are added. reset function seem now more consistent.
GCNUpwardRPTracker interface may seem a bit strange but I would like to clarify what I had in mind when created it.
The idea was to have a tracker that can work on non-implemented tentative schedules, that is:
For this reason it has no functions to advance to next instruction, it should not use dead/kill flags. It only uses LiveIntervals information to track usage of a register with subregs and to calculate starting live register set. The idea is that lane bitmask of a reg's use should be the same for any schedule.