[DAGCombine] Check reassoc flags in aggressive fsub fusion

[jsji]

Summary

The is from discussion in https://reviews.llvm.org/D104247#inline-993387

The contract and reassoc flags shouldn't imply each other .

All the aggressive fsub fusion reassociate operations,
we should guard them with reassoc flag check.

[lebedev.ri]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13255–13260

[jsji]

Fix typo.

[lebedev.ri]

Patch description doesn't explain *why* we should do that.

[jsji]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13255–13260

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13258:25: error: no matching function for call to object of type '(lambda at llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13255:25)'
  bool CanReassociate = isReassociable(N);
                        ^~~~~~~~~~~~~~
llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp:13255:25: note: candidate function not viable: no known conversion from 'llvm::SDNode *' to 'llvm::SDValue' for 1st argument
  auto isReassociable = [Options](SDValue N) {
                        ^
1 error generated.
ninja: build stopped: subcommand failed.

N is SDNode, any suggestion to get SDValue of it? SDValue() might cause crash in tests.

[mcberg2017]

Let some of the others also comment, but on the surface it's what I would expect.

[lebedev.ri]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13255–13260

SDValue(N, 0)?

13255–13260

... or more generally, [why] does it have to take SDValue?

[jsji]

Address comments.

[jsji]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13255–13260

Good idea, thanks!

[lebedev.ri]

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

13259

foldable is a rather generic and widely used term.
I'd suggest isContractableAndReassociableFMUL()

[jsji]

Rename to isContractableAndReassociableFMUL.

[lebedev.ri]

SGTM i guess.

[dfukalov]

AMDGPU tests LGTM, thanks!

[foad]

llvm/test/CodeGen/AMDGPU/fpext-free.ll

312

I don't think this transform should require "reassoc" on this fmul, should it? %u * %v is calculated the same way before and after the transform. Maybe it should require "reassoc" on the call to fmuladd though, because the add does get reassociated with the fsub.

llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp

if (N00.getOpcode() == ISD::FP_EXTEND) {
             DAG.getNode(PreferredFusedOpcode, SL, VT,
                         DAG.getNode(ISD::FP_EXTEND, SL, VT, N000.getOperand(0)),
                         DAG.getNode(ISD::FP_EXTEND, SL, VT, N000.getOperand(1)),
                         N1));
       }
     }
   }
 
+  auto isReassociable = [Options](SDNode *N) {
+    return Options.UnsafeFPMath || N->getFlags().hasAllowReassociation();
+  };
+
+  auto isFoldableFMUL = [isContractableFMUL, isReassociable](SDValue N) {
+    return isContractableFMUL(N) && isReassociable(N.getNode());
+  };
+
   // More folding opportunities when target permits.
-  if (Aggressive) {
+  if (Aggressive && isReassociable(N)) {
     bool CanFuse = Options.UnsafeFPMath || N->getFlags().hasAllowContract();
     // fold (fsub (fma x, y, (fmul u, v)), z)
     //   -> (fma x, y (fma u, v, (fneg z)))
     if (CanFuse && N0.getOpcode() == PreferredFusedOpcode &&
-        isContractableFMUL(N0.getOperand(2)) && N0->hasOneUse() &&
+        isFoldableFMUL(N0.getOperand(2)) && N0->hasOneUse() &&
         N0.getOperand(2)->hasOneUse()) {
       return DAG.getNode(PreferredFusedOpcode, SL, VT, N0.getOperand(0),
                          N0.getOperand(1),
                          DAG.getNode(PreferredFusedOpcode, SL, VT,
                                      N0.getOperand(2).getOperand(0),
                                      N0.getOperand(2).getOperand(1),
                                      DAG.getNode(ISD::FNEG, SL, VT, N1)));
     }
 
     // fold (fsub x, (fma y, z, (fmul u, v)))
     //   -> (fma (fneg y), z, (fma (fneg u), v, x))
     if (CanFuse && N1.getOpcode() == PreferredFusedOpcode &&
-        isContractableFMUL(N1.getOperand(2)) &&
-        N1->hasOneUse() && NoSignedZero) {
+        isFoldableFMUL(N1.getOperand(2)) && N1->hasOneUse() && NoSignedZero) {
       SDValue N20 = N1.getOperand(2).getOperand(0);
       SDValue N21 = N1.getOperand(2).getOperand(1);
       return DAG.getNode(
           PreferredFusedOpcode, SL, VT,
           DAG.getNode(ISD::FNEG, SL, VT, N1.getOperand(0)), N1.getOperand(1),
           DAG.getNode(PreferredFusedOpcode, SL, VT,
                       DAG.getNode(ISD::FNEG, SL, VT, N20), N21, N0));
     }
 
-
     // fold (fsub (fma x, y, (fpext (fmul u, v))), z)
     //   -> (fma x, y (fma (fpext u), (fpext v), (fneg z)))
     if (N0.getOpcode() == PreferredFusedOpcode &&
         N0->hasOneUse()) {
       SDValue N02 = N0.getOperand(2);
       if (N02.getOpcode() == ISD::FP_EXTEND) {
         SDValue N020 = N02.getOperand(0);
-        if (isContractableFMUL(N020) &&
+        if (isFoldableFMUL(N020) &&
             TLI.isFPExtFoldable(DAG, PreferredFusedOpcode, VT,
                                 N020.getValueType())) {
           return DAG.getNode(
               PreferredFusedOpcode, SL, VT, N0.getOperand(0), N0.getOperand(1),
               DAG.getNode(
                   PreferredFusedOpcode, SL, VT,
                   DAG.getNode(ISD::FP_EXTEND, SL, VT, N020.getOperand(0)),
                   DAG.getNode(ISD::FP_EXTEND, SL, VT, N020.getOperand(1)),
                   DAG.getNode(ISD::FNEG, SL, VT, N1)));
         }
       }
     }
 
     // fold (fsub (fpext (fma x, y, (fmul u, v))), z)
     //   -> (fma (fpext x), (fpext y),
     //           (fma (fpext u), (fpext v), (fneg z)))
     // FIXME: This turns two single-precision and one double-precision
     // operation into two double-precision operations, which might not be
     // interesting for all targets, especially GPUs.
     if (N0.getOpcode() == ISD::FP_EXTEND) {
       SDValue N00 = N0.getOperand(0);
       if (N00.getOpcode() == PreferredFusedOpcode) {
         SDValue N002 = N00.getOperand(2);
-        if (isContractableFMUL(N002) &&
+        if (isFoldableFMUL(N002) &&
             TLI.isFPExtFoldable(DAG, PreferredFusedOpcode, VT,
                                 N00.getValueType())) {
           return DAG.getNode(
               PreferredFusedOpcode, SL, VT,
               DAG.getNode(ISD::FP_EXTEND, SL, VT, N00.getOperand(0)),
               DAG.getNode(ISD::FP_EXTEND, SL, VT, N00.getOperand(1)),
               DAG.getNode(
                   PreferredFusedOpcode, SL, VT,
                   DAG.getNode(ISD::FP_EXTEND, SL, VT, N002.getOperand(0)),
                   DAG.getNode(ISD::FP_EXTEND, SL, VT, N002.getOperand(1)),
                   DAG.getNode(ISD::FNEG, SL, VT, N1)));
         }
       }
     }
 
     // fold (fsub x, (fma y, z, (fpext (fmul u, v))))
     //   -> (fma (fneg y), z, (fma (fneg (fpext u)), (fpext v), x))
     if (N1.getOpcode() == PreferredFusedOpcode &&
         N1.getOperand(2).getOpcode() == ISD::FP_EXTEND &&
         N1->hasOneUse()) {
       SDValue N120 = N1.getOperand(2).getOperand(0);
-      if (isContractableFMUL(N120) &&
+      if (isFoldableFMUL(N120) &&
           TLI.isFPExtFoldable(DAG, PreferredFusedOpcode, VT,
                               N120.getValueType())) {
         SDValue N1200 = N120.getOperand(0);
         SDValue N1201 = N120.getOperand(1);
         return DAG.getNode(
             PreferredFusedOpcode, SL, VT,
             DAG.getNode(ISD::FNEG, SL, VT, N1.getOperand(0)), N1.getOperand(1),
             DAG.getNode(PreferredFusedOpcode, SL, VT,
if (Aggressive && isReassociable(N)) {
     // operation into two double-precision operations, which might not be
     // interesting for all targets, especially GPUs.
     if (N1.getOpcode() == ISD::FP_EXTEND &&
         N1.getOperand(0).getOpcode() == PreferredFusedOpcode) {
       SDValue CvtSrc = N1.getOperand(0);
       SDValue N100 = CvtSrc.getOperand(0);
       SDValue N101 = CvtSrc.getOperand(1);
       SDValue N102 = CvtSrc.getOperand(2);
-      if (isContractableFMUL(N102) &&
+      if (isFoldableFMUL(N102) &&
           TLI.isFPExtFoldable(DAG, PreferredFusedOpcode, VT,
                               CvtSrc.getValueType())) {
         SDValue N1020 = N102.getOperand(0);
         SDValue N1021 = N102.getOperand(1);
         return DAG.getNode(
             PreferredFusedOpcode, SL, VT,
             DAG.getNode(ISD::FNEG, SL, VT,
                         DAG.getNode(ISD::FP_EXTEND, SL, VT, N100)),

llvm/test/CodeGen/AMDGPU/fpext-free.ll

 
 ; GFX9-F32DENORM-NEXT: v_mul_f16_e32 v3, v3, v4
 ; GFX9-F32DENORM-NEXT: v_cvt_f32_f16_e32 v3, v3
 ; GFX9-F32DENORM-NEXT: v_fma_f32 v0, v0, v1, v3
 ; GFX9-F32DENORM-NEXT: v_sub_f32_e32 v0, v0, v2
 ; GFX9-F32DENORM-NEXT: s_setpc_b64
 define float @fsub_muladd_fpext_mul_f16_to_f32(float %x, float %y, float %z, half %u, half %v) #0 {
 entry:
-  %mul = fmul half %u, %v
+  %mul = fmul reassoc half %u, %v
   %mul.ext = fpext half %mul to float
   %fma = call float @llvm.fmuladd.f32(float %x, float %y, float %mul.ext)
-  %add = fsub float %fma, %z
+  %add = fsub reassoc float %fma, %z
   ret float %add
 }
 
 ;  fold (fsub (fpext (fmad x, y, (fmul u, v))), z)
 ;    -> (fmad (fpext x), (fpext y),
 ;            (fmad (fpext u), (fpext v), (fneg z)))
 
 ; GCN-LABEL: {{^}}fsub_fpext_muladd_mul_f16_to_f32:

 
 ; GFX9-F32DENORM-NEXT: v_mul_f16_e32 v3, v3, v4
 ; GFX9-F32DENORM-NEXT: v_cvt_f32_f16_e32 v3, v3
 ; GFX9-F32DENORM-NEXT: v_fma_f32 v1, v1, v2, v3
 ; GFX9-F32DENORM-NEXT: v_sub_f32_e32 v0, v0, v1
 ; GFX9-F32DENORM-NEXT: s_setpc_b64
 define float @fsub_muladd_fpext_mul_f16_to_f32_commute(float %x, float %y, float %z, half %u, half %v) #0 {
 entry:
-  %mul = fmul half %u, %v
+  %mul = fmul reassoc half %u, %v
   %mul.ext = fpext half %mul to float
   %fma = call float @llvm.fmuladd.f32(float %y, float %z, float %mul.ext)
-  %add = fsub float %x, %fma
+  %add = fsub reassoc float %x, %fma
   ret float %add
 }
 
 ; fold (fsub x, (fpext (fma y, z, (fmul u, v))))
 ;    -> (fma (fneg (fpext y)), (fpext z),
 ;            (fma (fneg (fpext u)), (fpext v), x))
 ; GCN-LABEL: {{^}}fsub_fpext_muladd_mul_f16_to_f32_commute:
 ; GCN: s_waitcnt

llvm/test/CodeGen/PowerPC/fma-assoc.ll

; CHECK-VSX-NEXT:    blr
   %I = fpext float %H to double   ; <double> [#uses=1]
   %J = fadd double %E, %I         ; <double> [#uses=1]
   ret double %J
 }
 
 define double @test_FMSUB_ASSOC_EXT1(float %A, float %B, double %C,
 ; CHECK-LABEL: test_FMSUB_ASSOC_EXT1:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fmsub 0, 1, 2, 5
-; CHECK-NEXT:    fmadd 1, 3, 4, 0
+; CHECK-NEXT:    fmuls 0, 1, 2
+; CHECK-NEXT:    fmadd 0, 3, 4, 0
+; CHECK-NEXT:    fsub 1, 0, 5
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_FMSUB_ASSOC_EXT1:
 ; CHECK-VSX:       # %bb.0:
-; CHECK-VSX-NEXT:    xsmsubmdp 1, 2, 5
-; CHECK-VSX-NEXT:    xsmaddadp 1, 3, 4
+; CHECK-VSX-NEXT:    fmuls 0, 1, 2
+; CHECK-VSX-NEXT:    xsmaddadp 0, 3, 4
+; CHECK-VSX-NEXT:    xssubdp 1, 0, 5
 ; CHECK-VSX-NEXT:    blr
                                  double %D, double %E) {
   %F = fmul float %A, %B          ; <float> [#uses=1]
   %G = fpext float %F to double   ; <double> [#uses=1]
   %H = fmul double %C, %D         ; <double> [#uses=1]
   %I = fadd double %H, %G         ; <double> [#uses=1]
   %J = fsub double %I, %E         ; <double> [#uses=1]
   ret double %J
 }
 
 define double @test_FMSUB_ASSOC_EXT2(float %A, float %B, float %C,
 ; CHECK-LABEL: test_FMSUB_ASSOC_EXT2:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fmsub 0, 3, 4, 5
-; CHECK-NEXT:    fmadd 1, 1, 2, 0
+; CHECK-NEXT:    fmuls 0, 3, 4
+; CHECK-NEXT:    fmadds 0, 1, 2, 0
+; CHECK-NEXT:    fsub 1, 0, 5
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_FMSUB_ASSOC_EXT2:
 ; CHECK-VSX:       # %bb.0:
-; CHECK-VSX-NEXT:    xsmsubmdp 3, 4, 5
-; CHECK-VSX-NEXT:    xsmaddadp 3, 1, 2
-; CHECK-VSX-NEXT:    fmr 1, 3
+; CHECK-VSX-NEXT:    fmuls 0, 3, 4
+; CHECK-VSX-NEXT:    fmadds 0, 1, 2, 0
+; CHECK-VSX-NEXT:    xssubdp 1, 0, 5
 ; CHECK-VSX-NEXT:    blr
                                  float %D, double %E) {
   %F = fmul float %A, %B          ; <float> [#uses=1]
   %G = fmul float %C, %D          ; <float> [#uses=1]
   %H = fadd float %F, %G          ; <float> [#uses=1]
   %I = fpext float %H to double   ; <double> [#uses=1]
   %J = fsub double %I, %E         ; <double> [#uses=1]
   ret double %J
 }
 
 define double @test_FMSUB_ASSOC_EXT3(float %A, float %B, double %C,
 ; CHECK-LABEL: test_FMSUB_ASSOC_EXT3:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fneg 0, 1
-; CHECK-NEXT:    fmadd 0, 0, 2, 5
-; CHECK-NEXT:    fneg 1, 3
-; CHECK-NEXT:    fmadd 1, 1, 4, 0
+; CHECK-NEXT:    fmuls 0, 1, 2
+; CHECK-NEXT:    fmadd 0, 3, 4, 0
+; CHECK-NEXT:    fsub 1, 5, 0
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_FMSUB_ASSOC_EXT3:
 ; CHECK-VSX:       # %bb.0:
-; CHECK-VSX-NEXT:    xsnegdp 1, 1
-; CHECK-VSX-NEXT:    xsnegdp 0, 3
-; CHECK-VSX-NEXT:    xsmaddmdp 1, 2, 5
-; CHECK-VSX-NEXT:    xsmaddadp 1, 0, 4
+; CHECK-VSX-NEXT:    fmuls 0, 1, 2
+; CHECK-VSX-NEXT:    xsmaddadp 0, 3, 4
+; CHECK-VSX-NEXT:    xssubdp 1, 5, 0
 ; CHECK-VSX-NEXT:    blr
                                  double %D, double %E) {
   %F = fmul float %A, %B          ; <float> [#uses=1]
   %G = fpext float %F to double   ; <double> [#uses=1]
   %H = fmul double %C, %D         ; <double> [#uses=1]
   %I = fadd double %H, %G         ; <double> [#uses=1]
   %J = fsub double %E, %I         ; <double> [#uses=1]
   ret double %J
 }
 
 define double @test_FMSUB_ASSOC_EXT4(float %A, float %B, float %C,
 ; CHECK-LABEL: test_FMSUB_ASSOC_EXT4:
 ; CHECK:       # %bb.0:
-; CHECK-NEXT:    fneg 0, 3
-; CHECK-NEXT:    fmadd 0, 0, 4, 5
-; CHECK-NEXT:    fneg 1, 1
-; CHECK-NEXT:    fmadd 1, 1, 2, 0
+; CHECK-NEXT:    fmuls 0, 3, 4
+; CHECK-NEXT:    fmadds 0, 1, 2, 0
+; CHECK-NEXT:    fsub 1, 5, 0
 ; CHECK-NEXT:    blr
 ;
 ; CHECK-VSX-LABEL: test_FMSUB_ASSOC_EXT4:
 ; CHECK-VSX:       # %bb.0:
-; CHECK-VSX-NEXT:    xsnegdp 0, 3
-; CHECK-VSX-NEXT:    xsnegdp 1, 1
-; CHECK-VSX-NEXT:    xsmaddmdp 0, 4, 5
-; CHECK-VSX-NEXT:    xsmaddadp 0, 1, 2
-; CHECK-VSX-NEXT:    fmr 1, 0
+; CHECK-VSX-NEXT:    fmuls 0, 3, 4
+; CHECK-VSX-NEXT:    fmadds 0, 1, 2, 0
+; CHECK-VSX-NEXT:    xssubdp 1, 5, 0
 ; CHECK-VSX-NEXT:    blr
                                  float %D, double %E) {
   %F = fmul float %A, %B          ; <float> [#uses=1]
   %G = fmul float %C, %D          ; <float> [#uses=1]
   %H = fadd float %F, %G          ; <float> [#uses=1]
   %I = fpext float %H to double   ; <double> [#uses=1]
   %J = fsub double %E, %I         ; <double> [#uses=1]
   ret double %J