[AMDGPU][SIInsertWaitcnts] Do not add s_waitcnt when the counters are known to be 0 already #65735
Conversation
| @@ -477,7 +483,7 @@ class SIInsertWaitcnts : public MachineFunctionPass { | |||
| bool generateWaitcnt(AMDGPU::Waitcnt Wait, | |||
| MachineBasicBlock::instr_iterator It, | |||
| MachineBasicBlock &Block, WaitcntBrackets &ScoreBrackets, | |||
| MachineInstr *OldWaitcntInstr); | |||
| MachineInstr *OldWaitcntInstr) const; | |||
There was a problem hiding this comment.
Looks like an unrelated clean up? Please commit it separately.
| @@ -45,6 +45,7 @@ define void @back_off_barrier_no_fence(ptr %in, ptr %out) #0 { | |||
| ; GFX11-BACKOFF-NEXT: s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0) | |||
| ; GFX11-BACKOFF-NEXT: flat_load_b32 v0, v[0:1] | |||
| ; GFX11-BACKOFF-NEXT: s_waitcnt vmcnt(0) lgkmcnt(0) | |||
| ; GFX11-BACKOFF-NEXT: s_waitcnt_vscnt null, 0x0 | |||
There was a problem hiding this comment.
I don't understand why your patch would ever cause an extra waitcnt like this.
| @@ -364,7 +371,6 @@ class SIInsertWaitcnts : public MachineFunctionPass { | |||
| const MachineRegisterInfo *MRI = nullptr; | |||
| AMDGPU::IsaVersion IV; | |||
|
|
|||
| DenseSet<MachineInstr *> TrackedWaitcntSet; | |||
There was a problem hiding this comment.
Is my understanding correct? Previously we used TrackedWaitcntSet to distinguish pre-existing waitcnts from waitcnts inserted by this pass. Now we don't need that, all we need is to know the difference between hard and soft waitcnts?
| let mayLoad = 1; | ||
| let mayStore = 1; | ||
| let has_sdst = 1; |
There was a problem hiding this comment.
Why are these needed? The definition of S_WAITCNT above does not have them.
jmmartinez
force-pushed
the
D156679
branch
2 times, most recently
from
c70532e
to
9eb43d6
Compare
|
1st, 2nd and 4th commits seem like obvious cleanups. LGTM, please commit them separately. |
… known to be 0 already
…-waitcnt to work around the memory-legalizer-atomic-fence tests getting optimized out
jmmartinez
changed the title
|
Hi Juan, the CQE cycle came back green, so this PR can be merged. Thanks! |
| @@ -34,6 +34,11 @@ static cl::opt<bool> AmdgcnSkipCacheInvalidations( | |||
| "amdgcn-skip-cache-invalidations", cl::init(false), cl::Hidden, | |||
| cl::desc("Use this to skip inserting cache invalidating instructions.")); | |||
|
|
|||
| static cl::opt<bool> AmdgcnDisableSoftWaitcnt( | |||
There was a problem hiding this comment.
Instead of this I suggest you change memory-legalizer-atomic-fence.ll to use -stop-after=si-memory-legalizer (and generate the checks in it with utils/update_mir_test_checks.py).
|
@kzhuravl that's awesome! Thanks for the update. However, I am not able to rebase and address Jay's review since I don't have a PC available (for the next 6 months). If somebody else could move the PR forward it would be great. Otherwise I'll finish it when I'm back. |
|
Hi @jwanggit86, can you please take over this PR? This needs to be rebased, and review comments addressed, thanks! |
|
@kzhuravl Sure. I'll take over. |
|
@Pierre-vh got some cycles to look into this now, so re-assigning to him. |
Currently a WIP, I've only updated a single test to depict the change (there are ~50 tests that need update).
Differential Revision: https://reviews.llvm.org/D156679