[llvm] r363998 - [Reassociate] Remove bogus assert reported in PR42349.

[Cameron McInally via llvm-commits]

On Thu, Jun 20, 2019 at 7:19 PM Quentin Colombet <qcolombet at apple.com>
wrote:

> Hi Cameron,
>
> > On Jun 20, 2019, at 4:03 PM, Cameron McInally via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
> >
> > Author: mcinally
> > Date: Thu Jun 20 16:03:55 2019
> > New Revision: 363998
> >
> > URL:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject-3Frev-3D363998-26view-3Drev&d=DwICAg&c=slrrB7dE8n7gBJbeO0g-IQ&r=O_4M49EtSpZ_-BQYeigzGv0P4__noMcSu2RYEjS1vKs&m=YQ1mgHvmgBHlV0nHO9AWhIyWQljJ4e8hX6xVgoVetx4&s=l0eFhOg-tIlTl-6RTI5MDtIrzpFOP8pGGu75gGY49_M&e=
> > Log:
> > [Reassociate] Remove bogus assert reported in PR42349.
> >
> > Also, add a FIXME for the unsafe transform on a unary FNeg. A unary FNeg
> can only be transformed to a FMul by -1.0 when the nnan flag is present.
> The unary FNeg project is a WIP, so the unsafe transformation is acceptable
> until that work is complete.
> >
> > The bogus assert with introduced in D63445.
> >
> > Added:
> >    llvm/trunk/test/Transforms/Reassociate/pr42349.ll
> > Modified:
> >    llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
> >
> > Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp
> > URL:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_lib_Transforms_Scalar_Reassociate.cpp-3Frev-3D363998-26r1-3D363997-26r2-3D363998-26view-3Ddiff&d=DwICAg&c=slrrB7dE8n7gBJbeO0g-IQ&r=O_4M49EtSpZ_-BQYeigzGv0P4__noMcSu2RYEjS1vKs&m=YQ1mgHvmgBHlV0nHO9AWhIyWQljJ4e8hX6xVgoVetx4&s=ass_w-YYLnJKouXo-C4_fH9niONTGLAsHDoO-TQT-ls&e=
> >
> ==============================================================================
> > --- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)
> > +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Thu Jun 20 16:03:55
> 2019
> > @@ -268,11 +268,7 @@ static BinaryOperator *CreateNeg(Value *
> > static BinaryOperator *LowerNegateToMultiply(Instruction *Neg) {
> >   assert((isa<UnaryOperator>(Neg) || isa<BinaryOperator>(Neg)) &&
> >          "Expected a Negate!");
> > -  // It's not safe to lower a unary FNeg into a FMul by -1.0. However,
> > -  // we can only reach this function with fast flags set, so it's
> > -  // safe to do with nnan.
> > -  assert((!isa<FPMathOperator>(Neg) || Neg->isFast()) &&
> > -          "Expecting FastMathFlags!");
> > +  // FIXME: It's not safe to lower a unary FNeg into a FMul by -1.0.
> >   unsigned OpNo = isa<BinaryOperator>(Neg) ? 1 : 0;
> >   Type *Ty = Neg->getType();
> >   Constant *NegOne = Ty->isIntOrIntVectorTy() ?
> >
> > Added: llvm/trunk/test/Transforms/Reassociate/pr42349.ll
> > URL:
> https://urldefense.proofpoint.com/v2/url?u=http-3A__llvm.org_viewvc_llvm-2Dproject_llvm_trunk_test_Transforms_Reassociate_pr42349.ll-3Frev-3D363998-26view-3Dauto&d=DwICAg&c=slrrB7dE8n7gBJbeO0g-IQ&r=O_4M49EtSpZ_-BQYeigzGv0P4__noMcSu2RYEjS1vKs&m=YQ1mgHvmgBHlV0nHO9AWhIyWQljJ4e8hX6xVgoVetx4&s=hQnajJMprFGSMJsqdEdufwFwuSBg729Z6Z2LpqtTYMw&e=
> >
> ==============================================================================
> > --- llvm/trunk/test/Transforms/Reassociate/pr42349.ll (added)
> > +++ llvm/trunk/test/Transforms/Reassociate/pr42349.ll Thu Jun 20
> 16:03:55 2019
> > @@ -0,0 +1,18 @@
> > +; NOTE: Assertions have been autogenerated by
> utils/update_test_checks.py
> > +; RUN: opt < %s -reassociate -S | FileCheck %s
> > +
> > +define  float @wibble(float %tmp6) #0 {
> > +; CHECK-LABEL: @wibble(
> > +; CHECK-NEXT:  bb:
> > +; CHECK-NEXT:    [[TMP7:%.*]] = fmul float [[TMP6:%.*]], -1.000000e+00
> > +; CHECK-NEXT:    [[TMP0:%.*]] = fsub float -0.000000e+00, 0.000000e+00
> > +; CHECK-NEXT:    [[TMP9:%.*]] = fmul fast float [[TMP6]],
> 0xFFF0000000000000
> > +; CHECK-NEXT:    ret float [[TMP9]]
>
> The IR looks wrong to me in that sample:
>

I'm still looking into this, but unless I made a horrible mistake, this was
Reassociate's behavior before my changes.


> 1. Both tmp0 and tmp7 are dead
>

Agreed, it looks like Reassociate is leaving dead instructions around. I
suspect that's why the other tests in this directory are running
-instcombine as well, to clean up the dead instructions.


> 2. tmp9 went from `(-%tmp6) * cst` to `%tmp6 * cst`, i.e., we dropped the
> fneg.
>

Not quite, the fneg is being rolled into the constant. I.e.:

Input:

  %tmp7 = fsub float -0.000000e+00, %tmp6
  %tmp9 = fmul fast float %tmp7, 0x7FF0000000000000

Output:

  %tmp9 = fmul fast float %tmp6, 0xFFF0000000000000

But, yeah, it's still a mystery to why the dead instructions are being left
around...
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190620/7d1ebdd8/attachment.html>