<div dir="ltr">FWIW, I much prefer the approach in D30223 -- it seems much more focused and matches what the surrounding code is trying to accomplish...</div><br><div class="gmail_quote"><div dir="ltr">On Tue, Feb 21, 2017 at 2:19 PM Chad Rosier via Phabricator via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">mcrosier added a comment.<br class="gmail_msg">
<br class="gmail_msg">
In <a href="https://reviews.llvm.org/D29777#682654" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D29777#682654</a>, @efriedma wrote:<br class="gmail_msg">
<br class="gmail_msg">
> 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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
The convert only occurs if the Shl is feeding a Mul or an Add.  In my test case, the Shl is feeding a negation.  <a href="https://reviews.llvm.org/D30223" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D30223</a> is an alternative solution that has the same result.<br class="gmail_msg">
<br class="gmail_msg">
> (I'd like an answer to this to make sure you aren't papering over a real problem.)<br class="gmail_msg">
<br class="gmail_msg">
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..<br class="gmail_msg">
<br class="gmail_msg">
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.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/Scalar/Reassociate.cpp:371<br class="gmail_msg">
+  Shl->replaceAllUsesWith(Mul);<br class="gmail_msg">
+  Mul->setDebugLoc(Shl->getDebugLoc());<br class="gmail_msg">
+<br class="gmail_msg">
----------------<br class="gmail_msg">
efriedma wrote:<br class="gmail_msg">
> Why don't we don't erase the dead Shl here?<br class="gmail_msg">
I believe this is to avoid an iterator invalidation bug (but I could be mistaken).<br class="gmail_msg">
<br class="gmail_msg">
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).<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D29777" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D29777</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
_______________________________________________<br class="gmail_msg">
llvm-commits mailing list<br class="gmail_msg">
<a href="mailto:llvm-commits@lists.llvm.org" class="gmail_msg" target="_blank">llvm-commits@lists.llvm.org</a><br class="gmail_msg">
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" class="gmail_msg" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br class="gmail_msg">
</blockquote></div>