[PATCH] D139551: [AMDGPU] Use CAS loop for min/max atomics at system scope

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12896-12897
 
+  bool HasGlobalOrFlatAS =
+      AS == AMDGPUAS::GLOBAL_ADDRESS || AS == AMDGPUAS::FLAT_ADDRESS;
+
----------------
doru1004 wrote:
> arsenm wrote:
> > Should use AMDGPU::isFlatGlobalAddrSpace, in principal the same should apply to any other random address space number
> This check here is just a factoring out of the switch statement since now this check is done in 2 places. Are you implying that this check was wrong or insufficient before?
Yes


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12903-12906
+  bool UnsafeFPAtomicsDisabled =
+      RMW->getFunction()
+          ->getFnAttribute("amdgpu-unsafe-fp-atomics")
+          .getValueAsString() != "true";
----------------
doru1004 wrote:
> arsenm wrote:
> > move to predicate function to call at the use sites?
> Could you please give me more details regarding this request? Again this is a check which was factored out of the switch statement because the same check is now done in two different locations.
bool unsafeAtomicsDisabled(const Function &F) {
  return ....
}


and call that at the use sites rather than eagerly parsing the attribute


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139551