[PATCH] D64328: [AMDGPU] Optimize atomic max/min

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:322
+  APInt IdentityVal;
+  switch (Op) {
+  default:
----------------
Can you move this to a separate getIdentityValue function?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:328
+  case AtomicRMWInst::UMax:
+    IdentityVal = APInt::getMinValue(TyBitWidth);
+    break;
----------------
Shouldn't this just be zero for add/sub?


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:330-338
+  case AtomicRMWInst::UMin:
+    IdentityVal = APInt::getMaxValue(TyBitWidth);
+    break;
+  case AtomicRMWInst::Max:
+    IdentityVal = APInt::getSignedMinValue(TyBitWidth);
+    break;
+  case AtomicRMWInst::Min:
----------------
The min and max values seem backwards? min(MinValue, X) -> MinValue, not X


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:388
       CallInst *const ReadLaneLo = B.CreateIntrinsic(
           Intrinsic::amdgcn_readlane, {}, {ExtractLo, B.getInt32(63)});
       CallInst *const ReadLaneHi = B.CreateIntrinsic(
----------------
I don't understand why these are reading lane 63. Why not lane 0? Why not readfirstlane? This won't work with wave64, but I guess that's an existing problem


================
Comment at: llvm/test/CodeGen/AMDGPU/atomic_optimizations_local_pointer.ll:240
+  ret void
+}
----------------
foad wrote:
> arsenm wrote:
> > Can you add tests for i64?
> Will do, though I'm little nervous of both (a) the combinatorial explosion of unit tests ({add,sub,max,min,umax,umin} x {i32,i64} x {constant,uniform,varying} x {buffer,rawbuffer,structbuffer,localpoint,globalpointer} ...) and (b) the lack of any end-to-end tests to check that the optimization is actually doing the right thing and computing the right result.
I pretty consistently find bugs by enumerating all of the combinations. End-to-end execution tests would be nice, but won't be in the llvm tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D64328