(reland) [AMDGPU][SIInsertWaitCnts] Use RegUnits-based tracking (#162077) #171779
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
) Fixed a crash in Blender due to some weird control flow. The issue was with the "merge" function which was only looking at the keys of the "Other" VMem/SGPR maps. It needs to look at the keys of both maps and merge them. Original commit message below ---- The pass was already "reinventing" the concept just to deal with 16 bit registers. Clean up the entire tracking logic to only use register units. There are no test changes because functionality didn't change, except: - We can now track more LDS DMA IDs if we need it (up to `1 << 16`) - The debug prints also changed a bit because we now talk in terms of register units. This also changes the tracking to use a DenseMap instead of a massive fixed size table. This trades a bit of access speed for a smaller memory footprint. Allocating and memsetting a huge table to zero caused a non-negligible performance impact (I've observed up to 50% of the time in the pass spent in the `memcpy` built-in on a big test file). I also think we don't access these often enough to really justify using a vector. We do a few accesses per instruction, but not much more. In a huge 120MB LL file, I can barely see the trace of the DenseMap accesses.
Contributor
Author
This stack of pull requests is managed by Graphite. Learn more about stacking. |
Member
|
@llvm/pr-subscribers-backend-amdgpu Author: Pierre van Houtryve (Pierre-vh) ChangesFixed a crash in Blender due to some weird control flow. Original commit message belowThe pass was already "reinventing" the concept just to deal with 16 bit There are no test changes because functionality didn't change, except:
This also changes the tracking to use a DenseMap instead of a massive I also think we don't access these often enough to really justify using Patch is 42.92 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/171779.diff 2 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
index 3d6fc309c7cf4..7c0525b9c9957 100644
--- a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
+++ b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp
@@ -97,7 +97,40 @@ auto inst_counter_types(InstCounterType MaxCounter = NUM_INST_CNTS) {
return enum_seq(LOAD_CNT, MaxCounter);
}
-using RegInterval = std::pair<int, int>;
+/// Integer IDs used to track vector memory locations we may have to wait on.
+/// Encoded as u16 chunks:
+///
+/// [0, REGUNITS_END ): MCRegUnit
+/// [LDSDMA_BEGIN, LDSDMA_END ) : LDS DMA IDs
+///
+/// NOTE: The choice of encoding these as "u16 chunks" is arbitrary.
+/// It gives (2 << 16) - 1 entries per category which is more than enough
+/// for all register units. MCPhysReg is u16 so we don't even support >u16
+/// physical register numbers at this time, let alone >u16 register units.
+/// In any case, an assertion in "WaitcntBrackets" ensures REGUNITS_END
+/// is enough for all register units.
+using VMEMID = uint32_t;
+
+enum : VMEMID {
+ TRACKINGID_RANGE_LEN = (1 << 16),
+
+ // Important: MCRegUnits must always be tracked starting from 0, as we
+ // need to be able to convert between a MCRegUnit and a VMEMID freely.
+ REGUNITS_BEGIN = 0,
+ REGUNITS_END = REGUNITS_BEGIN + TRACKINGID_RANGE_LEN,
+
+ // Note for LDSDMA: LDSDMA_BEGIN corresponds to the "common"
+ // entry, which is updated for all LDS DMA operations encountered.
+ // Specific LDS DMA IDs start at LDSDMA_BEGIN + 1.
+ NUM_LDSDMA = TRACKINGID_RANGE_LEN,
+ LDSDMA_BEGIN = REGUNITS_END,
+ LDSDMA_END = LDSDMA_BEGIN + NUM_LDSDMA,
+};
+
+/// Convert a MCRegUnit to a VMEMID.
+static constexpr VMEMID toVMEMID(MCRegUnit RU) {
+ return static_cast<unsigned>(RU);
+}
struct HardwareLimits {
unsigned LoadcntMax; // Corresponds to VMcnt prior to gfx12.
@@ -146,30 +179,6 @@ static constexpr StringLiteral WaitEventTypeName[] = {
#undef AMDGPU_EVENT_NAME
// clang-format on
-// The mapping is:
-// 0 .. SQ_MAX_PGM_VGPRS-1 real VGPRs
-// SQ_MAX_PGM_VGPRS .. NUM_ALL_VGPRS-1 extra VGPR-like slots
-// NUM_ALL_VGPRS .. NUM_ALL_VGPRS+SQ_MAX_PGM_SGPRS-1 real SGPRs
-// NUM_ALL_VGPRS+SQ_MAX_PGM_SGPRS .. SCC
-// We reserve a fixed number of VGPR slots in the scoring tables for
-// special tokens like SCMEM_LDS (needed for buffer load to LDS).
-enum RegisterMapping {
- SQ_MAX_PGM_VGPRS = 2048, // Maximum programmable VGPRs across all targets.
- AGPR_OFFSET = 512, // Maximum programmable ArchVGPRs across all targets.
- SQ_MAX_PGM_SGPRS = 128, // Maximum programmable SGPRs across all targets.
- // Artificial register slots to track LDS writes into specific LDS locations
- // if a location is known. When slots are exhausted or location is
- // unknown use the first slot. The first slot is also always updated in
- // addition to known location's slot to properly generate waits if dependent
- // instruction's location is unknown.
- FIRST_LDS_VGPR = SQ_MAX_PGM_VGPRS, // Extra slots for LDS stores.
- NUM_LDS_VGPRS = 9, // One more than the stores we track.
- NUM_ALL_VGPRS = SQ_MAX_PGM_VGPRS + NUM_LDS_VGPRS, // Where SGPRs start.
- NUM_ALL_ALLOCATABLE = NUM_ALL_VGPRS + SQ_MAX_PGM_SGPRS,
- // Remaining non-allocatable registers
- SCC = NUM_ALL_ALLOCATABLE
-};
-
// Enumerate different types of result-returning VMEM operations. Although
// s_waitcnt orders them all with a single vmcnt counter, in the absence of
// s_waitcnt only instructions of the same VmemType are guaranteed to write
@@ -585,7 +594,30 @@ class SIInsertWaitcnts {
// "s_waitcnt 0" before use.
class WaitcntBrackets {
public:
- WaitcntBrackets(const SIInsertWaitcnts *Context) : Context(Context) {}
+ WaitcntBrackets(const SIInsertWaitcnts *Context) : Context(Context) {
+ assert(Context->TRI->getNumRegUnits() < REGUNITS_END);
+ }
+
+#ifndef NDEBUG
+ ~WaitcntBrackets() {
+ unsigned NumUnusedVmem = 0, NumUnusedSGPRs = 0;
+ for (auto &[ID, Val] : VMem) {
+ if (Val.empty())
+ ++NumUnusedVmem;
+ }
+ for (auto &[ID, Val] : SGPRs) {
+ if (Val.empty())
+ ++NumUnusedSGPRs;
+ }
+
+ if (NumUnusedVmem || NumUnusedSGPRs) {
+ errs() << "WaitcntBracket had unused entries at destruction time: "
+ << NumUnusedVmem << " VMem and " << NumUnusedSGPRs
+ << " SGPR unused entries\n";
+ std::abort();
+ }
+ }
+#endif
bool isSmemCounter(InstCounterType T) const {
return T == Context->SmemAccessCounter || T == X_CNT;
@@ -610,22 +642,18 @@ class WaitcntBrackets {
return getScoreUB(T) - getScoreLB(T);
}
- unsigned getRegScore(int GprNo, InstCounterType T) const {
- if (GprNo < NUM_ALL_VGPRS)
- return VgprScores[T][GprNo];
-
- if (GprNo < NUM_ALL_ALLOCATABLE)
- return SgprScores[getSgprScoresIdx(T)][GprNo - NUM_ALL_VGPRS];
+ unsigned getSGPRScore(MCRegUnit RU, InstCounterType T) const {
+ auto It = SGPRs.find(RU);
+ return It != SGPRs.end() ? It->second.Scores[getSgprScoresIdx(T)] : 0;
+ }
- assert(GprNo == SCC);
- return SCCScore;
+ unsigned getVMemScore(VMEMID TID, InstCounterType T) const {
+ auto It = VMem.find(TID);
+ return It != VMem.end() ? It->second.Scores[T] : 0;
}
bool merge(const WaitcntBrackets &Other);
- RegInterval getRegInterval(const MachineInstr *MI,
- const MachineOperand &Op) const;
-
bool counterOutOfOrder(InstCounterType T) const;
void simplifyWaitcnt(AMDGPU::Waitcnt &Wait);
void simplifyWaitcnt(InstCounterType T, unsigned &Count) const;
@@ -633,12 +661,10 @@ class WaitcntBrackets {
bool canOptimizeXCntWithLoadCnt(const AMDGPU::Waitcnt &Wait);
void simplifyXcnt(AMDGPU::Waitcnt &CheckWait, AMDGPU::Waitcnt &UpdateWait);
- void determineWait(InstCounterType T, RegInterval Interval,
- AMDGPU::Waitcnt &Wait) const;
- void determineWait(InstCounterType T, int RegNo,
- AMDGPU::Waitcnt &Wait) const {
- determineWait(T, {RegNo, RegNo + 1}, Wait);
- }
+ void determineWaitForPhysReg(InstCounterType T, MCPhysReg Reg,
+ AMDGPU::Waitcnt &Wait) const;
+ void determineWaitForLDSDMA(InstCounterType T, VMEMID TID,
+ AMDGPU::Waitcnt &Wait) const;
void tryClearSCCWriteEvent(MachineInstr *Inst);
void applyWaitcnt(const AMDGPU::Waitcnt &Wait);
@@ -686,19 +712,22 @@ class WaitcntBrackets {
// Return true if there might be pending writes to the vgpr-interval by VMEM
// instructions with types different from V.
- bool hasOtherPendingVmemTypes(RegInterval Interval, VmemType V) const {
- for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
- assert(RegNo < NUM_ALL_VGPRS);
- if (VgprVmemTypes[RegNo] & ~(1 << V))
+ bool hasOtherPendingVmemTypes(MCPhysReg Reg, VmemType V) const {
+ for (MCRegUnit RU : regunits(Reg)) {
+ auto It = VMem.find(toVMEMID(RU));
+ if (It != VMem.end() && (It->second.VMEMTypes & ~(1 << V)))
return true;
}
return false;
}
- void clearVgprVmemTypes(RegInterval Interval) {
- for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
- assert(RegNo < NUM_ALL_VGPRS);
- VgprVmemTypes[RegNo] = 0;
+ void clearVgprVmemTypes(MCPhysReg Reg) {
+ for (MCRegUnit RU : regunits(Reg)) {
+ if (auto It = VMem.find(toVMEMID(RU)); It != VMem.end()) {
+ It->second.VMEMTypes = 0;
+ if (It->second.empty())
+ VMem.erase(It);
+ }
}
}
@@ -714,11 +743,15 @@ class WaitcntBrackets {
bool hasPointSampleAccel(const MachineInstr &MI) const;
bool hasPointSamplePendingVmemTypes(const MachineInstr &MI,
- RegInterval Interval) const;
+ MCPhysReg RU) const;
void print(raw_ostream &) const;
void dump() const { print(dbgs()); }
+ // Free up memory by removing empty entries from the DenseMap that track event
+ // scores.
+ void purgeEmptyTrackingData();
+
private:
struct MergeInfo {
unsigned OldLB;
@@ -726,9 +759,24 @@ class WaitcntBrackets {
unsigned MyShift;
unsigned OtherShift;
};
+
+ void determineWaitForScore(InstCounterType T, unsigned Score,
+ AMDGPU::Waitcnt &Wait) const;
+
static bool mergeScore(const MergeInfo &M, unsigned &Score,
unsigned OtherScore);
+ iterator_range<MCRegUnitIterator> regunits(MCPhysReg Reg) const {
+ assert(Reg != AMDGPU::SCC && "Shouldn't be used on SCC");
+ if (!Context->TRI->isInAllocatableClass(Reg))
+ return {{}, {}};
+ const TargetRegisterClass *RC = Context->TRI->getPhysRegBaseClass(Reg);
+ unsigned Size = Context->TRI->getRegSizeInBits(*RC);
+ if (Size == 16 && Context->ST->hasD16Writes32BitVgpr())
+ Reg = Context->TRI->get32BitRegister(Reg);
+ return Context->TRI->regunits(Reg);
+ }
+
void setScoreLB(InstCounterType T, unsigned Val) {
assert(T < NUM_INST_CNTS);
ScoreLBs[T] = Val;
@@ -745,15 +793,28 @@ class WaitcntBrackets {
ScoreLBs[EXP_CNT] = ScoreUBs[EXP_CNT] - Context->getWaitCountMax(EXP_CNT);
}
- void setRegScore(int GprNo, InstCounterType T, unsigned Val) {
- setScoreByInterval({GprNo, GprNo + 1}, T, Val);
+ void setRegScore(MCPhysReg Reg, InstCounterType T, unsigned Val) {
+ const SIRegisterInfo *TRI = Context->TRI;
+ if (Reg == AMDGPU::SCC) {
+ SCCScore = Val;
+ } else if (TRI->isVectorRegister(*Context->MRI, Reg)) {
+ for (MCRegUnit RU : regunits(Reg))
+ VMem[toVMEMID(RU)].Scores[T] = Val;
+ } else if (TRI->isSGPRReg(*Context->MRI, Reg)) {
+ auto STy = getSgprScoresIdx(T);
+ for (MCRegUnit RU : regunits(Reg))
+ SGPRs[RU].Scores[STy] = Val;
+ } else {
+ llvm_unreachable("Register cannot be tracked/unknown register!");
+ }
}
- void setScoreByInterval(RegInterval Interval, InstCounterType CntTy,
- unsigned Score);
+ void setVMemScore(VMEMID TID, InstCounterType T, unsigned Val) {
+ VMem[TID].Scores[T] = Val;
+ }
- void setScoreByOperand(const MachineInstr *MI, const MachineOperand &Op,
- InstCounterType CntTy, unsigned Val);
+ void setScoreByOperand(const MachineOperand &Op, InstCounterType CntTy,
+ unsigned Val);
const SIInsertWaitcnts *Context;
@@ -764,26 +825,52 @@ class WaitcntBrackets {
unsigned LastFlat[NUM_INST_CNTS] = {0};
// Remember the last GDS operation.
unsigned LastGDS = 0;
- // wait_cnt scores for every vgpr.
- // Keep track of the VgprUB and SgprUB to make merge at join efficient.
- int VgprUB = -1;
- int SgprUB = -1;
- unsigned VgprScores[NUM_INST_CNTS][NUM_ALL_VGPRS] = {{0}};
- // Wait cnt scores for every sgpr, the DS_CNT (corresponding to LGKMcnt
- // pre-gfx12) or KM_CNT (gfx12+ only), and X_CNT (gfx1250) are relevant.
- // Row 0 represents the score for either DS_CNT or KM_CNT and row 1 keeps the
- // X_CNT score.
- unsigned SgprScores[2][SQ_MAX_PGM_SGPRS] = {{0}};
+
+ // The score tracking logic is fragmented as follows:
+ // - VMem: VGPR RegUnits and LDS DMA IDs, see the VMEMID encoding.
+ // - SGPRs: SGPR RegUnits
+ // - SCC: Non-allocatable and not general purpose: not a SGPR.
+ //
+ // For the VMem case, if the key is within the range of LDS DMA IDs,
+ // then the corresponding index into the `LDSDMAStores` vector below is:
+ // Key - LDSDMA_BEGIN - 1
+ // This is because LDSDMA_BEGIN is a generic entry and does not have an
+ // associated MachineInstr.
+ //
+ // TODO: Could we track SCC alongside SGPRs so it's not longer a special case?
+
+ struct VMEMInfo {
+ // Scores for all instruction counters.
+ std::array<unsigned, NUM_INST_CNTS> Scores = {0};
+ // Bitmask of the VmemTypes of VMEM instructions for this VGPR.
+ unsigned VMEMTypes = 0;
+
+ bool empty() const {
+ return all_of(Scores, [](unsigned K) { return K == 0; }) && !VMEMTypes;
+ }
+ };
+
+ struct SGPRInfo {
+ // Wait cnt scores for every sgpr, the DS_CNT (corresponding to LGKMcnt
+ // pre-gfx12) or KM_CNT (gfx12+ only), and X_CNT (gfx1250) are relevant.
+ // Row 0 represents the score for either DS_CNT or KM_CNT and row 1 keeps
+ // the X_CNT score.
+ std::array<unsigned, 2> Scores = {0};
+
+ bool empty() const { return !Scores[0] && !Scores[1]; }
+ };
+
+ DenseMap<VMEMID, VMEMInfo> VMem; // VGPR + LDS DMA
+ DenseMap<MCRegUnit, SGPRInfo> SGPRs;
+
// Reg score for SCC.
unsigned SCCScore = 0;
// The unique instruction that has an SCC write pending, if there is one.
const MachineInstr *PendingSCCWrite = nullptr;
- // Bitmask of the VmemTypes of VMEM instructions that might have a pending
- // write to each vgpr.
- unsigned char VgprVmemTypes[NUM_ALL_VGPRS] = {0};
+
// Store representative LDS DMA operations. The only useful info here is
// alias info. One store is kept per unique AAInfo.
- SmallVector<const MachineInstr *, NUM_LDS_VGPRS - 1> LDSDMAStores;
+ SmallVector<const MachineInstr *> LDSDMAStores;
};
class SIInsertWaitcntsLegacy : public MachineFunctionPass {
@@ -809,82 +896,9 @@ class SIInsertWaitcntsLegacy : public MachineFunctionPass {
} // end anonymous namespace
-RegInterval WaitcntBrackets::getRegInterval(const MachineInstr *MI,
- const MachineOperand &Op) const {
- if (Op.getReg() == AMDGPU::SCC)
- return {SCC, SCC + 1};
-
- const SIRegisterInfo *TRI = Context->TRI;
- const MachineRegisterInfo *MRI = Context->MRI;
-
- if (!TRI->isInAllocatableClass(Op.getReg()))
- return {-1, -1};
-
- // A use via a PW operand does not need a waitcnt.
- // A partial write is not a WAW.
- assert(!Op.getSubReg() || !Op.isUndef());
-
- RegInterval Result;
-
- MCRegister MCReg = AMDGPU::getMCReg(Op.getReg(), *Context->ST);
- unsigned RegIdx = TRI->getHWRegIndex(MCReg);
-
- const TargetRegisterClass *RC = TRI->getPhysRegBaseClass(Op.getReg());
- unsigned Size = TRI->getRegSizeInBits(*RC);
-
- // AGPRs/VGPRs are tracked every 16 bits, SGPRs by 32 bits
- if (TRI->isVectorRegister(*MRI, Op.getReg())) {
- unsigned Reg = RegIdx << 1 | (AMDGPU::isHi16Reg(MCReg, *TRI) ? 1 : 0);
- assert(!Context->ST->hasMAIInsts() || Reg < AGPR_OFFSET);
- Result.first = Reg;
- if (TRI->isAGPR(*MRI, Op.getReg()))
- Result.first += AGPR_OFFSET;
- assert(Result.first >= 0 && Result.first < SQ_MAX_PGM_VGPRS);
- assert(Size % 16 == 0);
- Result.second = Result.first + (Size / 16);
-
- if (Size == 16 && Context->ST->hasD16Writes32BitVgpr()) {
- // Regardless of which lo16/hi16 is used, consider the full 32-bit
- // register used.
- if (AMDGPU::isHi16Reg(MCReg, *TRI))
- Result.first -= 1;
- else
- Result.second += 1;
- }
- } else if (TRI->isSGPRReg(*MRI, Op.getReg()) && RegIdx < SQ_MAX_PGM_SGPRS) {
- // SGPRs including VCC, TTMPs and EXEC but excluding read-only scalar
- // sources like SRC_PRIVATE_BASE.
- Result.first = RegIdx + NUM_ALL_VGPRS;
- Result.second = Result.first + divideCeil(Size, 32);
- } else {
- return {-1, -1};
- }
-
- return Result;
-}
-
-void WaitcntBrackets::setScoreByInterval(RegInterval Interval,
- InstCounterType CntTy,
- unsigned Score) {
- for (int RegNo = Interval.first; RegNo < Interval.second; ++RegNo) {
- if (RegNo < NUM_ALL_VGPRS) {
- VgprUB = std::max(VgprUB, RegNo);
- VgprScores[CntTy][RegNo] = Score;
- } else if (RegNo < NUM_ALL_ALLOCATABLE) {
- SgprUB = std::max(SgprUB, RegNo - NUM_ALL_VGPRS);
- SgprScores[getSgprScoresIdx(CntTy)][RegNo - NUM_ALL_VGPRS] = Score;
- } else {
- assert(RegNo == SCC);
- SCCScore = Score;
- }
- }
-}
-
-void WaitcntBrackets::setScoreByOperand(const MachineInstr *MI,
- const MachineOperand &Op,
+void WaitcntBrackets::setScoreByOperand(const MachineOperand &Op,
InstCounterType CntTy, unsigned Score) {
- RegInterval Interval = getRegInterval(MI, Op);
- setScoreByInterval(Interval, CntTy, Score);
+ setRegScore(Op.getReg().asMCReg(), CntTy, Score);
}
// Return true if the subtarget is one that enables Point Sample Acceleration
@@ -907,12 +921,12 @@ bool WaitcntBrackets::hasPointSampleAccel(const MachineInstr &MI) const {
// one that has outstanding writes to vmem-types different than VMEM_NOSAMPLER
// (this is the type that a point sample accelerated instruction effectively
// becomes)
-bool WaitcntBrackets::hasPointSamplePendingVmemTypes(
- const MachineInstr &MI, RegInterval Interval) const {
+bool WaitcntBrackets::hasPointSamplePendingVmemTypes(const MachineInstr &MI,
+ MCPhysReg Reg) const {
if (!hasPointSampleAccel(MI))
return false;
- return hasOtherPendingVmemTypes(Interval, VMEM_NOSAMPLER);
+ return hasOtherPendingVmemTypes(Reg, VMEM_NOSAMPLER);
}
void WaitcntBrackets::updateByEvent(WaitEventType E, MachineInstr &Inst) {
@@ -940,57 +954,52 @@ void WaitcntBrackets::updateByEvent(WaitEventType E, MachineInstr &Inst) {
// All GDS operations must protect their address register (same as
// export.)
if (const auto *AddrOp = TII->getNamedOperand(Inst, AMDGPU::OpName::addr))
- setScoreByOperand(&Inst, *AddrOp, EXP_CNT, CurrScore);
+ setScoreByOperand(*AddrOp, EXP_CNT, CurrScore);
if (Inst.mayStore()) {
if (const auto *Data0 =
TII->getNamedOperand(Inst, AMDGPU::OpName::data0))
- setScoreByOperand(&Inst, *Data0, EXP_CNT, CurrScore);
+ setScoreByOperand(*Data0, EXP_CNT, CurrScore);
if (const auto *Data1 =
TII->getNamedOperand(Inst, AMDGPU::OpName::data1))
- setScoreByOperand(&Inst, *Data1, EXP_CNT, CurrScore);
+ setScoreByOperand(*Data1, EXP_CNT, CurrScore);
} else if (SIInstrInfo::isAtomicRet(Inst) && !SIInstrInfo::isGWS(Inst) &&
Inst.getOpcode() != AMDGPU::DS_APPEND &&
Inst.getOpcode() != AMDGPU::DS_CONSUME &&
Inst.getOpcode() != AMDGPU::DS_ORDERED_COUNT) {
for (const MachineOperand &Op : Inst.all_uses()) {
if (TRI->isVectorRegister(*MRI, Op.getReg()))
- setScoreByOperand(&Inst, Op, EXP_CNT, CurrScore);
+ setScoreByOperand(Op, EXP_CNT, CurrScore);
}
}
} else if (TII->isFLAT(Inst)) {
if (Inst.mayStore()) {
- setScoreByOperand(&Inst,
- *TII->getNamedOperand(Inst, AMDGPU::OpName::data),
+ setScoreByOperand(*TII->getNamedOperand(Inst, AMDGPU::OpName::data),
EXP_CNT, CurrScore);
} else if (SIInstrInfo::isAtomicRet(Inst)) {
- setScoreByOperand(&Inst,
- *TII->getNamedOperand(Inst, AMDGPU::OpName::data),
+ setScoreByOperand(*TII->getNamedOperand(Inst, AMDGPU::OpName::data),
EXP_CNT, CurrScore);
}
} else if (TII->isMIMG(Inst)) {
if (Inst.mayStore()) {
- setScoreByOperand(&Inst, Inst.getOperand(0), EXP_CNT, CurrScore);
+ setScoreByOperand(Inst.getOperand(0), EXP_CNT, CurrScore);
} else if (SIInstrInfo::isAtomicRet(Inst)) {
- setScoreByOperand(&Inst,
- *TII->getNamedOperand(Inst, AMDGPU::OpName::data),
+ setScoreByOperand(*TII->getNamedOperand(Inst, AMDGPU::OpName::data),
EXP_CNT, CurrScore);
}
} else if (TII->isMTBUF(Inst)) {
if (Inst.mayStore())
- setScoreByOperand(&Inst, Inst.getOperand(0), EXP_CNT, CurrScore);
+ setScoreByOperand(Inst.getOperand(0), EXP_CNT, CurrScore);
} else if (TII->isMUBUF(Inst)) {
if (Inst.mayStore()) {
- setScoreByOperand(&Inst, Inst.getOperand(0), EXP_CNT, CurrScore);
+ setScoreByOperand(Inst.getOperand(0), EXP_CNT, CurrScore);
} else if (SIInstrInfo::isAtomicRet(Inst)) {
- setScoreByOperand(&Inst,
- *TII->getNamedOperand(Inst, AMDGPU::OpName::data),
+ setScoreByOperand(*TII->getNamedOperand(Inst, AMDGPU::OpName::data),
...
[truncated]
|
Contributor
Author
|
The first commit of this patch is the untouched original patch. The second commit has the changes from the initial review. I did not add a new testcase, I want to but this issue is super difficult to reduce. It happens only in Blender cycles (and apparently only on one scene!). None of our lit tests, other llvm-test-suite projects, or internal test suite projects have any issues with this. I'm still trying to get a testcase but I am going on holidays soon so I'm not sure I'll have one in time. I'd like to land this without the added test case if possible (the buildbot will still flag any issue, even if it's not ideal) and I'll put a PR up for a test separately if I finally get a reduced version. |
Contributor
I do not understand why this changes anything. The old code should have worked, because accessing
Do you even have an unreduced reproduser, where this fix makes any difference at all to the behaviour? |
Contributor
Author
Yes, but we only iterated using the keys from the "Other" map. If the map in the current object had more keys, we did not visit (call
Yes; I'd not have claimed to have a fix otherwise :) |
Contributor
Author
|
Added a testcase reduced from the Blender source, and in which the waitcnts are affected if the merge is not done properly. |
Contributor
Yeah, I came to the same conclusion myself after I posted that comment earlier - the issue is that you can't elide the call to mergeScore for a key that is in this map but not the other map, because it's not a no-op even in that case. |
jayfoad
approved these changes
Dec 11, 2025
Contributor
|
We could probably revert #134835 now that |
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.
Fixed a crash in Blender due to some weird control flow.
The issue was with the "merge" function which was only looking at the
keys of the "Other" VMem/SGPR maps. It needs to look at the keys of both
maps and merge them.
Original commit message below
The pass was already "reinventing" the concept just to deal with 16 bit
registers. Clean up the entire tracking logic to only use register
units.
There are no test changes because functionality didn't change, except:
1 << 16)register units.
This also changes the tracking to use a DenseMap instead of a massive
fixed size table. This trades a bit of access speed for a smaller memory
footprint. Allocating and memsetting a huge table to zero caused a
non-negligible performance impact (I've observed up to 50% of the time
in the pass spent in the
memcpybuilt-in on a big test file).I also think we don't access these often enough to really justify using
a vector. We do a few accesses per instruction, but not much more. In a
huge 120MB LL file, I can barely see the trace of the DenseMap accesses.