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

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

LGTM besides the test issue



================
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:
----------------
tianshilei1992 wrote:
> 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?
Right, ugh. These really should have been named fmin/fmax


================
Comment at: llvm/test/Assembler/atomic.ll:50-55
+; CHECK: atomicrmw fmax float* %x, float 1.000000e+00 seq_cst
+  atomicrmw fmax float* %x, float 1.0 seq_cst
+
+  ; CHECK: atomicrmw volatile fmax float* %x, float 1.000000e+00 seq_cst
+  atomicrmw volatile fmax float* %x, float 1.0 seq_cst
+
----------------
Missing fmin test


================
Comment at: llvm/test/Transforms/InstCombine/atomicrmw.ll:304
+}
 
+; CHECK-LABEL: sat_fmin_inf
----------------
Could use some negative tests


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127041