[PATCH] D127041: [LLVM] Add the support for fmax and fmin in atomicrmw instruction

Wed Jun 29 07:47:37 PDT 2022

tianshilei1992 added inline comments.

================
Comment at: llvm/lib/Transforms/InstCombine/InstCombineAtomicRMW.cpp:66-67
+    case AtomicRMWInst::FMax:
+      // maxnum(x, +inf) -> +inf
+      return !CF->isNegative() && CF->isInfinity();
+    case AtomicRMWInst::FMin:
----------------
arsenm wrote:
> This isn't correct for signaling nans
Like we mentioned above, `fmax` and `fmin` pair is expected to match the behavior of `llvm.maxnum.*` and `llvm.minnum.*`. Based on https://llvm.org/docs/LangRef.html#llvm-maxnum-intrinsic:
> If either operand is a NaN, returns the other non-NaN operand. Returns NaN only if both operands are NaN. The returned NaN is always quiet.
So here, no matter what value the pointer operand would be, NaN or not, as long as we have `+inf` for `maxnum`, `+inf` should be returned.
As for the signaling NaNs, the manual says:
> Unlike the IEEE-754 2008 behavior, this does not distinguish between signaling and quiet NaN inputs.
Therefore, I don't see how it is wrong. Could you expatiate it?

================
Comment at: llvm/test/Bitcode/compatibility.ll:870
+; CHECK: %atomicrmw.fmin = atomicrmw fmin float* %word, float 1.000000e+00 monotonic
+  %atomicrmw.fmin = atomicrmw fmin float* %word, float 1.0 monotonic
+
----------------
arsenm wrote:
> Since you also touched dxil, is an additional test needed for dxil encoding?
Thanks for catching that. Will do.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127041