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

[Eli Friedman via Phabricator via llvm-commits]

efriedma 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;
----------------
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.


https://reviews.llvm.org/D30228