<div dir="ltr"><div>Correct - this patch is not NFC for 2 reasons:</div><div>1. It constant folds cases that might have escaped before (as seen in the ARM test).</div><div>2. It uses the flags exclusively.</div><div><br></div><div>I assumed that the node-level flags are created and propagated correctly now, so the target option is effectively deprecated. If that's not correct, let me know.<br></div></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Oct 6, 2020 at 5:11 PM Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">Hi Sanjay,<br>
<br>
Just double checking this is intended.<br>
<br>
This commit is not NFC because the old code used to check also the global options whereas the new one only checks the individual flags.<br>
<br>
Cheers,<br>
-Quentin<br>
<br>
> On Oct 4, 2020, at 8:35 AM, Sanjay Patel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> <br>
> <br>
> Author: Sanjay Patel<br>
> Date: 2020-10-04T11:31:57-04:00<br>
> New Revision: 2ccbf3dbd5bac9d4fea8b67404b4c6b006d4adbe<br>
> <br>
> URL: <a href="https://github.com/llvm/llvm-project/commit/2ccbf3dbd5bac9d4fea8b67404b4c6b006d4adbe" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/2ccbf3dbd5bac9d4fea8b67404b4c6b006d4adbe</a><br>
> DIFF: <a href="https://github.com/llvm/llvm-project/commit/2ccbf3dbd5bac9d4fea8b67404b4c6b006d4adbe.diff" rel="noreferrer" target="_blank">https://github.com/llvm/llvm-project/commit/2ccbf3dbd5bac9d4fea8b67404b4c6b006d4adbe.diff</a><br>
> <br>
> LOG: [SDAG] fold x * 0.0 at node creation time<br>
> <br>
> In the motivating case from <a href="https://llvm.org/PR47517" rel="noreferrer" target="_blank">https://llvm.org/PR47517</a><br>
> we create a node that does not get constant folded<br>
> before getNegatedExpression is attempted from some<br>
> other node, and we crash.<br>
> <br>
> By moving the fold into SelectionDAG::simplifyFPBinop(),<br>
> we get the constant fold sooner and avoid the problem.<br>
> <br>
> Added: <br>
> <br>
> <br>
> Modified: <br>
> llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
> llvm/test/CodeGen/ARM/softfp-constant-comparison.ll<br>
> llvm/test/CodeGen/X86/fmul-combines.ll<br>
> <br>
> Removed: <br>
> <br>
> <br>
> <br>
> ################################################################################<br>
> diff --git a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> index 0b6aca4ca34c..9df930a6e3ba 100644<br>
> --- a/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> +++ b/llvm/lib/CodeGen/SelectionDAG/DAGCombiner.cpp<br>
> @@ -13040,13 +13040,6 @@ SDValue DAGCombiner::visitFMUL(SDNode *N) {<br>
> if (SDValue NewSel = foldBinOpIntoSelect(N))<br>
> return NewSel;<br>
> <br>
> - if ((Options.NoNaNsFPMath && Options.NoSignedZerosFPMath) ||<br>
<br>
^^^ Here we check `Options`<br>
<br>
> - (Flags.hasNoNaNs() && Flags.hasNoSignedZeros())) {<br>
> - // fold (fmul A, 0) -> 0<br>
> - if (N1CFP && N1CFP->isZero())<br>
> - return N1;<br>
> - }<br>
> -<br>
> if (Options.UnsafeFPMath || Flags.hasAllowReassociation()) {<br>
> // fmul (fmul X, C1), C2 -> fmul X, C1 * C2<br>
> if (isConstantFPBuildVectorOrConstantFP(N1) &&<br>
> <br>
> diff --git a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
> index eef467d116b7..62d01fbf96cd 100644<br>
> --- a/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
> +++ b/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp<br>
> @@ -7475,6 +7475,11 @@ SDValue SelectionDAG::simplifyFPBinop(unsigned Opcode, SDValue X, SDValue Y,<br>
> if (YC->getValueAPF().isExactlyValue(1.0))<br>
> return X;<br>
> <br>
> + // X * 0.0 --> 0.0<br>
> + if (Opcode == ISD::FMUL && Flags.hasNoNaNs() && Flags.hasNoSignedZeros())<br>
<br>
^^^ Here we only check `Flags`<br>
<br>
> + if (YC->getValueAPF().isZero())<br>
> + return getConstantFP(0.0, SDLoc(Y), Y.getValueType());<br>
> +<br>
> return SDValue();<br>
> }<br>
> <br>
> <br>
> diff --git a/llvm/test/CodeGen/ARM/softfp-constant-comparison.ll b/llvm/test/CodeGen/ARM/softfp-constant-comparison.ll<br>
> index 0b4e42843cba..e076e75e8066 100644<br>
> --- a/llvm/test/CodeGen/ARM/softfp-constant-comparison.ll<br>
> +++ b/llvm/test/CodeGen/ARM/softfp-constant-comparison.ll<br>
> @@ -9,24 +9,13 @@ target triple = "thumbv7em-arm-none-eabi"<br>
> define hidden void @fn1() nounwind #0 {<br>
> ; CHECK-LABEL: fn1:<br>
> ; CHECK: @ %bb.0: @ %entry<br>
> -; CHECK-NEXT: .save {r7, lr}<br>
> -; CHECK-NEXT: push {r7, lr}<br>
> -; CHECK-NEXT: vldr d0, .LCPI0_0<br>
> -; CHECK-NEXT: vmov r0, r1, d0<br>
> -; CHECK-NEXT: mov r2, r0<br>
> -; CHECK-NEXT: mov r3, r1<br>
> -; CHECK-NEXT: bl __aeabi_dcmpeq<br>
> +; CHECK-NEXT: movs r0, #1<br>
> ; CHECK-NEXT: cbnz r0, .LBB0_2<br>
> ; CHECK-NEXT: b .LBB0_1<br>
> ; CHECK-NEXT: .LBB0_1: @ %land.rhs<br>
> ; CHECK-NEXT: b .LBB0_2<br>
> ; CHECK-NEXT: .LBB0_2: @ %land.end<br>
> -; CHECK-NEXT: pop {r7, pc}<br>
> -; CHECK-NEXT: .p2align 3<br>
> -; CHECK-NEXT: @ %bb.3:<br>
> -; CHECK-NEXT: .LCPI0_0:<br>
> -; CHECK-NEXT: .long 0 @ double 0<br>
> -; CHECK-NEXT: .long 0<br>
> +; CHECK-NEXT: bx lr<br>
> entry:<br>
> %0 = load i32, i32* @a, align 4<br>
> %conv = sitofp i32 %0 to double<br>
> <br>
> diff --git a/llvm/test/CodeGen/X86/fmul-combines.ll b/llvm/test/CodeGen/X86/fmul-combines.ll<br>
> index 0c8a822e7ff7..f9a3e75c3d78 100644<br>
> --- a/llvm/test/CodeGen/X86/fmul-combines.ll<br>
> +++ b/llvm/test/CodeGen/X86/fmul-combines.ll<br>
> @@ -261,3 +261,22 @@ define <4 x float> @fmul_fneg_fneg_v4f32(<4 x float> %x, <4 x float> %y) {<br>
> %mul = fmul <4 x float> %x.neg, %y.neg<br>
> ret <4 x float> %mul<br>
> }<br>
> +<br>
> +; PR47517 - this could crash if we create 'fmul x, 0.0' nodes<br>
> +; that do not constant fold in a particular order.<br>
> +<br>
> +define float @getNegatedExpression_crash(float* %p) {<br>
> +; CHECK-LABEL: getNegatedExpression_crash:<br>
> +; CHECK: # %bb.0:<br>
> +; CHECK-NEXT: movl $0, (%rdi)<br>
> +; CHECK-NEXT: xorps %xmm0, %xmm0<br>
> +; CHECK-NEXT: retq<br>
> + store float 0.0, float* %p, align 1<br>
> + %real = load float, float* %p, align 1<br>
> + %r2 = fmul fast float %real, %real<br>
> + %t1 = fmul fast float %real, 42.0<br>
> + %t2 = fmul fast float %real, %t1<br>
> + %mul_ac56 = fmul fast float %t2, %t1<br>
> + %mul_ac72 = fmul fast float %r2, %mul_ac56<br>
> + ret float %mul_ac72<br>
> +}<br>
> <br>
> <br>
> <br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
> <a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
<br>
</blockquote></div>