[AMDGPU] Fix infinite loop with fma combines

[kerbowa]

Summary

RFC since I'm not very familiar with the DAG
combiner.

https://reviews.llvm.org/D72312 introduced an infinite loop which involves
DAGCombiner::visitFMA and AMDGPUTargetLowering::performFNegCombine.

fma( a, fneg(b), fneg(c) ) => fneg( fma (a, b, c) ) => fma( a, fneg(b), fneg(c) ) ...

This only breaks with types where 'isFNegFree' returns flase, e.g. v4f32.
Reproducing the issue also needs the attribute 'no-signed-zeros-fp-math',
and no source mods allowed on one of the users of the Op.

This fix makes changes to indicate that it is not free to negate a fma if it
has users with source mods.

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

737–740

My initial thought is it should not be necessary to override this

742

This is incomplete and FMAD will have the same problem at minimum

[kerbowa]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

737–740

I had the same initial thought.

The issue is that performFNegCombine is doing the right thing. And the generic fma combine show below relies on 'isNegatibleForFree'. In this case it's not actually profitable.

if (!TLI.isFNegFree(VT) &&
+ TLI.isNegatibleForFree(SDValue(N, 0), DAG, LegalOperations,
+ ForCodeSize) == 2)

742

The hang at least doesn't effect FMAD. The combine that triggers this wont happen.

[rampitec]

LGTM

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

742

Where is the combine? It's also suspicious if an fneg combine triggered for FMA but not FMAD.

llvm/test/CodeGen/AMDGPU/fma-combine.ll

699

should also have an fmad case

[kerbowa]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

742

It's a loop so there are combines in two directions.

The FNeg combine -fma(a, b, c) => fma(a, -b, -c), triggers for FMA/FMAD and is correct.

AFAICS the unprofitable combine, fma(a, -b, -c) => -fma(a, b, c), only triggers for FMA.
https://github.com/llvm/llvm-project/blob/master/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp#L12652

[arsenm]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

742

This should be just as valid for fmad, but we probably don't want it. This should handle fmad as well.

I am somewhat concerned about splitting much of the same logic between isNegatibleForFree and the target specific fneg combines. Is there now more overlap in what happens in the AMDGPU performFNegCombine and DAGCombiner than there used to be?

[kerbowa]

Handle fmad.

[kerbowa]

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

742

It might just be this one combine that was added. I agree with you though that there is danger that the logic becomes unnecessarily split, but the only other short term option is reverting https://reviews.llvm.org/D72312.

I know not many targets are overriding isNegatibleForFree. In this case, the generic rule is that if fnegs are not free, then two is worse then one. I.e. -fma(a, b, c) is better than fma(a, -b, -c). It does seem like we need target specific logic here.

[arsenm]

LGTM to unblock this, but we do need to consolidate this logic. Either all of the AMDGPU fneg combines should be moved to generic code, or at least the AMDGPU code should start using isNegatibleForFree

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.h

public:
   bool isFNegFree(EVT VT) const override;
   bool isTruncateFree(EVT Src, EVT Dest) const override;
   bool isTruncateFree(Type *Src, Type *Dest) const override;
 
   bool isZExtFree(Type *Src, Type *Dest) const override;
   bool isZExtFree(EVT Src, EVT Dest) const override;
   bool isZExtFree(SDValue Val, EVT VT2) const override;
 
+  char isNegatibleForFree(SDValue Op, SelectionDAG &DAG, bool LegalOperations,
+                          bool ForCodeSize, unsigned Depth) const override;
+
   bool isNarrowingProfitable(EVT VT1, EVT VT2) const override;
 
   MVT getVectorIdxTy(const DataLayout &) const override;
   bool isSelectSupported(SelectSupportKind) const override;
 
   bool isFPImmLegal(const APFloat &Imm, EVT VT,
                     bool ForCodeSize) const override;
   bool ShouldShrinkFPConstant(EVT VT) const override;

llvm/lib/Target/AMDGPU/AMDGPUISelLowering.cpp

case ISD::LOAD:
           AMDGPUAS::CONSTANT_ADDRESS_32BIT)
         return true;
       return false;
     }
     break;
   }
 }
 
+char AMDGPUTargetLowering::isNegatibleForFree(SDValue Op, SelectionDAG &DAG,
+                                              bool LegalOperations,
+                                              bool ForCodeSize,
+                                              unsigned Depth) const {
+  switch (Op.getOpcode()) {
+    case ISD::FMA:
+    case ISD::FMAD: {
+      // Negating a fma is not free if it has users without source mods.
+      if (!allUsesHaveSourceMods(Op.getNode()))
+        return 0;
+      break;
+    }
+    default:
+      break;
+  }
+
+  return TargetLowering::isNegatibleForFree(Op, DAG, LegalOperations,
+                                            ForCodeSize, Depth);
+}
+
 //===---------------------------------------------------------------------===//
 // Target Properties
 //===---------------------------------------------------------------------===//
 
 bool AMDGPUTargetLowering::isFAbsFree(EVT VT) const {
   assert(VT.isFloatingPoint());
 
   // Packed operations do not have a fabs modifier.

llvm/test/CodeGen/AMDGPU/fma-combine.ll

 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=tahiti -verify-machineinstrs -fp-contract=fast < %s | FileCheck -enable-var-scope -check-prefix=SI-NOFMA -check-prefix=SI-SAFE -check-prefix=SI -check-prefix=FUNC %s
 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=verde -verify-machineinstrs -fp-contract=fast < %s | FileCheck -enable-var-scope -check-prefix=SI-NOFMA -check-prefix=SI-SAFE -check-prefix=SI -check-prefix=FUNC %s
 ; RUN:  llc -amdgpu-scalarize-global-loads=false  -march=amdgcn -mcpu=tahiti -verify-machineinstrs -fp-contract=fast -enable-no-infs-fp-math -enable-unsafe-fp-math -mattr=+fp32-denormals < %s | FileCheck -enable-var-scope -check-prefix=SI-FMA -check-prefix=SI-UNSAFE -check-prefix=SI -check-prefix=FUNC %s
 
 ; Note: The SI-FMA conversions of type x * (y + 1) --> x * y + x would be
 ; beneficial even without fp32 denormals, but they do require no-infs-fp-math
 ; for correctness.
 
 declare i32 @llvm.amdgcn.workitem.id.x() #0
 declare double @llvm.fabs.f64(double) #0
 declare double @llvm.fma.f64(double, double, double) #0
 declare float @llvm.fma.f32(float, float, float) #0
+declare <4 x float> @llvm.fma.v4f32(<4 x float>, <4 x float>, <4 x float>) #0
 
 ; (fadd (fmul x, y), z) -> (fma x, y, z)
 ; FUNC-LABEL: {{^}}combine_to_fma_f64_0:
 ; SI-DAG: buffer_load_dwordx2 [[A:v\[[0-9]+:[0-9]+\]]], v{{\[[0-9]+:[0-9]+\]}}, s{{\[[0-9]+:[0-9]+\]}}, 0 addr64{{$}}
 ; SI-DAG: buffer_load_dwordx2 [[B:v\[[0-9]+:[0-9]+\]]], v{{\[[0-9]+:[0-9]+\]}}, s{{\[[0-9]+:[0-9]+\]}}, 0 addr64 offset:8{{$}}
 ; SI-DAG: buffer_load_dwordx2 [[C:v\[[0-9]+:[0-9]+\]]], v{{\[[0-9]+:[0-9]+\]}}, s{{\[[0-9]+:[0-9]+\]}}, 0 addr64 offset:16{{$}}
 ; SI: v_fma_f64 [[RESULT:v\[[0-9]+:[0-9]+\]]], [[A]], [[B]], [[C]]
 ; SI: buffer_store_dwordx2 [[RESULT]]
define amdgpu_kernel void @test_f64_interp(double addrspace(1)* %out,
   %tx = fmul double %x, %t
   %ty = fmul double %y, %t1
   %r = fadd double %tx, %ty
   store double %r, double addrspace(1)* %out
   ret void
 }
 
 ; Make sure negative constant cancels out fneg
-; GCN-LABEL: {{^}}fma_neg_2.0_neg_a_b_f32:
-; GCN: {{buffer|flat|global}}_load_dword [[A:v[0-9]+]]
-; GCN: {{buffer|flat|global}}_load_dword [[B:v[0-9]+]]
-; GCN-NOT: [[A]]
-; GCN-NOT: [[B]]
-; GCN: v_fma_f32 v{{[0-9]+}}, [[A]], 2.0, [[B]]
+; SI-LABEL: {{^}}fma_neg_2.0_neg_a_b_f32:
+; SI: {{buffer|flat|global}}_load_dword [[A:v[0-9]+]]
+; SI: {{buffer|flat|global}}_load_dword [[B:v[0-9]+]]
+; SI-NOT: [[A]]
+; SI-NOT: [[B]]
+; SI: v_fma_f32 v{{[0-9]+}}, [[A]], 2.0, [[B]]
 define amdgpu_kernel void @fma_neg_2.0_neg_a_b_f32(float addrspace(1)* %out, float addrspace(1)* %in) #0 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x()
   %gep.0 = getelementptr float, float addrspace(1)* %out, i32 %tid
   %gep.1 = getelementptr float, float addrspace(1)* %gep.0, i32 1
   %gep.out = getelementptr float, float addrspace(1)* %out, i32 %tid
 
   %r1 = load volatile float, float addrspace(1)* %gep.0
   %r2 = load volatile float, float addrspace(1)* %gep.1
 
   %r1.fneg = fsub float -0.000000e+00, %r1
 
   %r3 = tail call float @llvm.fma.f32(float -2.0, float %r1.fneg, float %r2)
   store float %r3, float addrspace(1)* %gep.out
   ret void
 }
 
-; GCN-LABEL: {{^}}fma_2.0_neg_a_b_f32:
-; GCN: {{buffer|flat|global}}_load_dword [[A:v[0-9]+]]
-; GCN: {{buffer|flat|global}}_load_dword [[B:v[0-9]+]]
-; GCN-NOT: [[A]]
-; GCN-NOT: [[B]]
-; GCN: v_fma_f32 v{{[0-9]+}}, [[A]], -2.0, [[B]]
+; SI-LABEL: {{^}}fma_2.0_neg_a_b_f32:
+; SI: {{buffer|flat|global}}_load_dword [[A:v[0-9]+]]
+; SI: {{buffer|flat|global}}_load_dword [[B:v[0-9]+]]
+; SI-NOT: [[A]]
+; SI-NOT: [[B]]
+; SI: v_fma_f32 v{{[0-9]+}}, [[A]], -2.0, [[B]]
 define amdgpu_kernel void @fma_2.0_neg_a_b_f32(float addrspace(1)* %out, float addrspace(1)* %in) #0 {
   %tid = call i32 @llvm.amdgcn.workitem.id.x()
   %gep.0 = getelementptr float, float addrspace(1)* %out, i32 %tid
   %gep.1 = getelementptr float, float addrspace(1)* %gep.0, i32 1
   %gep.out = getelementptr float, float addrspace(1)* %out, i32 %tid
 
   %r1 = load volatile float, float addrspace(1)* %gep.0
   %r2 = load volatile float, float addrspace(1)* %gep.1
 
   %r1.fneg = fsub float -0.000000e+00, %r1
 
   %r3 = tail call float @llvm.fma.f32(float 2.0, float %r1.fneg, float %r2)
   store float %r3, float addrspace(1)* %gep.out
   ret void
 }
 
+; SI-LABEL: {{^}}fma_neg_b_c_v4f32:
+; SI: v_fma_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; SI: v_fma_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; SI: v_fma_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; SI: v_fma_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+define amdgpu_kernel void @fma_neg_b_c_v4f32(<4 x float> addrspace(1)* %out, <4 x float> addrspace(1)* %in) #2 {
+  %tid = call i32 @llvm.amdgcn.workitem.id.x()
+  %gep.0 = getelementptr <4 x float>, <4 x float> addrspace(1)* %in, i32 %tid
+  %gep.1 = getelementptr <4 x float>, <4 x float> addrspace(1)* %gep.0, i32 1
+  %gep.2 = getelementptr <4 x float>, <4 x float> addrspace(1)* %gep.1, i32 2
+  %gep.out = getelementptr <4 x float>, <4 x float> addrspace(1)* %out, i32 %tid
+
+  %tmp0 = load <4 x float>, <4 x float> addrspace(1)* %gep.0
+  %tmp1 = load <4 x float>, <4 x float> addrspace(1)* %gep.1
+  %tmp2 = load <4 x float>, <4 x float> addrspace(1)* %gep.2
+
+  %fneg0 = fneg fast <4 x float> %tmp0
+  %fneg1 = fneg fast <4 x float> %tmp1
+  %fma0 = tail call fast <4 x float> @llvm.fma.v4f32(<4 x float> %tmp2, <4 x float> %fneg0, <4 x float> %fneg1)
+
+  store <4 x float> %fma0, <4 x float> addrspace(1)* %gep.out
+  ret void
+}
+
 attributes #0 = { nounwind readnone }
 attributes #1 = { nounwind }
-
+attributes #2 = { nounwind "no-signed-zeros-fp-math"="true" }

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

define amdgpu_kernel void @v_fneg_fmad_f32(float addrspace(1)* %out, float addrspace(1)* %a.ptr, float addrspace(1)* %b.ptr, float addrspace(1)* %c.ptr) #0 {
   %b = load volatile float, float addrspace(1)* %b.gep
   %c = load volatile float, float addrspace(1)* %c.gep
   %fma = call float @llvm.fmuladd.f32(float %a, float %b, float %c)
   %fneg = fsub float -0.000000e+00, %fma
   store float %fneg, float addrspace(1)* %out.gep
   ret void
 }
 
+; GCN-LABEL: {{^}}v_fneg_fmad_v4f32:
+
+; GCN-NSZ: v_mad_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; GCN-NSZ: v_mad_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; GCN-NSZ: v_mad_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+; GCN-NSZ: v_mad_f32 v{{[0-9]+}}, v{{[0-9]+}}, -v{{[0-9]+}}, -v{{[0-9]+}}
+define amdgpu_kernel void @v_fneg_fmad_v4f32(<4 x float> addrspace(1)* %out, <4 x float> addrspace(1)* %a.ptr, <4 x float> addrspace(1)* %b.ptr, <4 x float> addrspace(1)* %c.ptr) #0 {
+  %tid = call i32 @llvm.amdgcn.workitem.id.x()
+  %tid.ext = sext i32 %tid to i64
+  %a.gep = getelementptr inbounds <4 x float>, <4 x float> addrspace(1)* %a.ptr, i64 %tid.ext
+  %b.gep = getelementptr inbounds <4 x float>, <4 x float> addrspace(1)* %b.ptr, i64 %tid.ext
+  %c.gep = getelementptr inbounds <4 x float>, <4 x float> addrspace(1)* %c.ptr, i64 %tid.ext
+  %out.gep = getelementptr inbounds <4 x float>, <4 x float> addrspace(1)* %out, i64 %tid.ext
+  %a = load volatile <4 x float>, <4 x float> addrspace(1)* %a.gep
+  %b = load volatile <4 x float>, <4 x float> addrspace(1)* %b.gep
+  %c = load volatile <4 x float>, <4 x float> addrspace(1)* %c.gep
+  %fma = call <4 x float> @llvm.fmuladd.v4f32(<4 x float> %a, <4 x float> %b, <4 x float> %c)
+  %fneg = fneg <4 x float> %fma
+  store <4 x float> %fneg, <4 x float> addrspace(1)* %out.gep
+  ret void
+}
+
 ; GCN-LABEL: {{^}}v_fneg_fmad_multi_use_fmad_f32:
 ; GCN: {{buffer|flat}}_load_dword [[A:v[0-9]+]]
 ; GCN: {{buffer|flat}}_load_dword [[B:v[0-9]+]]
 ; GCN: {{buffer|flat}}_load_dword [[C:v[0-9]+]]
 
 ; GCN-SAFE: v_mac_f32_e32 [[C]], [[A]], [[B]]
 ; GCN-SAFE: v_xor_b32_e32 [[NEG_MAD:v[0-9]+]], 0x80000000, [[C]]
 ; GCN-SAFE-NEXT: v_mul_f32_e32 [[MUL:v[0-9]+]], 4.0, [[C]]
define amdgpu_kernel void @multi_use_cost_to_fold_into_src(float addrspace(1)* %out, float addrspace(1)* %a.ptr, float addrspace(1)* %b.ptr, float addrspace(1)* %c.ptr, float addrspace(1)* %d.ptr) #0 {
   store volatile float %fma0, float addrspace(1)* %out
   store volatile float %mul1, float addrspace(1)* %out
   ret void
 }
 
 declare i32 @llvm.amdgcn.workitem.id.x() #1
 declare float @llvm.fma.f32(float, float, float) #1
 declare float @llvm.fmuladd.f32(float, float, float) #1
+declare <4 x float> @llvm.fmuladd.v4f32(<4 x float>, <4 x float>, <4 x float>) #1
 declare float @llvm.sin.f32(float) #1
 declare float @llvm.trunc.f32(float) #1
 declare float @llvm.round.f32(float) #1
 declare float @llvm.rint.f32(float) #1
 declare float @llvm.nearbyint.f32(float) #1
 declare float @llvm.canonicalize.f32(float) #1
 declare float @llvm.minnum.f32(float, float) #1
 declare float @llvm.maxnum.f32(float, float) #1