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

Thu Jun 20 17:08:55 PDT 2019

> 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.

> 
> 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/e64ab802/attachment.html>