[llvm] r339176 - [InstSimplify] fold fsub+fadd with common operand

[Sanjay Patel via llvm-commits]

Hi Roman,

Thanks for reviewing this change.

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':
"Allow reassociation transformations for floating-point instructions. This
may dramatically change results in floating-point."

Once you allow FP reassociation, there's no really no way for the compiler
to guarantee anything. :)

We're following the precedent of GCC here:
https://godbolt.org/g/ycFXXe

-fassociative-math maps to 'reassoc' in LLVM
-fno-signed-zeros maps to 'nsz'
-fno-trapping-math has no equivalent in LLVM (we always assume no-trapping
unless code uses the 'strict' FP intrinsics)

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.


On Wed, Aug 8, 2018 at 2:24 PM, Roman Tereshin <rtereshin at apple.com> wrote:

> Hi Sanjay,
>
> + Michael
>
>
> I'm not sure it's enough to check for nsz and reassoc flags here.
>
> I think reassoc let's you do
>
> Y + (X - Y)  -->   (Y - Y) + X
>
> 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.
> The check could be done by checking just the nnan flag on the top level op.
>
> Shouldn't we check for the nnan flag here as well while doing this?
>
>
> Another question here is that we check here the top-level reassoc flag
> only. I'm not sure if it's enough either.
>
> Meaning, I think it's possible that while
>
> (Y + (X - Y)<reassoc>)<reassoc>  -->   ((Y - Y)<reassoc> + X)<reassoc>
>
> is perfectly legal
>
> (Y + (X - Y))<reassoc>  -->  (Y - Y) + X
>
> is not allowed.
>
> What do you think?
>
>
> Thanks,
> Roman
>
> > On Aug 7, 2018, at 1:32 PM, Sanjay Patel via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: spatel
> > Date: Tue Aug  7 13:32:55 2018
> > New Revision: 339176
> >
> > URL: http://llvm.org/viewvc/llvm-project?rev=339176&view=rev
> > Log:
> > [InstSimplify] fold fsub+fadd with common operand
> >
> > Modified:
> >    llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> >    llvm/trunk/test/Transforms/InstSimplify/floating-point-arithmetic.ll
> >
> > Modified: llvm/trunk/lib/Analysis/InstructionSimplify.cpp
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/
> Analysis/InstructionSimplify.cpp?rev=339176&r1=339175&r2=339176&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/lib/Analysis/InstructionSimplify.cpp (original)
> > +++ llvm/trunk/lib/Analysis/InstructionSimplify.cpp Tue Aug  7 13:32:55
> 2018
> > @@ -4359,6 +4359,14 @@ static Value *SimplifyFAddInst(Value *Op
> >                        match(Op1, m_FSub(m_AnyZeroFP(),
> m_Specific(Op0)))))
> >     return ConstantFP::getNullValue(Op0->getType());
> >
> > +  // (X - Y) + Y --> X
> > +  // Y + (X - Y) --> X
> > +  Value *X;
> > +  if (FMF.noSignedZeros() && FMF.allowReassoc() &&
> > +      (match(Op0, m_FSub(m_Value(X), m_Specific(Op1))) ||
> > +       match(Op1, m_FSub(m_Value(X), m_Specific(Op0)))))
> > +    return X;
> > +
> >   return nullptr;
> > }
> >
> >
> > Modified: llvm/trunk/test/Transforms/InstSimplify/floating-point-
> arithmetic.ll
> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/
> Transforms/InstSimplify/floating-point-arithmetic.ll?
> rev=339176&r1=339175&r2=339176&view=diff
> > ============================================================
> ==================
> > --- llvm/trunk/test/Transforms/InstSimplify/floating-point-arithmetic.ll
> (original)
> > +++ llvm/trunk/test/Transforms/InstSimplify/floating-point-arithmetic.ll
> Tue Aug  7 13:32:55 2018
> > @@ -849,9 +849,7 @@ define float @fadd_fsub_common_op_wrong_
> >
> > define <2 x float> @fsub_fadd_common_op_vec(<2 x float> %x, <2 x float>
> %y) {
> > ; CHECK-LABEL: @fsub_fadd_common_op_vec(
> > -; CHECK-NEXT:    [[S:%.*]] = fsub <2 x float> [[X:%.*]], [[Y:%.*]]
> > -; CHECK-NEXT:    [[R:%.*]] = fadd reassoc nsz <2 x float> [[Y]], [[S]]
> > -; CHECK-NEXT:    ret <2 x float> [[R]]
> > +; CHECK-NEXT:    ret <2 x float> [[X:%.*]]
> > ;
> >   %s = fsub <2 x float> %x, %y
> >   %r = fadd reassoc nsz <2 x float> %y, %s
> > @@ -862,9 +860,7 @@ define <2 x float> @fsub_fadd_common_op_
> >
> > define float @fsub_fadd_common_op_commute(float %x, float %y) {
> > ; CHECK-LABEL: @fsub_fadd_common_op_commute(
> > -; CHECK-NEXT:    [[S:%.*]] = fsub float [[X:%.*]], [[Y:%.*]]
> > -; CHECK-NEXT:    [[R:%.*]] = fadd reassoc nsz float [[S]], [[Y]]
> > -; CHECK-NEXT:    ret float [[R]]
> > +; CHECK-NEXT:    ret float [[X:%.*]]
> > ;
> >   %s = fsub float %x, %y
> >   %r = fadd reassoc nsz float %s, %y
> >
> >
> > _______________________________________________
> > llvm-commits mailing list
> > llvm-commits at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180808/f25acfa8/attachment.html>