<div dir="ltr"><div dir="ltr"><div dir="ltr">On Thu, Jun 20, 2019 at 8:09 PM Quentin Colombet <<a href="mailto:qcolombet@apple.com">qcolombet@apple.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><div dir="auto"><br><div dir="ltr"><br>Le 20 juin 2019 à 17:02, Cameron McInally <<a href="mailto:cameron.mcinally@nyu.edu" target="_blank">cameron.mcinally@nyu.edu</a>> a écrit :<br><br></div><blockquote type="cite"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr"><div dir="ltr">On Thu, Jun 20, 2019 at 7:19 PM Quentin Colombet <<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Hi Cameron,<br>
<br>
> On Jun 20, 2019, at 4:03 PM, Cameron McInally via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a>> wrote:<br>
> <br>
> Author: mcinally<br>
> Date: Thu Jun 20 16:03:55 2019<br>
> New Revision: 363998<br>
> <br>
> URL: <a href="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=" rel="noreferrer" target="_blank">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=</a> <br>
> Log:<br>
> [Reassociate] Remove bogus assert reported in PR42349.<br>
> <br>
> 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.<br>
> <br>
> The bogus assert with introduced in D63445.<br>
> <br>
> Added:<br>
> llvm/trunk/test/Transforms/Reassociate/pr42349.ll<br>
> Modified:<br>
> llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp<br>
> <br>
> Modified: llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp<br>
> URL: <a href="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=" rel="noreferrer" target="_blank">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=</a> <br>
> ==============================================================================<br>
> --- llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp (original)<br>
> +++ llvm/trunk/lib/Transforms/Scalar/Reassociate.cpp Thu Jun 20 16:03:55 2019<br>
> @@ -268,11 +268,7 @@ static BinaryOperator *CreateNeg(Value *<br>
> static BinaryOperator *LowerNegateToMultiply(Instruction *Neg) {<br>
> assert((isa<UnaryOperator>(Neg) || isa<BinaryOperator>(Neg)) &&<br>
> "Expected a Negate!");<br>
> - // It's not safe to lower a unary FNeg into a FMul by -1.0. However,<br>
> - // we can only reach this function with fast flags set, so it's<br>
> - // safe to do with nnan.<br>
> - assert((!isa<FPMathOperator>(Neg) || Neg->isFast()) &&<br>
> - "Expecting FastMathFlags!");<br>
> + // FIXME: It's not safe to lower a unary FNeg into a FMul by -1.0.<br>
> unsigned OpNo = isa<BinaryOperator>(Neg) ? 1 : 0;<br>
> Type *Ty = Neg->getType();<br>
> Constant *NegOne = Ty->isIntOrIntVectorTy() ?<br>
> <br>
> Added: llvm/trunk/test/Transforms/Reassociate/pr42349.ll<br>
> URL: <a href="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=" rel="noreferrer" target="_blank">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=</a> <br>
> ==============================================================================<br>
> --- llvm/trunk/test/Transforms/Reassociate/pr42349.ll (added)<br>
> +++ llvm/trunk/test/Transforms/Reassociate/pr42349.ll Thu Jun 20 16:03:55 2019<br>
> @@ -0,0 +1,18 @@<br>
> +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py<br>
> +; RUN: opt < %s -reassociate -S | FileCheck %s<br>
> +<br>
> +define float @wibble(float %tmp6) #0 {<br>
> +; CHECK-LABEL: @wibble(<br>
> +; CHECK-NEXT: bb:<br>
> +; CHECK-NEXT: [[TMP7:%.*]] = fmul float [[TMP6:%.*]], -1.000000e+00<br>
> +; CHECK-NEXT: [[TMP0:%.*]] = fsub float -0.000000e+00, 0.000000e+00<br>
> +; CHECK-NEXT: [[TMP9:%.*]] = fmul fast float [[TMP6]], 0xFFF0000000000000<br>
> +; CHECK-NEXT: ret float [[TMP9]]<br>
<br>
The IR looks wrong to me in that sample:<br></blockquote><div><br></div><div>I'm still looking into this, but unless I made a horrible mistake, this was Reassociate's behavior before my changes. <br></div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
1. Both tmp0 and tmp7 are dead<br></blockquote><div><br></div><div>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.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
2. tmp9 went from `(-%tmp6) * cst` to `%tmp6 * cst`, i.e., we dropped the fneg.<br></blockquote><div><br></div><div>Not quite, the fneg is being rolled into the constant. I.e.:</div><div><br></div><div>Input:</div><div><br></div><div><div> %tmp7 = fsub float -0.000000e+00, %tmp6</div></div><div> %tmp9 = fmul fast float %tmp7, 0x7FF0000000000000<br></div><div><br></div><div>Output:</div><div><br></div><div><div> %tmp9 = fmul fast float %tmp6, 0xFFF0000000000000</div></div></div></div></div></div></div></div></div></div></div></blockquote><div><br></div><div>Oh I missed that!</div><div>Then everything is fine. The dead code is expected. I thought it was an artefact of the transformation being half done.</div><div><br></div><div>Sorry for the noise.</div></div></blockquote><div><br></div><div>Great news! It's 8pm here and I'm spent. ;) Thanks for the quick reply.</div><div><br></div><div>I'll continue to clean up PR42349 tomorrow, but for now master should be functioning as expected. </div></div></div></div>