[AMDGPU] Avoid DAG combining assert with fneg(fadd(A,0))

[tpr]

Summary

fneg combining attempts to turn it into fadd(fneg(A), fneg(0)), but
creating the new fadd folds to just fneg(A). When A has multiple uses,
this confuses it and you get an assert. Fixed.

Change-Id: I0ddc9b7286abe78edc0cd8d734fdeb05ff09821c

[tpr]

That's the best bugpoint could do with the test.

[arsenm]

test/CodeGen/AMDGPU/assert-combine-fneg-fadd-0.ll

8–39

I can usually go a lot further than bug point, especially once I know what the actual problem is. This is also going to only hit one of the cases you added here.

The test cases should also go in the existing file for these combines

[tpr]

V2: Cut down test a bit more and put it in fneg-combines.ll.

[tpr]

I have cut down the test a bit more and put it into fneg-combines.ll. I did not manage to repro any problems with the other cases that I added fixes for.

I don't have a massive amount of time to spend on this. Would you prefer I land it as is, without tests for the other cases, or remove the precautionary fix for those cases and wait for someone else to possibly encounter them?

[arsenm]

In D60633#1466195, @tpr wrote:

I have cut down the test a bit more and put it into fneg-combines.ll. I did not manage to repro any problems with the other cases that I added fixes for.

I don't have a massive amount of time to spend on this. Would you prefer I land it as is, without tests for the other cases, or remove the precautionary fix for those cases and wait for someone else to possibly encounter them?

In either case, you should open a bug somewhere to look at this again.

lib/Target/AMDGPU/AMDGPUISelLowering.cpp

3715–3716

I think it would be slightly safer to check != ISD::FADD, in case some other fold decided to do something else

test/CodeGen/AMDGPU/fneg-combines.ll

219 ↗

(On Diff #195116)

Should check something

[tpr]

V3: Addressed review comments.

[tpr]

Bug for adding remaining test cases is https://bugs.llvm.org/show_bug.cgi?id=41500

[arsenm]

LGTM

lib/Target/AMDGPU/AMDGPUISelLowering.cpp

else
       LHS = LHS.getOperand(0);
 
     if (RHS.getOpcode() != ISD::FNEG)
       RHS = DAG.getNode(ISD::FNEG, SL, VT, RHS);
     else
       RHS = RHS.getOperand(0);
 
     SDValue Res = DAG.getNode(ISD::FADD, SL, VT, LHS, RHS, N0->getFlags());
+    if (Res.getOpcode() == ISD::FNEG)
+      return SDValue(); // Op got folded away.
     if (!N0.hasOneUse())
       DAG.ReplaceAllUsesWith(N0, DAG.getNode(ISD::FNEG, SL, VT, Res));
     return Res;
   }
   case ISD::FMUL:
   case AMDGPUISD::FMUL_LEGACY: {
     // (fneg (fmul x, y)) -> (fmul x, (fneg y))
     // (fneg (fmul_legacy x, y)) -> (fmul_legacy x, (fneg y))
     SDValue LHS = N0.getOperand(0);
     SDValue RHS = N0.getOperand(1);
 
     if (LHS.getOpcode() == ISD::FNEG)
       LHS = LHS.getOperand(0);
     else if (RHS.getOpcode() == ISD::FNEG)
       RHS = RHS.getOperand(0);
     else
       RHS = DAG.getNode(ISD::FNEG, SL, VT, RHS);
 
     SDValue Res = DAG.getNode(Opc, SL, VT, LHS, RHS, N0->getFlags());
+    if (Res.getOpcode() == ISD::FNEG)
+      return SDValue(); // Op got folded away.
     if (!N0.hasOneUse())
       DAG.ReplaceAllUsesWith(N0, DAG.getNode(ISD::FNEG, SL, VT, Res));
     return Res;
   }
   case ISD::FMA:
   case ISD::FMAD: {
     if (!mayIgnoreSignedZero(N0))
       return SDValue();
else
       MHS = DAG.getNode(ISD::FNEG, SL, VT, MHS);
 
     if (RHS.getOpcode() != ISD::FNEG)
       RHS = DAG.getNode(ISD::FNEG, SL, VT, RHS);
     else
       RHS = RHS.getOperand(0);
 
     SDValue Res = DAG.getNode(Opc, SL, VT, LHS, MHS, RHS);
+    if (Res.getOpcode() == ISD::FNEG)
+      return SDValue(); // Op got folded away.
     if (!N0.hasOneUse())
       DAG.ReplaceAllUsesWith(N0, DAG.getNode(ISD::FNEG, SL, VT, Res));
     return Res;
   }
   case ISD::FMAXNUM:
   case ISD::FMINNUM:
   case ISD::FMAXNUM_IEEE:
   case ISD::FMINNUM_IEEE:
case AMDGPUISD::FMIN_LEGACY: {
     if (isConstantCostlierToNegate(RHS))
       return SDValue();
 
     SDValue NegLHS = DAG.getNode(ISD::FNEG, SL, VT, LHS);
     SDValue NegRHS = DAG.getNode(ISD::FNEG, SL, VT, RHS);
     unsigned Opposite = inverseMinMax(Opc);
 
     SDValue Res = DAG.getNode(Opposite, SL, VT, NegLHS, NegRHS, N0->getFlags());
+    if (Res.getOpcode() == ISD::FNEG)
+      return SDValue(); // Op got folded away.
     if (!N0.hasOneUse())
       DAG.ReplaceAllUsesWith(N0, DAG.getNode(ISD::FNEG, SL, VT, Res));
     return Res;
   }
   case AMDGPUISD::FMED3: {
     SDValue Ops[3];
     for (unsigned I = 0; I < 3; ++I)
       Ops[I] = DAG.getNode(ISD::FNEG, SL, VT, N0->getOperand(I), N0->getFlags());
 
     SDValue Res = DAG.getNode(AMDGPUISD::FMED3, SL, VT, Ops, N0->getFlags());
+    if (Res.getOpcode() == ISD::FNEG)
+      return SDValue(); // Op got folded away.
     if (!N0.hasOneUse())
       DAG.ReplaceAllUsesWith(N0, DAG.getNode(ISD::FNEG, SL, VT, Res));
     return Res;
   }
   case ISD::FP_EXTEND:
   case ISD::FTRUNC:
   case ISD::FRINT:
   case ISD::FNEARBYINT: // XXX - Should fround be handled?

test/CodeGen/AMDGPU/assert-combine-fneg-fadd-0.ll

+; RUN: llc -mtriple=amdgcn--amdpal -mcpu=gfx900 -enable-no-signed-zeros-fp-math -stop-after=amdgpu-isel -o - <%s | FileCheck -check-prefix=GCN %s
+
+; Check that this does not assert when trying to combine an fneg of a
+; multi-use fadd of constant 0.
+
+; GCN-LABEL: body:
+
+define amdgpu_ps void @_amdgpu_ps_main(<4 x i32> %arg) local_unnamed_addr {
+.entry:
+  %tmp = tail call i32 @llvm.amdgcn.s.buffer.load.i32(<4 x i32> %arg, i32 224, i32 0)
+  %tmp1 = bitcast i32 %tmp to float
+  %tmp2 = tail call float @llvm.maxnum.f32(float %tmp1, float 0.000000e+00)
+  %tmp3 = tail call i32 @llvm.amdgcn.s.buffer.load.i32(<4 x i32> %arg, i32 144, i32 0)
+  %tmp4 = bitcast i32 %tmp3 to float
+  %tmp5 = fdiv float 1.000000e+00, %tmp4
+  %tmp6 = fmul float %tmp5, 0.000000e+00
+  %.i067 = or i1 false, undef
+  %.i070 = select i1 %.i067, float %tmp2, float undef
+  %.i081 = or i1 false, undef
+  %.i084 = select i1 %.i081, float %.i070, float undef
+  %tmp7 = fdiv float 1.000000e+00, %tmp6
+  %tmp8 = fmul float 0.000000e+00, %tmp7
+  %tmp9 = fmul reassoc nnan arcp contract float 0.000000e+00, %tmp8
+  %.i188 = fadd float %tmp9, 0.000000e+00
+  %tmp10 = fcmp uge float %.i188, %.i084
+  %tmp11 = fsub float -0.000000e+00, %.i188
+  %.i089 = or i1 false, %tmp10
+  %.i092 = select i1 %.i089, float %.i084, float %tmp11
+  %tmp12 = fcmp ule float %.i092, 0.000000e+00
+  %tmp13 = fmul reassoc nnan arcp contract float undef, -5.000000e-01
+  %.i095 = or i1 false, %tmp12
+  %.i198 = select i1 %.i095, float 0.000000e+00, float %tmp13
+  %tmp14 = tail call <4 x float> @llvm.amdgcn.image.sample.2darray.v4f32.f32(i32 15, float undef, float %.i198, float undef, <8 x i32> undef, <4 x i32> undef, i1 false, i32 0, i32 0)
+  %tmp15 = extractelement <4 x float> %tmp14, i32 1
+  %tmp16 = tail call float @llvm.log2.f32(float %tmp15)
+  %.i1246 = fmul float %tmp16, 0x40019999A0000000
+  tail call void @llvm.amdgcn.exp.compr.v2f16(i32 immarg 0, i32 immarg 15, <2 x half> undef, <2 x half> undef, i1 immarg true, i1 immarg true)
+  ret void
+}
+
+declare float @llvm.maxnum.f32(float, float)
+declare float @llvm.log2.f32(float)
+declare <4 x float> @llvm.amdgcn.image.sample.2darray.v4f32.f32(i32 immarg, float, float, float, <8 x i32>, <4 x i32>, i1 immarg, i32 immarg, i32 immarg)
+declare i32 @llvm.amdgcn.s.buffer.load.i32(<4 x i32>, i32, i32 immarg)
+declare void @llvm.amdgcn.exp.compr.v2f16(i32 immarg, i32 immarg, <2 x half>, <2 x half>, i1 immarg, i1 immarg)