[PATCH] D156301: [WIP] Support FP global atomics in AMDGPUAtomicOptimizer.

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

the fmin/fmax case and fadd/fsub cases have nothing to do with each other, you're probably better off handling them in separate patches



================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:142
+
+bool isOpFP(AtomicRMWInst::BinOp &Op) {
+  switch (Op) {
----------------
AtomicRMWInst already has isFloatingPointOperation/isFPOperation for this, which also picks up fsub


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:224
+  case AtomicRMWInst::FMax:
+  case AtomicRMWInst::FMin:
     break;
----------------
Should also handle fsub


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:410
+  case AtomicRMWInst::FMax:
+    Pred = CmpInst::FCMP_UGT;
+    break;
----------------
you can't do it like this, you should use minnum/maxnum intrinsics


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:650
+  case AtomicRMWInst::FMax:
+    return APFloat::getSmallest(APFloat::IEEEsingle(), false);
+  case AtomicRMWInst::FMin:
----------------
This would be +infinity for fmax.

For fadd you there isn't really an identity value since fadd -0, 0 -> -0. You probably can't do this without nsz, which we don't have a way of representing.

I have a draft patch for unsafe FP atomic metadata I don't have time to pick up.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:652
+  case AtomicRMWInst::FMin:
+    return APFloat::getLargest(APFloat::IEEEsingle(), false);
+  }
----------------
This would be -infinity


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:822
+      Value *const CtpopFP = B.CreateUIToFP(Ctpop, Ty);
+      NewV = B.CreateFMul(V, CtpopFP);
+      break;
----------------
I don't follow how this can be a convert and multiply


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156301