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

[Chad Rosier via Phabricator via llvm-commits]

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