[llvm] SelectionDAG: Improve expandFMINIMUM_FMAXIMUM (PR #137367)

[Matt Arsenault via llvm-commits]

================
@@ -8758,54 +8780,66 @@ SDValue TargetLowering::expandFMINIMUM_FMAXIMUM(SDNode *N,
   // First, implement comparison not propagating NaN. If no native fmin or fmax
   // available, use plain select with setcc instead.
   SDValue MinMax;
-  unsigned CompOpcIeee = IsMax ? ISD::FMAXNUM_IEEE : ISD::FMINNUM_IEEE;
-  unsigned CompOpc = IsMax ? ISD::FMAXNUM : ISD::FMINNUM;
-
-  // FIXME: We should probably define fminnum/fmaxnum variants with correct
-  // signed zero behavior.
-  bool MinMaxMustRespectOrderedZero = false;
-
-  if (isOperationLegalOrCustom(CompOpcIeee, VT)) {
-    MinMax = DAG.getNode(CompOpcIeee, DL, VT, LHS, RHS, Flags);
-    MinMaxMustRespectOrderedZero = true;
-  } else if (isOperationLegalOrCustom(CompOpc, VT)) {
-    MinMax = DAG.getNode(CompOpc, DL, VT, LHS, RHS, Flags);
-  } else {
-    if (VT.isVector() && !isOperationLegalOrCustom(ISD::VSELECT, VT))
-      return DAG.UnrollVectorOp(N);
-
-    // NaN (if exists) will be propagated later, so orderness doesn't matter.
-    SDValue Compare =
-        DAG.getSetCC(DL, CCVT, LHS, RHS, IsMax ? ISD::SETOGT : ISD::SETOLT);
-    MinMax = DAG.getSelect(DL, VT, Compare, LHS, RHS, Flags);
+  unsigned MinMaxOpcIeee = IsMax ? ISD::FMAXNUM_IEEE : ISD::FMINNUM_IEEE;
+  // TODO: Add ISD::FMAXNUM or ISD::FMINNUM when we are sure that they have
+  // the same behavior on all platforms.
+  unsigned MinMaxOpcNum2019 = IsMax ? ISD::FMAXIMUMNUM : ISD::FMINIMUMNUM;
+  unsigned MinMaxOpc = ISD::DELETED_NODE;
+
+  // Note: Currently, isOperationLegalOrCustom works well here, while if one
+  // add some new Opc type, please note that you may need to use Legal first
+  // and then Custom.
+  // If a backend defines MinMaxOpcNum2019 as Legal, and MinMaxOpcIeee as
+  // Custom in future, we will need to split to isOperationLegal group and
+  // isOperationCustom group.
+  if (isOperationLegalOrCustom(MinMaxOpcIeee, VT))
+    MinMaxOpc = MinMaxOpcIeee;
+  else if (isOperationLegalOrCustom(MinMaxOpcNum2019, VT))
+    MinMaxOpc = MinMaxOpcNum2019;
+  if (MinMaxOpc != ISD::DELETED_NODE) {
+    // TODO: we have another choice for NaNs
+    //   if RHS is NaN; then LHS = RHS; fi
+    //   if LHS is NaN; then RHS = LHS; fi
+    //   MinMax = MinMaxOpc (LHS, RHS)
+    // With this we can keep the payloads of NaNs and avoid load/store,
+    // while it may cause worse performance on platforms with advanced
+    // immediate loading support.
+    MinMax = DAG.getNode(MinMaxOpc, DL, VT, LHS, RHS, Flags);
+    if (!N->getFlags().hasNoNaNs() &&
+        (!DAG.isKnownNeverNaN(RHS) || !DAG.isKnownNeverNaN(LHS))) {
+      ConstantFP *FPNaN = ConstantFP::get(
+          *DAG.getContext(), APFloat::getNaN(VT.getFltSemantics()));
+      MinMax =
+          DAG.getSelect(DL, VT, DAG.getSetCC(DL, CCVT, LHS, RHS, ISD::SETUO),
+                        DAG.getConstantFP(*FPNaN, DL, VT), MinMax, Flags);
+    }
+    return MinMax;
   }
 
-  // Propagate any NaN of both operands
-  if (!N->getFlags().hasNoNaNs() &&
-      (!DAG.isKnownNeverNaN(RHS) || !DAG.isKnownNeverNaN(LHS))) {
-    ConstantFP *FPNaN = ConstantFP::get(*DAG.getContext(),
-                                        APFloat::getNaN(VT.getFltSemantics()));
-    MinMax = DAG.getSelect(DL, VT, DAG.getSetCC(DL, CCVT, LHS, RHS, ISD::SETUO),
-                           DAG.getConstantFP(*FPNaN, DL, VT), MinMax, Flags);
-  }
+  if (VT.isVector() && !isOperationLegalOrCustom(ISD::VSELECT, VT))
+    return DAG.UnrollVectorOp(N);
----------------
arsenm wrote:

Is this really necessary? Can't you just produce the vector op and let that legalize separately?

https://github.com/llvm/llvm-project/pull/137367