<div dir="ltr"><div>Hi Roman,</div><div><br></div><div>Thanks for reviewing this change. <br></div><div><br></div><div>You're correct that the original code could return NaN while the optimized code would not. This is allowed by our (admittedly vague) definition of 'reassoc':</div><div>"Allow reassociation transformations for floating-point instructions.
This may dramatically change results in floating-point."<br></div><div><br></div><div>Once you allow FP reassociation, there's no really no way for the compiler to guarantee anything. :)<br></div><div><br></div><div>We're following the precedent of GCC here:<br></div><div><a href="https://godbolt.org/g/ycFXXe">https://godbolt.org/g/ycFXXe</a></div><div><br></div><div>-fassociative-math maps to 'reassoc' in LLVM</div><div>-fno-signed-zeros maps to 'nsz'</div><div>-fno-trapping-math has no equivalent in LLVM (we always assume no-trapping unless code uses the 'strict' FP intrinsics)<br></div><div><br></div><div>The question of whether we should require FMF on both instructions or just the top-level is also established in IR and the backend. The reasoning is that if the top-level instruction/node allows some form of fast-math, then ops that feed into that result are granted the same fast-ness because the final result was allowed to be non-strict. If there is some other user of an intermediate result, then it could be treated with a different set of FMF for the purpose of the other calculation. AFAIK, this scenario hasn't been tested extensively because it requires some kind of LTO with mixed FP optimizations.<br></div><div><br></div><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Aug 8, 2018 at 2:24 PM, Roman Tereshin <span dir="ltr"><<a href="mailto:rtereshin@apple.com" target="_blank">rtereshin@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Sanjay,<br>
<br>
+ Michael<br>
<br>
<br>
I'm not sure it's enough to check for nsz and reassoc flags here.<br>
<br>
I think reassoc let's you do<br>
<br>
Y + (X - Y) --> (Y - Y) + X<br>
<br>
But w/o knowing that Y is non a NaN and not an infinity (of any sign) we can't tell if the result is a NaN or X, I believe.<br>
The check could be done by checking just the nnan flag on the top level op.<br>
<br>
Shouldn't we check for the nnan flag here as well while doing this?<br>
<br>
<br>
Another question here is that we check here the top-level reassoc flag only. I'm not sure if it's enough either.<br>
<br>
Meaning, I think it's possible that while<br>
<br>
(Y + (X - Y)<reassoc>)<reassoc> --> ((Y - Y)<reassoc> + X)<reassoc><br>
<br>
is perfectly legal<br>
<br>
(Y + (X - Y))<reassoc> --> (Y - Y) + X<br>
<br>
is not allowed.<br>
<br>
What do you think?<br>
<br>
<br>
Thanks,<br>
Roman<br>
<div class="HOEnZb"><div class="h5"><br>
> On Aug 7, 2018, at 1:32 PM, Sanjay Patel via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> <br>
> Author: spatel<br>
> Date: Tue Aug 7 13:32:55 2018<br>
> New Revision: 339176<br>
> <br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=339176&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project?rev=339176&view=rev</a><br>
> Log:<br>
> [InstSimplify] fold fsub+fadd with common operand<br>
> <br>
> Modified:<br>
> llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp<br>
> llvm/trunk/test/Transforms/<wbr>InstSimplify/floating-point-<wbr>arithmetic.ll<br>
> <br>
> Modified: llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/InstructionSimplify.cpp?rev=339176&r1=339175&r2=339176&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/lib/<wbr>Analysis/InstructionSimplify.<wbr>cpp?rev=339176&r1=339175&r2=<wbr>339176&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp (original)<br>
> +++ llvm/trunk/lib/Analysis/<wbr>InstructionSimplify.cpp Tue Aug 7 13:32:55 2018<br>
> @@ -4359,6 +4359,14 @@ static Value *SimplifyFAddInst(Value *Op<br>
> match(Op1, m_FSub(m_AnyZeroFP(), m_Specific(Op0)))))<br>
> return ConstantFP::getNullValue(Op0-><wbr>getType());<br>
> <br>
> + // (X - Y) + Y --> X<br>
> + // Y + (X - Y) --> X<br>
> + Value *X;<br>
> + if (FMF.noSignedZeros() && FMF.allowReassoc() &&<br>
> + (match(Op0, m_FSub(m_Value(X), m_Specific(Op1))) ||<br>
> + match(Op1, m_FSub(m_Value(X), m_Specific(Op0)))))<br>
> + return X;<br>
> +<br>
> return nullptr;<br>
> }<br>
> <br>
> <br>
> Modified: llvm/trunk/test/Transforms/<wbr>InstSimplify/floating-point-<wbr>arithmetic.ll<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstSimplify/floating-point-arithmetic.ll?rev=339176&r1=339175&r2=339176&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-<wbr>project/llvm/trunk/test/<wbr>Transforms/InstSimplify/<wbr>floating-point-arithmetic.ll?<wbr>rev=339176&r1=339175&r2=<wbr>339176&view=diff</a><br>
> ==============================<wbr>==============================<wbr>==================<br>
> --- llvm/trunk/test/Transforms/<wbr>InstSimplify/floating-point-<wbr>arithmetic.ll (original)<br>
> +++ llvm/trunk/test/Transforms/<wbr>InstSimplify/floating-point-<wbr>arithmetic.ll Tue Aug 7 13:32:55 2018<br>
> @@ -849,9 +849,7 @@ define float @fadd_fsub_common_op_wrong_<br>
> <br>
> define <2 x float> @fsub_fadd_common_op_vec(<2 x float> %x, <2 x float> %y) {<br>
> ; CHECK-LABEL: @fsub_fadd_common_op_vec(<br>
> -; CHECK-NEXT: [[S:%.*]] = fsub <2 x float> [[X:%.*]], [[Y:%.*]]<br>
> -; CHECK-NEXT: [[R:%.*]] = fadd reassoc nsz <2 x float> [[Y]], [[S]]<br>
> -; CHECK-NEXT: ret <2 x float> [[R]]<br>
> +; CHECK-NEXT: ret <2 x float> [[X:%.*]]<br>
> ;<br>
> %s = fsub <2 x float> %x, %y<br>
> %r = fadd reassoc nsz <2 x float> %y, %s<br>
> @@ -862,9 +860,7 @@ define <2 x float> @fsub_fadd_common_op_<br>
> <br>
> define float @fsub_fadd_common_op_commute(<wbr>float %x, float %y) {<br>
> ; CHECK-LABEL: @fsub_fadd_common_op_commute(<br>
> -; CHECK-NEXT: [[S:%.*]] = fsub float [[X:%.*]], [[Y:%.*]]<br>
> -; CHECK-NEXT: [[R:%.*]] = fadd reassoc nsz float [[S]], [[Y]]<br>
> -; CHECK-NEXT: ret float [[R]]<br>
> +; CHECK-NEXT: ret float [[X:%.*]]<br>
> ;<br>
> %s = fsub float %x, %y<br>
> %r = fadd reassoc nsz float %s, %y<br>
> <br>
> <br>
> ______________________________<wbr>_________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-commits</a><br>
<br>
</div></div></blockquote></div><br></div></div>