[PATCH] D106891: [AMDGPU] [Remarks] Emit optimization remarks for atomics generating CAS loop

[Stanislav Mekhanoshin via Phabricator via llvm-commits]

rampitec added inline comments.


================
Comment at: clang/test/CodeGenCUDA/fp-atomics-optremarks.cu:10
+
+// GFX90A-CAS: A compare and swap loop was generated for an atomic operation at system memory scope
+// GFX90A-CAS-LABEL: _Z14atomic_add_casPf
----------------
gandhi21299 wrote:
> rampitec wrote:
> > gandhi21299 wrote:
> > > rampitec wrote:
> > > > Need tests for all scopes.
> > > `__atomic_fetch_add` does not take scope as an argument, how could I add tests with different scopes?
> > At least in the IR test.
> What do you mean by that?
You need to test all of that. If you cannot write a proper .cu test, then write an IR test and run llc.


================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:618
       expandAtomicRMWToCmpXchg(AI, createCmpXchgInstFun);
+      Ctx.getSyncScopeNames(SSNs);
+      auto MemScope = SSNs[AI->getSyncScopeID()].empty()
----------------
gandhi21299 wrote:
> rampitec wrote:
> > Only if SSNs.empty().
> Sorry, what do you mean? SSN will be empty at that point.
I thought want to cache it. But really just declare it here.


================
Comment at: llvm/lib/CodeGen/AtomicExpandPass.cpp:624
+        Remark << "A compare and swap loop was generated for an "
+               << AI->getOpcodeName() << "operation at " << MemScope
+               << " memory scope";
----------------
gandhi21299 wrote:
> rampitec wrote:
> > I believe getOpcodeName() will return "atomicrmw" instead of the operation. Also missing space after it.
> getOpcodeName() returns `atomicrmwoperation`, as per the tests the spacing looks correct to me.
The operation to report is AI->getOperation(). Spacing is wrong, "operation" is your text.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D106891