[PATCH] D137655: Expand fminimum/fmaximum into fminnum/fmaxnum + NaN check

[Matt Arsenault via Phabricator via llvm-commits]

arsenm added a comment.

In D137655#3918964 <https://reviews.llvm.org/D137655#3918964>, @gflegar wrote:

> In D137655#3917359 <https://reviews.llvm.org/D137655#3917359>, @nikic wrote:
>
>> I don't think this is right either. LLVM defines minnum according to the old semantics, which don't specify an order between zeroes. We'd need a separate ISD opcode for minnum according to 2018 semantics.
>
> Interestingly though, for the NVPTX backend this will end up producing the correct code, since minnum is lowered to the min/max PTX instructions, which defines the 2018 semantics. (I do agree though that the intermediate code is not correct.)

It's not OK to have wrong intermediate code. We do have the "new" semantic opcodes already in FMINIMUM/FMAXIMUM



================
Comment at: llvm/lib/Target/NVPTX/NVPTXISelLowering.cpp:2227-2228
+  SDValue NonPropagatingResult = DAG.getNode(NT, SL, VT, {LHS, RHS});
+  SDValue NaN = DAG.getConstantFP(std::numeric_limits<double>::quiet_NaN(), SL,
+                                  VT);
+  return DAG.getSelectCC(SL, LHS, RHS, NaN, NonPropagatingResult, ISD::SETUO);
----------------
Should go through APFloat 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137655