[AMDGPU][SIInsertWaitcnts] Do not add s_waitcnt when the counters are known to be 0 already #65735
Conversation
There was a problem hiding this comment.
Looks like an unrelated clean up? Please commit it separately.
There was a problem hiding this comment.
I don't understand why your patch would ever cause an extra waitcnt like this.
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?
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! |
| "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. |
6 participants
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