[TargetLowering] Allow constants with multiple uses

[dstuttard]

Summary

Multiple uses of a constant was causing an assert in target lowering for
propagating fnegs. Allow multiple uses when it is a constant fixes the issue.

Change-Id: Iffd1ce18bf13ec68d91ff47aecd7cf1918fd3c0a

[lebedev.ri]

llvm/test/CodeGen/AMDGPU/const-multiuse-tl.ll

2

This test doesn't actually check anything.

[dstuttard]

Included wrong version of the test initially

[dstuttard]

llvm/test/CodeGen/AMDGPU/const-multiuse-tl.ll

2

Yes, spotted it just after I uploaded. It does now. It's really only testing that it doesn't assert.

[dstuttard]

Added @RKSimon to review as the last major modifier of the code here.

[RKSimon]

llvm/test/CodeGen/AMDGPU/const-multiuse-tl.ll

2

Is this reduced as much as possible (both in terms of instructions and metadata)?

[dstuttard]

I've got some failures as a result of this change in X86 (haven't tried the other backends). I guess that the failures are probably due to the fneg propagation working better for constants - but I haven't investigated yet. There will need to be more test changes as a result of this change.
My question is - does the change look reasonable? If so I'll take a look at the lit regressions for other backends.

llvm/test/CodeGen/AMDGPU/const-multiuse-tl.ll

2

I've attempted to reduce it as much as possible - I did attempt to reduce it further but that stopped the problem occurring.
I might be able to reduce the metadata more.

[RKSimon]

I'd like to see more of the regressions on other targets (x86 in particular) - in many cases this will cause a lot of extra constant pool entries.

[dstuttard]

In D70595#1756702, @RKSimon wrote:

I'd like to see more of the regressions on other targets (x86 in particular) - in many cases this will cause a lot of extra constant pool entries.

Ok - I'' take a look at those and upload some patches.

I'm more than happy if this can be solved in a different way - the issue isn't an optimisation opportunity, it is actually causing a problem with the attached example due to the difference between checking isNegatibleForFree (and it syaing it is) and it then finding a problem in getNegatedExpression when it returns a different (and unexpected) result from a call to isNegatableForFree when handling an ISD::FMA.

[RKSimon]

I'd be more interested in fixing the isNegatibleForFree/getNegatedExpression mismatch - the next step for PR42863 will be to convert AMDGPU's performFNegCombine to TLI isNegatibleForFree/getNegatedExpression callbacks like X86.

[dstuttard]

Added test updates for failing lit tests.

[dstuttard]

Changed the implementation slightly.
Now the isNegatibleForFree includes a bool for allowing multi-use
constants. This is only set to true for calls from getNegatedExpression so it
only allows for the situation where a constant becomes multi-use during
getNegatedExpression recursive evaluation.

This makes all the existing lit tests pass again (with no changes).

[spatel]

First, we can definitely reduce the test - it only needs an fneg(fma(fmul X, C), -C, Y) pattern to show the crash, and that should repro on any target with fma support (so it's just unlucky that we didn't hit this bug sooner).

I thought we could get away with a simpler patch as originally proposed here, but I have convinced myself that would only be sweeping the bug under the rug. In fact, I don't think the current version of this patch goes far enough. When we're rewriting the expression, we can create arbitrary values, so *any* value (not just a constant) could have its number of uses changed and cause a crash/assert from mismatched handling in isNegatibleForFree/getNegatedExpression. I haven't reduced a test for that, but it seems possible.

I'll post an alternate patch that should solve the problem and see what people think of it.

[spatel]

In D70595#1767515, @spatel wrote:

I'll post an alternate patch that should solve the problem and see what people think of it.

D70975

[lebedev.ri]

@dstuttard does D70975 cover the original problem motivating this patch?

[dstuttard]

In D70595#1775820, @lebedev.ri wrote:

@dstuttard does D70975 cover the original problem motivating this patch?

Yes - confirmed that the alternative also fixes the issue for me.
I'll abandon this one.

[dstuttard]

Alternative fix D70975 works.

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp

char TargetLowering::isNegatibleForFree(SDValue Op, SelectionDAG &DAG,
   // fneg is removable even if it has multiple uses.
   if (Op.getOpcode() == ISD::FNEG)
     return 2;
 
   // Don't allow anything with multiple uses unless we know it is free.
   EVT VT = Op.getValueType();
   const SDNodeFlags Flags = Op->getFlags();
   const TargetOptions &Options = DAG.getTarget().Options;
-  if (!Op.hasOneUse() && !(Op.getOpcode() == ISD::FP_EXTEND &&
-                           isFPExtFree(VT, Op.getOperand(0).getValueType())))
+  if (!Op.hasOneUse() &&
+      !(Op.getOpcode() == ISD::FP_EXTEND &&
+        isFPExtFree(VT, Op.getOperand(0).getValueType())) &&
+      !(Op.getOpcode() == ISD::ConstantFP))
     return 0;
 
   // Don't recurse exponentially.
   if (Depth > SelectionDAG::MaxRecursionDepth)
     return 0;
 
   switch (Op.getOpcode()) {
   case ISD::ConstantFP: {

llvm/test/CodeGen/AMDGPU/const-multiuse-tl.ll

+; RUN: llc -march=amdgcn -mcpu=gfx1010 -verify-machineinstrs < %s | FileCheck --check-prefixes=GCN %s
+
+; GCN: v_fma_f32 v0, s0, 0, 0.5
+
+define amdgpu_ps void @main(float addrspace(6)* %arg1) #0 {
+main_body:
+  %tmp2 = load float, float addrspace(6)* %arg1
+  %tmp3 = call nsz float @llvm.floor.f32(float undef) #2
+  %tmp4 = fptosi float %tmp3 to i32
+  %tmp5 = add i32 0, %tmp4
+  %tmp6 = sub i32 0, %tmp5
+  %tmp7 = sitofp i32 %tmp4 to float
+  %tmp8 = fmul nsz float %tmp7, 0x3FEBB67AE0000000
+  %tmp9 = sitofp i32 %tmp6 to float
+  %tmp10 = call nsz float @llvm.fmuladd.f32(float %tmp9, float 0xBFEBB67AE0000000, float %tmp8) #2
+  %tmp11 = fsub nsz float 0.000000e+00, %tmp10
+  %tmp12 = call nsz float @llvm.fmuladd.f32(float %tmp11, float 5.000000e-01, float 5.000000e-01) #2
+  %tmp13 = call nsz float @llvm.fmuladd.f32(float 0.000000e+00, float %tmp2, float 5.000000e-01) #2
+  %tmp14 = call nsz float @llvm.floor.f32(float %tmp13) #2
+  %tmp15 = fptosi float %tmp14 to i32
+  %tmp16 = icmp eq i32 %tmp15, 0
+  br i1 %tmp16, label %endif06, label %if04
+
+if04:                                             ; preds = %main_body
+  %tmp17 = fadd nsz float %tmp12, -5.000000e-01
+  %tmp18 = fneg nsz float %tmp17
+  %tmp19 = fmul nsz float 0.000000e+00, %tmp18
+  %tmp20 = call nsz float @llvm.fmuladd.f32(float 0.000000e+00, float 0.000000e+00, float %tmp19) #2
+  %tmp21 = fadd nsz float %tmp20, 5.000000e-01
+  %tmp22 = fadd nsz float %tmp21, 0.000000e+00
+  %tmp23 = fmul nsz float %tmp22, 0.000000e+00
+  %tmp24 = call nsz <4 x float> @llvm.amdgcn.image.sample.2d.v4f32.f32(i32 15, float %tmp23, float undef, <8 x i32> undef, <4 x i32> undef, i1 false, i32 0, i32 0) #4
+  %tmp25 = extractelement <4 x float> %tmp24, i32 3
+  %tmp26 = fsub nsz float 1.000000e+00, %tmp25
+  %tmp27 = call nsz float @llvm.fmuladd.f32(float undef, float %tmp26, float undef) #2
+  unreachable
+
+endif06:                                          ; preds = %main_body
+  ret void
+}
+
+; Function Attrs: nounwind readonly
+declare <4 x float> @llvm.amdgcn.image.sample.2d.v4f32.f32(i32 immarg, float, float, <8 x i32>, <4 x i32>, i1 immarg, i32 immarg, i32 immarg) #1
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare float @llvm.floor.f32(float) #3
+
+; Function Attrs: nounwind readnone speculatable willreturn
+declare float @llvm.fmuladd.f32(float, float, float) #3
+
+attributes #0 = { "no-signed-zeros-fp-math"="true" }
+attributes #1 = { nounwind readonly }
+attributes #2 = { nounwind readnone }
+attributes #3 = { nounwind readnone speculatable willreturn }
+attributes #4 = { convergent nounwind readnone }