[PATCH] D152370: [Intrinsic] Introduce reduction intrinsics for minimum/maximum

[Anna Thomas via Phabricator via llvm-commits]

anna added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/ISDOpcodes.h:1294
   VECREDUCE_FMIN,
+  /// FMINIMUM/FMAXIMUM nodes support NaNs and signed zeroes compared to the
+  /// FMIN/FMAX variant above.
----------------
craig.topper wrote:
> FMAX/FMIN support NaNs they just don't propagate them unless all inputs are NaN.
yes, the way of propagation is different. will update comment.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp:12313
+  case ISD::FMAXIMUM: {
+    // Neutral element for fminnum/fminimum is NaN, Inf or FLT_MAX, depending on FMF.
     const fltSemantics &Semantics = EVTToAPFloatSemantics(VT);
----------------
craig.topper wrote:
> The neutral element for FMINIMUM and FMAXIMUM shouldn't be NaN. That would turn the entire result into NaN.
you're right. I completely missed what "neutral" actually means here. Would this be correct:
```
case ISD::FMAXIMUM:
case ISD::FMINIMUM:
  
const fltSemantics &Semantics = EVTToAPFloatSemantics(VT);
    APFloat NeutralAF = 
                        !Flags.hasNoInfs() ? APFloat::getInf(Semantics) :
                        APFloat::getLargest(Semantics);
    if (Opcode == ISD::FMAXIMUM)
      NeutralAF.changeSign();
```
i.e. neutral is either inf (if present) or the largest element for the `VT`.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152370