[PATCH] D30228: [Reassociate] Add negated value of negative constant to the Duplicates list.

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added a subscriber: gberry.
mcrosier added inline comments.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:1523
           Factor = ConstantInt::get(CI->getContext(), -CI->getValue());
-          assert(!Duplicates.count(Factor) &&
-                 "Shouldn't have two constant factors, missed a canonicalize");
+          if (!Duplicates.insert(Factor).second)
+            continue;
----------------
efriedma wrote:
> Okay, now I understand the issue: the problem is that this code is assuming that all the multiply operands of the add are already reassociated.  We break that assumption with the way we optimize shl->mul: we transform the shl into a mul, and stick the mul into RedoInsts, but don't revisit it until it's "too late".
> 
> Your first two patches dance around the issue in slightly different ways; basically, you dodge the issue by changing the visitation order.  This seems like a landmine, even if it does fix the immediate problem.
> 
> This patch avoids the issue by making OptimizeAdd tolerate multiplies which haven't been completely optimized; this sort of works, but we're doing wasted work: we'll end up revisiting the add later anyway.
> 
> Another possible approach would be to enforce RPO iteration order more strongly.  If we have RedoInsts, we process them immediately in RPO order, rather than waiting until we've finished processing the whole function.  Intuitively, it seems like the natural approach: reassociation works on expression trees, so the optimization only works in one direction.  That said, I'm not sure how practical that is given the current Reassociate; the "optimal" form for an expression depends on its use list (see all the uses of "user_back()"), so Reassociate is really an iterative optimization of sorts, so any changes here would probably get messy.
Yes, this is a good synopsis of what is going on and I agree my previous two approaches (D29777 and D30223) skate around the issue.  You're also correct in that this patch results in wasted work (which is pervasive throughout this pass, unfortunately), but I don't think the RPO suggestion is practical for fixing this issue for the 4.0 release.

Would you be okay accepting this patch?



https://reviews.llvm.org/D30228