llvmorg-github-actions[bot] wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-amdgpu Author: Pierre van Houtryve (Pierre-vh) <details> <summary>Changes</summary> This can be considered a RFC. I'd personally like to get rid of VMEMTypes but I don't know if anyone feels strongly that they should be kept. My motivation for removing VMemTypes is simple: They are just a repeat of VMEM events, just under a different name, and messier (defined as a basic enum but actually stored as a bitmask later). It's just confusing. This patch eliminates the need for them by: - Adding a new entrypoint in AMDGPUHWEvents to get the basic set of VMEM events issued by a VMEM Instruction. - Set BVH/SAMPLER events irrespective of whether the HW can track them. These events exist anyway, it should be up to InsertWaitCnt to deal with them properly (which is easy, only `counterOutOfOrder` needed work). - Tracking an additional set of per-VGPR "PendingEvents" which is set using the "basic set of VMEM events" and cleared as needed. I believe this is worth it because it streamlines the implementation of `SIInsertWaitCnts` and eliminates a bit of code duplication along the way. --- Full diff: https://github.com/llvm/llvm-project/pull/206440.diff 3 Files Affected: - (modified) llvm/lib/Target/AMDGPU/AMDGPUHWEvents.cpp (+7-6) - (modified) llvm/lib/Target/AMDGPU/AMDGPUHWEvents.h (+11) - (modified) llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp (+37-54) ``````````diff diff --git a/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.cpp b/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.cpp index 1087b08ac9d91..acfc54f9db04e 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.cpp +++ b/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.cpp @@ -56,8 +56,8 @@ static HWEvents getExpertSchedulingEventType(const MachineInstr &Inst, return HWEvents::NONE; } -static HWEvents getVmemHWEvent(const MachineInstr &Inst, const GCNSubtarget &ST, - const SIInstrInfo &TII) { +HWEvents getSimplifiedVMEMEventsFor(const MachineInstr &Inst, + const SIInstrInfo &TII) { switch (Inst.getOpcode()) { // FIXME: GLOBAL_INV needs to be tracked with xcnt too. case AMDGPU::GLOBAL_INV: @@ -82,7 +82,8 @@ static HWEvents getVmemHWEvent(const MachineInstr &Inst, const GCNSubtarget &ST, return HWEvents::SCRATCH_WRITE_ACCESS; return HWEvents::VMEM_WRITE_ACCESS; } - if (!ST.hasExtendedWaitCounts() || SIInstrInfo::isFLAT(Inst)) + + if (SIInstrInfo::isFLAT(Inst)) return HWEvents::VMEM_READ_ACCESS; if (SIInstrInfo::isImage(Inst)) { @@ -116,14 +117,14 @@ static HWEvents getEventsForImpl(const MachineInstr &Inst, if (TII.isFLAT(Inst)) { if (SIInstrInfo::isGFX12CacheInvOrWBInst(Inst.getOpcode())) - return getVmemHWEvent(Inst, ST, TII); + return getSimplifiedVMEMEventsFor(Inst, TII); assert(Inst.mayLoadOrStore()); HWEvents E = HWEvents::NONE; if (TII.mayAccessVMEMThroughFlat(Inst)) { if (ST.hasWaitXcnt()) E |= HWEvents::VMEM_GROUP; - E |= getVmemHWEvent(Inst, ST, TII); + E |= getSimplifiedVMEMEventsFor(Inst, TII); } if (TII.mayAccessLDSThroughFlat(Inst)) @@ -144,7 +145,7 @@ static HWEvents getEventsForImpl(const MachineInstr &Inst, // BUFFER_WBL2 is included here because unlike invalidates, has to be // followed "S_WAITCNT vmcnt(0)" is needed after to ensure the writeback has // completed. - HWEvents E = getVmemHWEvent(Inst, ST, TII); + HWEvents E = getSimplifiedVMEMEventsFor(Inst, TII); if (ST.hasWaitXcnt()) E |= HWEvents::VMEM_GROUP; if (ST.vmemWriteNeedsExpWaitcnt() && diff --git a/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.h b/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.h index 5901ee40177b3..bc5e10fae1c42 100644 --- a/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.h +++ b/llvm/lib/Target/AMDGPU/AMDGPUHWEvents.h @@ -21,6 +21,7 @@ namespace llvm { class GCNSubtarget; class MachineInstr; class raw_ostream; +class SIInstrInfo; namespace AMDGPU { @@ -164,6 +165,16 @@ class HWEvents { value_type Data = NONE; }; +/// \param Inst A VMEM instruction (as per `SIInstrInfo::isVMEM`). +/// \returns the simplified set of events triggered by the VMEM instruction \p +/// Inst. The returned mask is not exhaustive, but is guaranteed to be a subset +/// of the mask that'd be returned by \ref getEventsFor. +/// +/// Useful to quickly categorize VMEM instructions without having to fetch all +/// events. +HWEvents getSimplifiedVMEMEventsFor(const MachineInstr &Inst, + const SIInstrInfo &TII); + /// \returns A bitmask of HWEvent triggered by \p Inst HWEvents getEventsFor(const MachineInstr &Inst, const GCNSubtarget &ST, bool IsExpertMode); diff --git a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp index c261ae9c4b6c6..c496666601967 100644 --- a/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp +++ b/llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp @@ -114,21 +114,6 @@ static constexpr VMEMID toVMEMID(MCRegUnit RU) { namespace { -// 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 -// their results in order -- so there is no need to insert an s_waitcnt between -// two instructions of the same type that write the same vgpr. -enum VmemType { - // BUF instructions and MIMG instructions without a sampler. - VMEM_NOSAMPLER, - // MIMG instructions with a sampler. - VMEM_SAMPLER, - // BVH instructions - VMEM_BVH, - NUM_VMEM_TYPES -}; - // Maps values of InstCounterType to the instruction that waits on that // counter. Only used if GCNSubtarget::hasExtendedWaitCounts() // returns true, and does not cover VA_VDST or VM_VSRC. @@ -163,26 +148,6 @@ static bool isNormalMode(AMDGPU::InstCounterType MaxCounter) { } #endif // NDEBUG -VmemType getVmemType(const MachineInstr &Inst) { - assert(updateVMCntOnly(Inst)); - if (!SIInstrInfo::isImage(Inst)) - return VMEM_NOSAMPLER; - const AMDGPU::MIMGInfo *Info = AMDGPU::getMIMGInfo(Inst.getOpcode()); - const AMDGPU::MIMGBaseOpcodeInfo *BaseInfo = - AMDGPU::getMIMGBaseOpcodeInfo(Info->BaseOpcode); - - if (BaseInfo->BVH) - return VMEM_BVH; - - // We have to make an additional check for isVSAMPLE here since some - // instructions don't have a sampler, but are still classified as sampler - // instructions for the purposes of e.g. waitcnt. - if (BaseInfo->Sampler || BaseInfo->MSAA || SIInstrInfo::isVSAMPLE(Inst)) - return VMEM_SAMPLER; - - return VMEM_NOSAMPLER; -} - class WaitcntBrackets; // This abstracts the logic for generating and updating S_WAIT* instructions @@ -636,20 +601,20 @@ class WaitcntBrackets { void setPendingGDS() { LastGDS = ScoreUBs[AMDGPU::DS_CNT]; } // Return true if there might be pending writes to the vgpr-interval by VMEM - // instructions with types different from V. - bool hasOtherPendingVmemTypes(MCPhysReg Reg, VmemType V) const { + // instructions where the HWEvents in VGPRContext are not contained in E. + bool hasDifferentVGPRPendingEvents(MCPhysReg Reg, HWEvents E) const { for (MCRegUnit RU : regunits(Reg)) { auto It = VMem.find(toVMEMID(RU)); - if (It != VMem.end() && (It->second.VMEMTypes & ~(1 << V))) + if (It != VMem.end() && (It->second.VGPRPendingEvents & ~E).any()) return true; } return false; } - void clearVgprVmemTypes(MCPhysReg Reg) { + void clearVGPRPendingEvents(MCPhysReg Reg) { for (MCRegUnit RU : regunits(Reg)) { if (auto It = VMem.find(toVMEMID(RU)); It != VMem.end()) { - It->second.VMEMTypes = 0; + It->second.VGPRPendingEvents = HWEvents::NONE; if (It->second.empty()) VMem.erase(It); } @@ -772,10 +737,13 @@ class WaitcntBrackets { struct VMEMInfo { // Scores for all instruction counters. Zero-initialized. CounterValueArray Scores{}; - // Bitmask of the VmemTypes of VMEM instructions for this VGPR. - unsigned VMEMTypes = 0; + // For VGPRs, we need to track an additional fine-grained set of pending + // events. + HWEvents VGPRPendingEvents; - bool empty() const { return all_of(Scores, equal_to(0)) && !VMEMTypes; } + bool empty() const { + return all_of(Scores, equal_to(0)) && !VGPRPendingEvents; + } }; /// Wait cnt scores for every sgpr, the DS_CNT (corresponding to LGKMcnt @@ -884,7 +852,7 @@ bool WaitcntBrackets::hasPointSamplePendingVmemTypes(const MachineInstr &MI, if (!hasPointSampleAccel(MI)) return false; - return hasOtherPendingVmemTypes(Reg, VMEM_NOSAMPLER); + return hasDifferentVGPRPendingEvents(Reg, HWEvents::VMEM_READ_ACCESS); } void WaitcntBrackets::updateByEvent(HWEvents E, MachineInstr &Inst) { @@ -1025,16 +993,16 @@ void WaitcntBrackets::updateByEvent(HWEvents E, MachineInstr &Inst) { if (updateVMCntOnly(Inst)) { // updateVMCntOnly should only leave us with VGPRs // MUBUF, MTBUF, MIMG, FlatGlobal, and FlatScratch only have VGPR/AGPR - // defs. That's required for a sane index into `VgprMemTypes` below + // defs. assert(TRI.isVectorRegister(MRI, Op.getReg())); - VmemType V = getVmemType(Inst); - unsigned char TypesMask = 1 << V; + HWEvents VGPRContext = + AMDGPU::getSimplifiedVMEMEventsFor(Inst, Context->TII); // If instruction can have Point Sample Accel applied, we have to flag // this with another potential dependency if (hasPointSampleAccel(Inst)) - TypesMask |= 1 << VMEM_NOSAMPLER; + VGPRContext |= HWEvents::VMEM_READ_ACCESS; for (MCRegUnit RU : regunits(Op.getReg().asMCReg())) - VMem[toVMEMID(RU)].VMEMTypes |= TypesMask; + VMem[toVMEMID(RU)].VGPRPendingEvents |= VGPRContext; } } setScoreByOperand(Op, T, CurrScore); @@ -1552,6 +1520,19 @@ bool WaitcntBrackets::counterOutOfOrder(AMDGPU::InstCounterType T) const { HWEvents Events = PendingEvents & Context->getWaitEvents(T); + // If the target does not have extended counters, VMEM_BVH/SAMPLE_READ + // events are equivalent to VMEM_READ_ACCESS. We do not go out of order in + // such cases. + static constexpr HWEvents ExtendedImageEvents = + HWEvents::VMEM_SAMPLER_READ_ACCESS | HWEvents::VMEM_BVH_READ_ACCESS; + if (!Context->ST.hasExtendedWaitCounts() && + (Events & ExtendedImageEvents).any()) { + Events -= ExtendedImageEvents; // TODO: Tests pass even if I only use + // VMEM_SAMPLER_READ_ACCESS which isn't + // normal; indicates weak testing coverage + Events |= HWEvents::VMEM_READ_ACCESS; + } + // GLOBAL_INV completes in-order with other LOAD_CNT events, // so having GLOBAL_INV_ACCESS mixed with other LOAD_CNT // events doesn't cause out-of-order completion. @@ -2451,7 +2432,8 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore( // Additionally check instructions where Point Sample Acceleration // might be applied. if (Op.isUse() || !updateVMCntOnly(MI) || - ScoreBrackets.hasOtherPendingVmemTypes(Reg, getVmemType(MI)) || + ScoreBrackets.hasDifferentVGPRPendingEvents( + Reg, AMDGPU::getSimplifiedVMEMEventsFor(MI, TII)) || ScoreBrackets.hasPointSamplePendingVmemTypes(MI, Reg) || !ST.hasVmemWriteVgprInOrder()) { ScoreBrackets.determineWaitForPhysReg(AMDGPU::LOAD_CNT, Reg, Wait, @@ -2460,7 +2442,7 @@ bool SIInsertWaitcnts::generateWaitcntInstBefore( MI); ScoreBrackets.determineWaitForPhysReg(AMDGPU::BVH_CNT, Reg, Wait, MI); - ScoreBrackets.clearVgprVmemTypes(Reg); + ScoreBrackets.clearVGPRPendingEvents(Reg); } if (Op.isDef() || @@ -2848,9 +2830,10 @@ bool WaitcntBrackets::merge(const WaitcntBrackets &Other) { for (auto &[TID, Info] : VMem) { if (auto It = Other.VMem.find(TID); It != Other.VMem.end()) { - unsigned char NewVmemTypes = Info.VMEMTypes | It->second.VMEMTypes; - StrictDom |= NewVmemTypes != Info.VMEMTypes; - Info.VMEMTypes = NewVmemTypes; + HWEvents NewVGPRContext = + Info.VGPRPendingEvents | It->second.VGPRPendingEvents; + StrictDom |= NewVGPRContext != Info.VGPRPendingEvents; + Info.VGPRPendingEvents = NewVGPRContext; } } `````````` </details> https://github.com/llvm/llvm-project/pull/206440 _______________________________________________ llvm-branch-commits mailing list [email protected] https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
