[SDAG] Generate FMINNUM/FMAXNUM/FMINNAN/FMAXNAN from SDAGBuilder

[jmolloy]

Summary

These only get generated if the target supports them. If one of the variants is not legal and the other is, and it is safe to do so, the other variant will be emitted.

For example on AArch32 (V8), we have scalar fminnm but not fmin.

Fix up a couple of tests while we're here - one now produces better code, and the other was just plain wrong to start with.

This is tested by the fact that both ARM and AArch64 mark these nodes as legal.

I know this is basically mechanical, but as it's in the target independent layer I'd appreciate peoples' eyes on it.

[mcrosier]

LGTM, but I'd still be interested in seeing comments from others that are more familiar with this code.

[jmolloy]

Hi David,

Do you have any comments on this?

Cheers,

James

[hfinkel]

LGTM.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

void SelectionDAGBuilder::visitSelect(const User &I) {
   SDValue LHSVal   = getValue(I.getOperand(1));
   SDValue RHSVal   = getValue(I.getOperand(2));
   auto BaseOps = {Cond};
   ISD::NodeType OpCode = Cond.getValueType().isVector() ?
     ISD::VSELECT : ISD::SELECT;
 
   // Min/max matching is only viable if all output VTs are the same.
   if (std::equal(ValueVTs.begin(), ValueVTs.end(), ValueVTs.begin())) {
+    EVT VT = ValueVTs[0];
+    LLVMContext &Ctx = *DAG.getContext();
+    auto &TLI = DAG.getTargetLoweringInfo();
+    while (TLI.getTypeAction(Ctx, VT) == TargetLoweringBase::TypeSplitVector)
+      VT = TLI.getTypeToTransformTo(Ctx, VT);
+
     Value *LHS, *RHS;
-    SelectPatternFlavor SPF =
-      matchSelectPattern(const_cast<User*>(&I), LHS, RHS).Flavor;
+    auto SPR = matchSelectPattern(const_cast<User*>(&I), LHS, RHS);
     ISD::NodeType Opc = ISD::DELETED_NODE;
-    switch (SPF) {
-    case SPF_UMAX: Opc = ISD::UMAX; break;
-    case SPF_UMIN: Opc = ISD::UMIN; break;
-    case SPF_SMAX: Opc = ISD::SMAX; break;
-    case SPF_SMIN: Opc = ISD::SMIN; break;
+    switch (SPR.Flavor) {
+    case SPF_UMAX:    Opc = ISD::UMAX; break;
+    case SPF_UMIN:    Opc = ISD::UMIN; break;
+    case SPF_SMAX:    Opc = ISD::SMAX; break;
+    case SPF_SMIN:    Opc = ISD::SMIN; break;
+    case SPF_FMINNUM:
+      switch (SPR.NaNBehavior) {
+      case SPNB_NA: llvm_unreachable("No NaN behavior for FP op?");
+      case SPNB_RETURNS_NAN:   Opc = ISD::FMINNAN; break;
+      case SPNB_RETURNS_OTHER: Opc = ISD::FMINNUM; break;
+      case SPNB_RETURNS_ANY:
+        Opc = TLI.isOperationLegalOrCustom(ISD::FMINNUM, VT) ? ISD::FMINNUM
+          : ISD::FMINNAN;
+        break;
+      }
+      break;
+    case SPF_FMAXNUM:
+      switch (SPR.NaNBehavior) {
+      case SPNB_NA: llvm_unreachable("No NaN behavior for FP op?");
+      case SPNB_RETURNS_NAN:   Opc = ISD::FMAXNAN; break;
+      case SPNB_RETURNS_OTHER: Opc = ISD::FMAXNUM; break;
+      case SPNB_RETURNS_ANY:
+        Opc = TLI.isOperationLegalOrCustom(ISD::FMAXNUM, VT) ? ISD::FMAXNUM
+          : ISD::FMAXNAN;
+        break;
+      }
+      break;
     default: break;
     }
 
-    EVT VT = ValueVTs[0];
-    LLVMContext &Ctx = *DAG.getContext();
-    auto &TLI = DAG.getTargetLoweringInfo();
-    while (TLI.getTypeAction(Ctx, VT) == TargetLoweringBase::TypeSplitVector)
-      VT = TLI.getTypeToTransformTo(Ctx, VT);
-
     if (Opc != ISD::DELETED_NODE && TLI.isOperationLegalOrCustom(Opc, VT) &&
         // If the underlying comparison instruction is used by any other instruction,
         // the consumed instructions won't be destroyed, so it is not profitable
         // to convert to a min/max.
         cast<SelectInst>(&I)->getCondition()->hasOneUse()) {
       OpCode = Opc;
       LHSVal = getValue(LHS);
       RHSVal = getValue(RHS);

test/CodeGen/AArch64/arm64-fmax-safe.ll

 ; RUN: llc -march=arm64 < %s | FileCheck %s
 
 define double @test_direct(float %in) {
 ; CHECK-LABEL: test_direct:
   %cmp = fcmp olt float %in, 0.000000e+00
   %val = select i1 %cmp, float 0.000000e+00, float %in
   %longer = fpext float %val to double
   ret double %longer
 
-; CHECK: fmax
+; CHECK: fmax s
 }
 
 define double @test_cross(float %in) {
 ; CHECK-LABEL: test_cross:
   %cmp = fcmp ult float %in, 0.000000e+00
   %val = select i1 %cmp, float %in, float 0.000000e+00
   %longer = fpext float %val to double
   ret double %longer
 
-; CHECK: fmin
+; CHECK: fmin s
 }
 
 ; Same as previous, but with ordered comparison;
-; can't be converted in safe-math mode.
+; must become fminnm, not fmin.
 define double @test_cross_fail_nan(float %in) {
 ; CHECK-LABEL: test_cross_fail_nan:
   %cmp = fcmp olt float %in, 0.000000e+00
   %val = select i1 %cmp, float %in, float 0.000000e+00
   %longer = fpext float %val to double
   ret double %longer
 
-; CHECK: fcsel s0, s0, s1, mi
+; CHECK: fminnm s
 }
 
 ; This isn't a min or a max, but passes the first condition for swapping the
 ; results. Make sure they're put back before we resort to the normal fcsel.
 define float @test_cross_fail(float %lhs, float %rhs) {
 ; CHECK-LABEL: test_cross_fail:
   %tst = fcmp une float %lhs, %rhs
   %res = select i1 %tst, float %rhs, float %lhs

test/CodeGen/ARM/vminmaxnm.ll

 
 define float @fp-armv8_vminmaxnm_neg0(float %a) {
 ; CHECK-LABEL: "fp-armv8_vminmaxnm_neg0":
 ; CHECK-NOT: vcmp
 ; CHECK: vminnm.f32
 ; CHECK: vmaxnm.f32
   %cmp1 = fcmp fast olt float %a, -0.
   %cond1 = select i1 %cmp1, float %a, float -0.
-  %cmp2 = fcmp fast ogt float %cond1, -0.
+  %cmp2 = fcmp fast ugt float %cond1, -0.
   %cond2 = select i1 %cmp2, float %cond1, float -0.
   ret float %cond2
 }
 
 define float @fp-armv8_vminmaxnm_e_0(float %a) {
 ; CHECK-LABEL: "fp-armv8_vminmaxnm_e_0":
 ; CHECK-NOT: vcmp
 ; CHECK: vminnm.f32