[PATCH] D29777: [Reassociate] Convert shl by constant into multiply during tree linearization.

[Chandler Carruth via llvm-commits]

FWIW, I much prefer the approach in D30223 -- it seems much more focused
and matches what the surrounding code is trying to accomplish...

On Tue, Feb 21, 2017 at 2:19 PM Chad Rosier via Phabricator via
llvm-commits <llvm-commits at lists.llvm.org> wrote:

> mcrosier added a comment.
>
> In https://reviews.llvm.org/D29777#682654, @efriedma wrote:
>
> > I'm not following why this fixes the crash.  As far as I can tell, on
> trunk, Reassociate special-cases shl instructions in exactly one place: the
> check at the beginning of ReassociatePass::OptimizeInst.  In that case, we
> replace all the uses of the shl instruction, so it shouldn't matter.
>
>
> The convert only occurs if the Shl is feeding a Mul or an Add.  In my test
> case, the Shl is feeding a negation.  https://reviews.llvm.org/D30223 is
> an alternative solution that has the same result.
>
> > (I'd like an answer to this to make sure you aren't papering over a real
> problem.)
>
> This fixes PR30256 by avoiding the particular case that is causing the
> assert.  However, I think the underlying issue is that if a factor is a
> negative constant, we consider the positive value as a factor as well
> (because we can percolate the negate out).  Thus, I think this patch does
> kinda papers over the issue, but I can also argue it's a reasonable
> optimization..
>
> I think I've just come up with a patch that can more directly address the
> underlying issue..  Let me post that before we continue the review here.
>
>
>
> ================
> Comment at: lib/Transforms/Scalar/Reassociate.cpp:371
> +  Shl->replaceAllUsesWith(Mul);
> +  Mul->setDebugLoc(Shl->getDebugLoc());
> +
> ----------------
> efriedma wrote:
> > Why don't we don't erase the dead Shl here?
> I believe this is to avoid an iterator invalidation bug (but I could be
> mistaken).
>
> In the current code, the Shl is inserted into the RedoInsts list after the
> call to ConvertShiftToMul.  This will ensure the Shl is removed as dead
> code.  I should do the same for my code (or alternatively pass in the
> RedoInsts into ConvertShiftToMul and insert the Shl into RedoInsts directly
> in CovertShiftToMul).
>
>
> https://reviews.llvm.org/D29777
>
>
>
> _______________________________________________
> 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/20170221/cedfef98/attachment.html>