[PATCH] D96639: [AMDGPU] Add two TSFlags: IsAtomicNoRtn and IsAtomicRtn

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIInsertWaitcnts.cpp:572
         setExpScore(&Inst, TII, TRI, MRI, 0, CurrScore);
-      } else if (AMDGPU::getAtomicNoRetOp(Inst.getOpcode()) != -1) {
         setExpScore(
----------------
foad wrote:
> rampitec wrote:
> > foad wrote:
> > > t-tye wrote:
> > > > foad wrote:
> > > > > foad wrote:
> > > > > > rampitec wrote:
> > > > > > > @foad do I get it right, all MIMG atomics are rtn only? MIMG has never used AtomicNoRet map, so this branch was dead. Maybe we were missing to wait for exp, maybe they all mayStore, or maybe it is really dead an unused. Potentially you will see some extra exp waits, but it didn't appear in our tests. That said I am afraid test coverage for waitcounts is not sufficient.
> > > > > > That's not true, architecturally all image atomics have ret/noret versions controlled by the glc bit just like buffer instructions. But I guess in LLVM we have never bothered to select the noret instructions.
> > > > > Incidentally I don't understand this code at all. Surely all atomics are both mayLoad and mayStore, so this will have taken the "if" path on line 570?
> > > > The hardware can be configured as to whether IMG instructions behave as non-IMG instruction wit respect to the waitcnts and the ordering. For HSA the hardware is configured to make them behave the same.
> > > > Incidentally I don't understand this code at all. Surely all atomics are both mayLoad and mayStore, so this will have taken the "if" path on line 570?
> > > 
> > > Just to prove my point, this still passes the test suite: https://reviews.llvm.org/differential/diff/323776/
> > I am afraid we do not have tests covering every path in this pass.
> I am saying this is dead code because every atomic is mayLoad and mayStore. Do you disagree?
Theoretically an atomic load can eecape that.


================
Comment at: llvm/lib/Target/AMDGPU/SIInstrInfo.h:557
+
+  static bool isAtomic(const MachineInstr &MI) {
+    return MI.getDesc().TSFlags & (SIInstrFlags::IsAtomicRtn |
----------------
foad wrote:
> I am surprised that there isn't already a MachineInstr::IsAtomic method.
Me either.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96639/new/

https://reviews.llvm.org/D96639