[PATCH] D156301: [AMDGPU] Support FAdd/FSub global atomics in AMDGPUAtomicOptimizer.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:327
+
+  // TODO: Operand order is not consistent for atomic fadd intrinsics
+  if (Op == AtomicRMWInst::FAdd) {
----------------
The intrinsics should just be deleted, everything should move to atomicrmw


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:445
     // Reduce across the upper and lower 32 lanes.
-    return buildNonAtomicBinOp(
-        B, Op, V, B.CreateIntrinsic(Intrinsic::amdgcn_permlane64, {}, V));
+    V = isAtomicFloatingPointTy ? B.CreateBitCast(V, Int32Ty) : V;
+    Value *Permlane64Call =
----------------
You can just unconditionally call CreateBitCast, it's a no-op if the type matches anyway


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:712
+  case AtomicRMWInst::FAdd:
+    return APFloat::getZero(Semantics, false);
+  case AtomicRMWInst::FSub:
----------------
Identity value for fadd is -0, you got these backwards


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:714
+  case AtomicRMWInst::FSub:
+    return APFloat::getZero(Semantics, true);
+  }
----------------
identity for fsub is +0, so no true


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:814-818
+  if (Ty->isIntegerTy()) {
+    Identity = B.getInt(getIdentityValueForAtomicOp(Op, TyBitWidth));
+  } else if (isAtomicFloatingPointTy) {
+    const fltSemantics &Semantics = Ty->getScalarType()->getFltSemantics();
+    Identity = ConstantFP::get(C, getIdentityValueForFAtomicOp(Op, Semantics));
----------------
Can you just make getIdentityValueForAtomicOp return a Constant? Or add a variant that does?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156301