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

[Gheorghe-Teodor Bercea via Phabricator via llvm-commits]

doru1004 added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12896-12897
 
+  bool HasGlobalOrFlatAS =
+      AS == AMDGPUAS::GLOBAL_ADDRESS || AS == AMDGPUAS::FLAT_ADDRESS;
+
----------------
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?


================
Comment at: llvm/lib/Target/AMDGPU/SIISelLowering.cpp:12903-12906
+  bool UnsafeFPAtomicsDisabled =
+      RMW->getFunction()
+          ->getFnAttribute("amdgpu-unsafe-fp-atomics")
+          .getValueAsString() != "true";
----------------
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.


================
Comment at: llvm/test/CodeGen/AMDGPU/dag-divergence-atomic.ll:184
   %n64 = zext i32 %n32 to i64
   %p1 = getelementptr inbounds %S, %S addrspace(1)* %q, i64 %n64, i32 0
   store float 1.0, float addrspace(1)* %p1
----------------
arsenm wrote:
> This breaks the purpose of this test. These should be updated to have a scope that doesn't require the cas loop
I agree, this is a good point and I'll try to fix it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D139551