AMDGPU: Try to unfold fneg source when matching legacy fmin/fmax

[arsenm]

Summary

This is NFC as it stands, since other combines will effectively
prevent this from being reachable. This will avoid regressions in a
future change which tries to make better use of select source
modifiers.

Didn't bother with the GlobalISel part for now, since the baseline
combine doesn't seem to work on the existing test.

This is the target specific partner to D140128

[arsenm]

Rebase on some expanded test coverage

[foad]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

1416

This function doesn't use False. Might be clearer to take a Swapped flag instead of True and False.

1502

It's very unclear what you're trying to match here. I think it's something like this, depending on whether NegTrue actually stripped a neg or not:

// LHS op RHS ? LHS : -RHS -> -min/max(LHS, RHS)
// LHS op RHS ? -LHS : -RHS -> -min/max(LHS, RHS)

The first one doesn't make any sense to me. For the second one don't you need to flip the condition code?

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

295–297

I think these three lines implement roughly max(s0, 0) so how can converting it to min(0, s0) be correct?

[arsenm]

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

295–297

It's ngt, not gt. It's more like !(v_cmp_le_f32)

[foad]

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

295–297

Right, the code is doing s0<=0?v0:v1 which is s0<=0?-0:s0 which is max.

[foad]

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

295–297

Oh sorry I think I've misremembered how cndmask works.

[arsenm]

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

295–297

Right, cndmask is backward from how you would expect

[arsenm]

ping

[foad]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

1502

I still don't understand this

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

1502

fmin/fmax legacy are not commutative so there's no swapping here. Your sample here missed the part where the constant was negated, this is just pulling a negate out of the two select operands

[arsenm]

Add comment

[bogner]

lgtm

[arsenm]

36cfe26a5288d99e66c75d82989d154874999b98

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

1416

I think it's harder to read if you're considering the caller context

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h

SDValue LowerDYNAMIC_STACKALLOC(SDValue Op,
                                   SelectionDAG &DAG) const;
 
   SDValue LowerOperation(SDValue Op, SelectionDAG &DAG) const override;
   SDValue PerformDAGCombine(SDNode *N, DAGCombinerInfo &DCI) const override;
   void ReplaceNodeResults(SDNode * N,
                           SmallVectorImpl<SDValue> &Results,
                           SelectionDAG &DAG) const override;
 
+  SDValue combineFMinMaxLegacyImpl(const SDLoc &DL, EVT VT, SDValue LHS,
+                                   SDValue RHS, SDValue True, SDValue False,
+                                   SDValue CC, DAGCombinerInfo &DCI) const;
+
   SDValue combineFMinMaxLegacy(const SDLoc &DL, EVT VT, SDValue LHS,
                                SDValue RHS, SDValue True, SDValue False,
                                SDValue CC, DAGCombinerInfo &DCI) const;
 
   const char* getTargetNodeName(unsigned Opcode) const override;
 
   // FIXME: Turn off MergeConsecutiveStores() before Instruction Selection for
   // AMDGPU.  Commit r319036,

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

if (((SrcVT == MVT::v16f16 && VT == MVT::v8f16) ||
     return Op;
 
   DAG.ExtractVectorElements(Op.getOperand(0), Args, Start,
                             VT.getVectorNumElements());
 
   return DAG.getBuildVector(Op.getValueType(), SDLoc(Op), Args);
 }
 
-/// Generate Min/Max node
-SDValue AMDGPUTargetLowering::combineFMinMaxLegacy(const SDLoc &DL, EVT VT,
-                                                   SDValue LHS, SDValue RHS,
-                                                   SDValue True, SDValue False,
-                                                   SDValue CC,
-                                                   DAGCombinerInfo &DCI) const {
-  if (!(LHS == True && RHS == False) && !(LHS == False && RHS == True))
-    return SDValue();
-
+// TODO: Handle fabs too
+static SDValue peekFNeg(SDValue Val) {
+  if (Val.getOpcode() == ISD::FNEG)
+    return Val.getOperand(0);
+
+  return Val;
+}
+SDValue AMDGPUTargetLowering::combineFMinMaxLegacyImpl(
+    const SDLoc &DL, EVT VT, SDValue LHS, SDValue RHS, SDValue True,
+    SDValue False, SDValue CC, DAGCombinerInfo &DCI) const {
   SelectionDAG &DAG = DCI.DAG;
   ISD::CondCode CCOpcode = cast<CondCodeSDNode>(CC)->get();
   switch (CCOpcode) {
   case ISD::SETOEQ:
   case ISD::SETONE:
   case ISD::SETUNE:
   case ISD::SETNE:
   case ISD::SETUEQ:
case ISD::SETOGT: {
     return DAG.getNode(AMDGPUISD::FMIN_LEGACY, DL, VT, RHS, LHS);
   }
   case ISD::SETCC_INVALID:
     llvm_unreachable("Invalid setcc condcode!");
   }
   return SDValue();
 }
 
+/// Generate Min/Max node
+SDValue AMDGPUTargetLowering::combineFMinMaxLegacy(const SDLoc &DL, EVT VT,
+                                                   SDValue LHS, SDValue RHS,
+                                                   SDValue True, SDValue False,
+                                                   SDValue CC,
+                                                   DAGCombinerInfo &DCI) const {
+  if ((LHS == True && RHS == False) || (LHS == False && RHS == True))
+    return combineFMinMaxLegacyImpl(DL, VT, LHS, RHS, True, False, CC, DCI);
+
+  SelectionDAG &DAG = DCI.DAG;
+
+  // If we can't directly match this, try to see if we can fold an fneg to
+  // match.
+
+  ConstantFPSDNode *CRHS = dyn_cast<ConstantFPSDNode>(RHS);
+  ConstantFPSDNode *CFalse = dyn_cast<ConstantFPSDNode>(False);
+  SDValue NegTrue = peekFNeg(True);
+
+  // Undo the combine foldFreeOpFromSelect does if it helps us match the
+  // fmin/fmax.
+  //
+  // select (fcmp olt (lhs, K)), (fneg lhs), -K
+  // -> fneg (fmin_legacy lhs, K)
+  //
+  // TODO: Use getNegatedExpression
+  if (LHS == NegTrue && CFalse && CRHS) {
+    APFloat NegRHS = neg(CRHS->getValueAPF());
+    if (NegRHS == CFalse->getValueAPF()) {
+      SDValue Combined =
+          combineFMinMaxLegacyImpl(DL, VT, LHS, RHS, NegTrue, False, CC, DCI);
+      if (Combined)
+        return DAG.getNode(ISD::FNEG, DL, VT, Combined);
+      return SDValue();
+    }
+  }
+
+  return SDValue();
+}
+
 std::pair<SDValue, SDValue>
 AMDGPUTargetLowering::split64BitValue(SDValue Op, SelectionDAG &DAG) const {
   SDLoc SL(Op);
 
   SDValue Vec = DAG.getNode(ISD::BITCAST, SL, MVT::v2i32, Op);
 
   const SDValue Zero = DAG.getConstant(0, SL, MVT::i32);
   const SDValue One = DAG.getConstant(1, SL, MVT::i32);

llvm/lib/Target/AMDGPU/AMDGPUPostLegalizerCombiner.cpp

bool AMDGPUPostLegalizerCombinerHelper::matchFMinFMaxLegacy(
   if (!MRI.hasOneNonDBGUse(Cond) ||
       !mi_match(Cond, MRI,
                 m_GFCmp(m_Pred(Info.Pred), m_Reg(Info.LHS), m_Reg(Info.RHS))))
     return false;
 
   Info.True = MI.getOperand(2).getReg();
   Info.False = MI.getOperand(3).getReg();
 
+  // TODO: Handle case where the the selected value is an fneg and the compared
+  // constant is the negation of the selected value.
   if (!(Info.LHS == Info.True && Info.RHS == Info.False) &&
       !(Info.LHS == Info.False && Info.RHS == Info.True))
     return false;
 
   switch (Info.Pred) {
   case CmpInst::FCMP_FALSE:
   case CmpInst::FCMP_OEQ:
   case CmpInst::FCMP_ONE:

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

 
 ; This is a workaround because -enable-no-signed-zeros-fp-math does not set up
 ; function attribute unsafe-fp-math automatically. Combine with the previous test
 ; when that is done.
 define amdgpu_ps half @fneg_fadd_0_nsz_f16(half inreg %tmp2, half inreg %tmp6, <4 x i32> %arg) #2 {
 ; SI-SAFE-LABEL: fneg_fadd_0_nsz_f16:
 ; SI-SAFE:       ; %bb.0: ; %.entry
 ; SI-SAFE-NEXT:    v_cvt_f16_f32_e32 v0, s0
-; SI-SAFE-NEXT:    v_bfrev_b32_e32 v1, 1
-; SI-SAFE-NEXT:    v_mov_b32_e32 v2, 0x7fc00000
+; SI-SAFE-NEXT:    s_brev_b32 s0, 1
+; SI-SAFE-NEXT:    v_mov_b32_e32 v1, 0x7fc00000
 ; SI-SAFE-NEXT:    v_cvt_f32_f16_e32 v0, v0
-; SI-SAFE-NEXT:    v_cmp_nlt_f32_e32 vcc, 0, v0
-; SI-SAFE-NEXT:    v_cndmask_b32_e32 v0, v1, v0, vcc
-; SI-SAFE-NEXT:    v_cmp_nlt_f32_e32 vcc, 0, v0
-; SI-SAFE-NEXT:    v_cndmask_b32_e64 v0, v2, 0, vcc
+; SI-SAFE-NEXT:    v_min_legacy_f32_e32 v0, 0, v0
+; SI-SAFE-NEXT:    v_cmp_ngt_f32_e32 vcc, s0, v0
+; SI-SAFE-NEXT:    v_cndmask_b32_e64 v0, v1, 0, vcc
 ; SI-SAFE-NEXT:    ; return to shader part epilog
 ;
 ; SI-NSZ-LABEL: fneg_fadd_0_nsz_f16:
 ; SI-NSZ:       ; %bb.0: ; %.entry
 ; SI-NSZ-NEXT:    v_cvt_f16_f32_e32 v0, s1
 ; SI-NSZ-NEXT:    v_cvt_f16_f32_e32 v1, s0
 ; SI-NSZ-NEXT:    v_mov_b32_e32 v2, 0x7fc00000
 ; SI-NSZ-NEXT:    v_cvt_f32_f16_e32 v0, v0

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

 }
 
 ; This is a workaround because -enable-no-signed-zeros-fp-math does not set up
 ; function attribute unsafe-fp-math automatically. Combine with the previous test
 ; when that is done.
 define amdgpu_ps float @fneg_fadd_0_nsz_f32(float inreg %tmp2, float inreg %tmp6, <4 x i32> %arg) #2 {
 ; SI-SAFE-LABEL: fneg_fadd_0_nsz_f32:
 ; SI-SAFE:       ; %bb.0: ; %.entry
-; SI-SAFE-NEXT:    v_bfrev_b32_e32 v0, 1
-; SI-SAFE-NEXT:    v_mov_b32_e32 v1, s0
-; SI-SAFE-NEXT:    v_cmp_ngt_f32_e64 vcc, s0, 0
-; SI-SAFE-NEXT:    v_cndmask_b32_e32 v0, v0, v1, vcc
+; SI-SAFE-NEXT:    v_min_legacy_f32_e64 v0, 0, s0
+; SI-SAFE-NEXT:    s_brev_b32 s0, 1
 ; SI-SAFE-NEXT:    v_mov_b32_e32 v1, 0x7fc00000
-; SI-SAFE-NEXT:    v_cmp_nlt_f32_e32 vcc, 0, v0
+; SI-SAFE-NEXT:    v_cmp_ngt_f32_e32 vcc, s0, v0
 ; SI-SAFE-NEXT:    v_cndmask_b32_e64 v0, v1, 0, vcc
 ; SI-SAFE-NEXT:    ; return to shader part epilog
 ;
 ; GCN-NSZ-LABEL: fneg_fadd_0_nsz_f32:
 ; GCN-NSZ:       ; %bb.0: ; %.entry
 ; GCN-NSZ-NEXT:    v_rcp_f32_e32 v0, s1
 ; GCN-NSZ-NEXT:    v_mov_b32_e32 v1, s0
 ; GCN-NSZ-NEXT:    v_mul_f32_e32 v0, 0x80000000, v0