DAG: Use getNegatedExpression in combineMinNumMaxNum

[arsenm]

Summary

Computing the negated RHS expression just to see if it compares equal
and throw it away feels dirty.

[arsenm]

ping

[RKSimon]

llvm/include/llvm/CodeGen/TargetLowering.h

4098

(style) either create a if (Neg) {} outer wrapper or do a if (!Neg) return SDValue(); early-out

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

392

drop DAG?

10402

Might be worth converting the static method to DAGCombiner::combineMinNumMaxNum as a NFC pre-commit.

10420–10429

Move this into the braces above? Its looks that's the only place Combined could be set

[arsenm]

Address comments, reduce duplication

[RKSimon]

LGTM - cheers

[arsenm]

65420c8041f4ca44a3a14c5f7faf426ee6a7c6a4

llvm/include/llvm/CodeGen/TargetLowering.h

public:
   /// Return the newly negated expression if the cost is not expensive and
   /// set the cost in \p Cost to indicate that if it is cheaper or neutral to
   /// do the negation.
   virtual SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG,
                                        bool LegalOps, bool OptForSize,
                                        NegatibleCost &Cost,
                                        unsigned Depth = 0) const;
 
-  /// This is the helper function to return the newly negated expression only
-  /// when the cost is cheaper.
-  SDValue getCheaperNegatedExpression(SDValue Op, SelectionDAG &DAG,
-                                      bool LegalOps, bool OptForSize,
-                                      unsigned Depth = 0) const {
+  SDValue getCheaperOrNeutralNegatedExpression(
+      SDValue Op, SelectionDAG &DAG, bool LegalOps, bool OptForSize,
+      const NegatibleCost CostThreshold = NegatibleCost::Neutral,
     NegatibleCost Cost = NegatibleCost::Expensive;
     SDValue Neg =
         getNegatedExpression(Op, DAG, LegalOps, OptForSize, Cost, Depth);
-    if (Neg && Cost == NegatibleCost::Cheaper)
+    if (!Neg)
+      return SDValue();
+
+    if (Cost <= CostThreshold)
       return Neg;
+
     // Remove the new created node to avoid the side effect to the DAG.
-    if (Neg && Neg->use_empty())
+    if (Neg->use_empty())
       DAG.RemoveDeadNode(Neg.getNode());
     return SDValue();
   }
 
+  /// This is the helper function to return the newly negated expression only
+  /// when the cost is cheaper.
+  SDValue getCheaperNegatedExpression(SDValue Op, SelectionDAG &DAG,
+                                      bool LegalOps, bool OptForSize,
+                                      unsigned Depth = 0) const {
+    return getCheaperOrNeutralNegatedExpression(Op, DAG, LegalOps, OptForSize,
+                                                NegatibleCost::Cheaper, Depth);
+  }
+
   /// This is the helper function to return the newly negated expression if
   /// the cost is not expensive.
   SDValue getNegatedExpression(SDValue Op, SelectionDAG &DAG, bool LegalOps,
                                bool OptForSize, unsigned Depth = 0) const {
     NegatibleCost Cost = NegatibleCost::Expensive;
     return getNegatedExpression(Op, DAG, LegalOps, OptForSize, Cost, Depth);
   }
 

 
   // Lower custom output constraints. If invalid, return SDValue().
   virtual SDValue LowerAsmOutputForConstraint(SDValue &Chain, SDValue &Flag,
                                               const SDLoc &DL,
                                               const AsmOperandInfo &OpInfo,
                                               SelectionDAG &DAG) const;
 
   // Targets may override this function to collect operands from the CallInst
-  // and for example, lower them into the SelectionDAG operands. 
+  // and for example, lower them into the SelectionDAG operands.
   virtual void CollectTargetIntrinsicOperands(const CallInst &I,
                                               SmallVectorImpl<SDValue> &Ops,
                                               SelectionDAG &DAG) const;
 
   //===--------------------------------------------------------------------===//
   // Div utility functions
   //
 

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

private:
     SDValue PromoteOperand(SDValue Op, EVT PVT, bool &Replace);
     SDValue SExtPromoteOperand(SDValue Op, EVT PVT);
     SDValue ZExtPromoteOperand(SDValue Op, EVT PVT);
     SDValue PromoteIntBinOp(SDValue Op);
     SDValue PromoteIntShiftOp(SDValue Op);
     SDValue PromoteExtend(SDValue Op);
     bool PromoteLoad(SDValue Op);
 
+    SDValue combineMinNumMaxNum(const SDLoc &DL, EVT VT, SDValue LHS,
+                                SDValue RHS, SDValue True, SDValue False,
+                                ISD::CondCode CC);
+
     /// Call the node-specific routine that knows how to fold each
     /// particular type of node. If that doesn't do anything, try the
     /// target-specific DAG combines.
     SDValue combine(SDNode *N);
 
     // Visitation implementation - Implement dag node combining for different
     // node types.  The semantics are as follows:
     // Return Value:
case ISD::SETUGE: {
     return SDValue();
   }
   default:
     return SDValue();
   }
 }
 
 /// Generate Min/Max node
-static SDValue combineMinNumMaxNum(const SDLoc &DL, EVT VT, SDValue LHS,
-                                   SDValue RHS, SDValue True, SDValue False,
-                                   ISD::CondCode CC, const TargetLowering &TLI,
-                                   SelectionDAG &DAG) {
+SDValue DAGCombiner::combineMinNumMaxNum(const SDLoc &DL, EVT VT, SDValue LHS,
+                                         SDValue RHS, SDValue True,
+                                         SDValue False, ISD::CondCode CC) {
   if ((LHS == True && RHS == False) || (LHS == False && RHS == True))
     return combineMinNumMaxNumImpl(DL, VT, LHS, RHS, True, False, CC, TLI, DAG);
 
   // If we can't directly match this, try to see if we can pull an fneg out of
   // the select.
-  if (True.getOpcode() != ISD::FNEG)
+  SDValue NegTrue = TLI.getCheaperOrNeutralNegatedExpression(
+      True, DAG, LegalOperations, ForCodeSize);
+  if (!NegTrue)
     return SDValue();
 
-  ConstantFPSDNode *CRHS = dyn_cast<ConstantFPSDNode>(RHS);
-  ConstantFPSDNode *CFalse = dyn_cast<ConstantFPSDNode>(False);
-  SDValue NegTrue = True.getOperand(0);
+  HandleSDNode NegTrueHandle(NegTrue);
 
   // Try to unfold an fneg from the select if we are comparing the negated
   // constant.
   //
   // select (setcc x, K) (fneg x), -K -> fneg(minnum(x, K))
   //
   // TODO: Handle fabs
-  if (LHS == NegTrue && CFalse && CRHS) {
-    APFloat NegRHS = neg(CRHS->getValueAPF());
-    if (NegRHS == CFalse->getValueAPF()) {
+  if (LHS == NegTrue) {
+    // If we can't directly match this, try to see if we can pull an fneg out of
+    // the select.
+    SDValue NegRHS = TLI.getCheaperOrNeutralNegatedExpression(
+        RHS, DAG, LegalOperations, ForCodeSize);
+    if (NegRHS) {
+      HandleSDNode NegRHSHandle(NegRHS);
+      if (NegRHS == False) {
         SDValue Combined = combineMinNumMaxNumImpl(DL, VT, LHS, RHS, NegTrue,
                                                    False, CC, TLI, DAG);
-      if (Combined)
         return DAG.getNode(ISD::FNEG, DL, VT, Combined);
-      return SDValue();
+      }
     }
   }
 
   return SDValue();
 }
 
 /// If a (v)select has a condition value that is a sign-bit test, try to smear
 /// the condition operand sign-bit across the value width and use it as a mask.
if (N0.getOpcode() == ISD::SETCC) {
     SDValue Cond0 = N0.getOperand(0), Cond1 = N0.getOperand(1);
     ISD::CondCode CC = cast<CondCodeSDNode>(N0.getOperand(2))->get();
 
     // select (fcmp lt x, y), x, y -> fminnum x, y
     // select (fcmp gt x, y), x, y -> fmaxnum x, y
     //
     // This is OK if we don't care what happens if either operand is a NaN.
     if (N0.hasOneUse() && isLegalToCombineMinNumMaxNum(DAG, N1, N2, TLI))
-      if (SDValue FMinMax = combineMinNumMaxNum(DL, VT, Cond0, Cond1, N1, N2,
-                                                CC, TLI, DAG))
+      if (SDValue FMinMax =
+              combineMinNumMaxNum(DL, VT, Cond0, Cond1, N1, N2, CC))
         return FMinMax;
 
     // Use 'unsigned add with overflow' to optimize an unsigned saturating add.
     // This is conservatively limited to pre-legal-operations to give targets
     // a chance to reverse the transform if they want to do that. Also, it is
     // unlikely that the pattern would be formed late, so it's probably not
     // worth going through the other checks.
     if (!LegalOperations && TLI.isOperationLegalOrCustom(ISD::UADDO, VT) &&
if (N0.getOpcode() == ISD::SETCC) {
 
     // vselect x, y (fcmp lt x, y) -> fminnum x, y
     // vselect x, y (fcmp gt x, y) -> fmaxnum x, y
     //
     // This is OK if we don't care about what happens if either operand is a
     // NaN.
     //
     if (N0.hasOneUse() && isLegalToCombineMinNumMaxNum(DAG, LHS, RHS, TLI)) {
-      if (SDValue FMinMax =
-              combineMinNumMaxNum(DL, VT, LHS, RHS, N1, N2, CC, TLI, DAG))
+      if (SDValue FMinMax = combineMinNumMaxNum(DL, VT, LHS, RHS, N1, N2, CC))
         return FMinMax;
     }
 
     if (SDValue S = PerformMinMaxFpToSatCombine(LHS, RHS, N1, N2, CC, DAG))
       return S;
     if (SDValue S = PerformUMinFpToSatCombine(LHS, RHS, N1, N2, CC, DAG))
       return S;
 

llvm/test/CodeGen/AMDGPU/fneg-combines.f16.ll

 ; VI-NSZ-NEXT:    v_mul_f16_e32 v0, v1, v4
 ; VI-NSZ-NEXT:    v_mul_f16_e32 v1, v1, v5
 ; VI-NSZ-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; GFX11-SAFE-LABEL: free_fold_src_code_size_cost_use_f16:
 ; GFX11-SAFE:       ; %bb.0:
 ; GFX11-SAFE-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)
 ; GFX11-SAFE-NEXT:    s_waitcnt_vscnt null, 0x0
-; GFX11-SAFE-NEXT:    s_movk_i32 s0, 0x4000
-; GFX11-SAFE-NEXT:    s_delay_alu instid0(SALU_CYCLE_1) | instskip(NEXT) | instid1(VALU_DEP_1)
-; GFX11-SAFE-NEXT:    v_mov_b32_e32 v1, s0
-; GFX11-SAFE-NEXT:    v_fmac_f16_e32 v1, v3, v2
+; GFX11-SAFE-NEXT:    v_fma_f16 v1, v2, v3, 2.0
 ; GFX11-SAFE-NEXT:    s_delay_alu instid0(VALU_DEP_1)
 ; GFX11-SAFE-NEXT:    v_mul_f16_e64 v0, -v1, v4
 ; GFX11-SAFE-NEXT:    v_mul_f16_e64 v1, -v1, v5
 ; GFX11-SAFE-NEXT:    s_setpc_b64 s[30:31]
 ;
 ; GFX11-NSZ-LABEL: free_fold_src_code_size_cost_use_f16:
 ; GFX11-NSZ:       ; %bb.0:
 ; GFX11-NSZ-NEXT:    s_waitcnt vmcnt(0) expcnt(0) lgkmcnt(0)

llvm/test/CodeGen/ARM/unsafe-fneg-select-minnum-maxnum-combine.ll

 }
 
 define float @select_fsub0_or_8_cmp_olt_fsub1_neg8_f32(float %a, float %b) #0 {
 ; CHECK-LABEL: select_fsub0_or_8_cmp_olt_fsub1_neg8_f32:
 ; CHECK:       @ %bb.0:
 ; CHECK-NEXT:    vmov.f32 s0, #4.000000e+00
 ; CHECK-NEXT:    vmov s2, r0
 ; CHECK-NEXT:    vmov.f32 s4, #-8.000000e+00
-; CHECK-NEXT:    vmov.f32 s8, #8.000000e+00
-; CHECK-NEXT:    vsub.f32 s6, s0, s2
-; CHECK-NEXT:    vsub.f32 s0, s2, s0
-; CHECK-NEXT:    vcmp.f32 s4, s6
-; CHECK-NEXT:    vmrs APSR_nzcv, fpscr
-; CHECK-NEXT:    vselgt.f32 s0, s0, s8
+; CHECK-NEXT:    vsub.f32 s0, s0, s2
+; CHECK-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    eor r0, r0, #-2147483648
 ; CHECK-NEXT:    mov pc, lr
   %sub.0 = fsub nnan nsz float 4.0, %a
   %sub.1 = fsub nnan nsz float %a, 4.0
   %cmp.a = fcmp nnan nsz olt float %sub.0, -8.0
   %min.a = select nnan nsz i1 %cmp.a, float %sub.1, float 8.0
   ret float %min.a
 }
 
 define float @select_fsub0_or_neg8_cmp_olt_fsub1_8_f32(float %a, float %b) #0 {
 ; CHECK-LABEL: select_fsub0_or_neg8_cmp_olt_fsub1_8_f32:
 ; CHECK:       @ %bb.0:
 ; CHECK-NEXT:    vmov.f32 s0, #4.000000e+00
 ; CHECK-NEXT:    vmov s2, r0
 ; CHECK-NEXT:    vmov.f32 s4, #8.000000e+00
-; CHECK-NEXT:    vmov.f32 s8, #-8.000000e+00
-; CHECK-NEXT:    vsub.f32 s6, s0, s2
-; CHECK-NEXT:    vsub.f32 s0, s2, s0
-; CHECK-NEXT:    vcmp.f32 s4, s6
-; CHECK-NEXT:    vmrs APSR_nzcv, fpscr
-; CHECK-NEXT:    vselgt.f32 s0, s0, s8
+; CHECK-NEXT:    vsub.f32 s0, s0, s2
+; CHECK-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    eor r0, r0, #-2147483648
 ; CHECK-NEXT:    mov pc, lr
   %sub.0 = fsub nnan nsz float 4.0, %a
   %sub.1 = fsub nnan nsz float %a, 4.0
   %cmp.a = fcmp nnan nsz olt float %sub.0, 8.0
   %min.a = select nnan nsz i1 %cmp.a, float %sub.1, float -8.0
   ret float %min.a
 }
 
 define float @select_mul4_or_neg8_cmp_olt_mulneg4_8_f32(float %a, float %b) #0 {
 ; CHECK-LABEL: select_mul4_or_neg8_cmp_olt_mulneg4_8_f32:
 ; CHECK:       @ %bb.0:
 ; CHECK-NEXT:    vmov.f32 s0, #-4.000000e+00
 ; CHECK-NEXT:    vmov s2, r0
-; CHECK-NEXT:    vmov.f32 s6, #8.000000e+00
-; CHECK-NEXT:    vmov.f32 s4, #4.000000e+00
-; CHECK-NEXT:    vmov.f32 s8, #-8.000000e+00
+; CHECK-NEXT:    vmov.f32 s4, #8.000000e+00
 ; CHECK-NEXT:    vmul.f32 s0, s2, s0
-; CHECK-NEXT:    vmul.f32 s2, s2, s4
-; CHECK-NEXT:    vcmp.f32 s6, s0
-; CHECK-NEXT:    vmrs APSR_nzcv, fpscr
-; CHECK-NEXT:    vselgt.f32 s0, s2, s8
+; CHECK-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    eor r0, r0, #-2147483648
 ; CHECK-NEXT:    mov pc, lr
   %mul.0 = fmul nnan nsz float %a, 4.0
   %mul.1 = fmul nnan nsz float %a, -4.0
   %cmp.a = fcmp nnan nsz olt float %mul.1, 8.0
   %min.a = select nnan nsz i1 %cmp.a, float %mul.0, float -8.0
   ret float %min.a
 }
 
 define float @select_mul4_or_8_cmp_olt_mulneg4_neg8_f32(float %a, float %b) #0 {
 ; CHECK-LABEL: select_mul4_or_8_cmp_olt_mulneg4_neg8_f32:
 ; CHECK:       @ %bb.0:
 ; CHECK-NEXT:    vmov.f32 s0, #-4.000000e+00
 ; CHECK-NEXT:    vmov s2, r0
-; CHECK-NEXT:    vmov.f32 s6, #-8.000000e+00
-; CHECK-NEXT:    vmov.f32 s4, #4.000000e+00
-; CHECK-NEXT:    vmov.f32 s8, #8.000000e+00
+; CHECK-NEXT:    vmov.f32 s4, #-8.000000e+00
 ; CHECK-NEXT:    vmul.f32 s0, s2, s0
-; CHECK-NEXT:    vmul.f32 s2, s2, s4
-; CHECK-NEXT:    vcmp.f32 s6, s0
-; CHECK-NEXT:    vmrs APSR_nzcv, fpscr
-; CHECK-NEXT:    vselgt.f32 s0, s2, s8
+; CHECK-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    eor r0, r0, #-2147483648
 ; CHECK-NEXT:    mov pc, lr
   %mul.0 = fmul nnan nsz float %a, 4.0
   %mul.1 = fmul nnan nsz float %a, -4.0
   %cmp.a = fcmp nnan nsz olt float %mul.1, -8.0
   %min.a = select nnan nsz i1 %cmp.a, float %mul.0, float 8.0
   ret float %min.a
 }
 
; CHECK-NEXT:    mov pc, lr
   ret float %min.a
 }
 
 define float @select_mulneg4_or_neg8_cmp_olt_mul4_8_f32(float %a, float %b) #0 {
 ; CHECK-LABEL: select_mulneg4_or_neg8_cmp_olt_mul4_8_f32:
 ; CHECK:       @ %bb.0:
 ; CHECK-NEXT:    vmov.f32 s0, #4.000000e+00
 ; CHECK-NEXT:    vmov s2, r0
-; CHECK-NEXT:    vmov.f32 s6, #8.000000e+00
-; CHECK-NEXT:    vmov.f32 s4, #-4.000000e+00
-; CHECK-NEXT:    vmov.f32 s8, #-8.000000e+00
+; CHECK-NEXT:    vmov.f32 s4, #8.000000e+00
 ; CHECK-NEXT:    vmul.f32 s0, s2, s0
-; CHECK-NEXT:    vmul.f32 s2, s2, s4
-; CHECK-NEXT:    vcmp.f32 s6, s0
-; CHECK-NEXT:    vmrs APSR_nzcv, fpscr
-; CHECK-NEXT:    vselgt.f32 s0, s2, s8
+; CHECK-NEXT:    vminnm.f32 s0, s0, s4
 ; CHECK-NEXT:    vmov r0, s0
+; CHECK-NEXT:    eor r0, r0, #-2147483648
 ; CHECK-NEXT:    mov pc, lr
   %mul.0 = fmul nnan nsz float %a, -4.0
   %mul.1 = fmul nnan nsz float %a, 4.0
   %cmp.a = fcmp nnan nsz olt float %mul.1, 8.0
   %min.a = select nnan nsz i1 %cmp.a, float %mul.0, float -8.0
   ret float %min.a
 }
 
 ; FIXME: Should be unnecessary
 attributes #0 = { "no-signed-zeros-fp-math"="true" }