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

[Cameron McInally via llvm-commits]

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

>
>
> Le 20 juin 2019 à 17:02, Cameron McInally <cameron.mcinally at nyu.edu> a
> écrit :
>
> 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
>
>
> Oh I missed that!
> Then everything is fine. The dead code is expected. I thought it was an
> artefact of the transformation being half done.
>
> Sorry for the noise.
>

Great news! It's 8pm here and I'm spent. ;) Thanks for the quick reply.

I'll continue to clean up PR42349 tomorrow, but for now master should be
functioning as expected.
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190620/be63f0de/attachment.html>