[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 ↗

(On Diff #194945)

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 ↗

(On Diff #195116)

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

llvm/trunk/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::FADD)
+      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() != Opc)
+      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() != Opc)
+      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() != Opposite)
+      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() != AMDGPUISD::FMED3)
+      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?

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

define amdgpu_kernel void @v_fneg_add_multi_use_fneg_x_f32(float addrspace(1)* %out, float addrspace(1)* %a.ptr, float addrspace(1)* %b.ptr, float %c) #0 {
   %add = fadd float %fneg.a, %b
   %fneg = fsub float -0.000000e+00, %add
   %use1 = fmul float %fneg.a, %c
   store volatile float %fneg, float addrspace(1)* %out
   store volatile float %use1, float addrspace(1)* %out
   ret void
 }
 
+; This one asserted with -enable-no-signed-zeros-fp-math
+; GCN-LABEL: {{^}}fneg_fadd_0:
+; GCN-SAFE-DAG: v_mad_f32 [[A:v[0-9]+]],
+; GCN-SAFE-DAG: v_xor_b32_e32 [[B:v[0-9]+]], 0x80000000
+; GCN-SAFE-DAG: v_cmp_ngt_f32_e32 {{.*}}, [[A]]
+; GCN-NSZ-DAG: v_mac_f32_e32 [[C:v[0-9]+]],
+; GCN-NSZ-DAG: v_cmp_nlt_f32_e64 {{.*}}, -[[C]]
+
+define amdgpu_ps float @fneg_fadd_0(float inreg %tmp2, float inreg %tmp6, <4 x i32> %arg) local_unnamed_addr #0 {
+.entry:
+  %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, %tmp2
+  %tmp11 = fsub float -0.000000e+00, %.i188
+  %.i092 = select i1 %tmp10, float %tmp2, float %tmp11
+  %tmp12 = fcmp ule float %.i092, 0.000000e+00
+  %.i198 = select i1 %tmp12, float 0.000000e+00, float 0x7FF8000000000000
+  ret float %.i198
+}
+
 ; --------------------------------------------------------------------------------
 ; fmul tests
 ; --------------------------------------------------------------------------------
 
 ; GCN-LABEL: {{^}}v_fneg_mul_f32:
 ; GCN: {{buffer|flat}}_load_dword [[A:v[0-9]+]]
 ; GCN: {{buffer|flat}}_load_dword [[B:v[0-9]+]]
 ; GCN: v_mul_f32_e64 [[RESULT:v[0-9]+]], [[A]], -[[B]]