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

[Chad Rosier via Phabricator via llvm-commits]

mcrosier added inline comments.


================
Comment at: lib/Transforms/Scalar/Reassociate.cpp:597-598
         }
-
+        // If this is a multiply expression, convert any left shift by a
+        // constant into a multiply so they can be reassociated.
+        if (Opcode == Instruction::Mul && BO->getOpcode() == Instruction::Shl &&
----------------
chandlerc wrote:
> I'm OK doing this temporarily if we need to, but we should have a FIXME to revisit this.
> 
> - Does this create an endless cycle between reassociate and instcombine? That seems bad
> - Can we just synthesize this by teaching the reassociation itself to magically treat shifts of this form as-if they were multiplies and transforming them as necessary on the fly?
> - Alternatively, can we add some strong assurance that if we don't reassociate, we transform everything back to a shl afterward? Maybe test cases for interesting expression trees where we put everything into multiply form and then back?
> 
> Essentially, all of this is that we somehow need this to not leak out of reassociate in the cases where we don't change anything (or we need to change our canonicalization).
Yes, reassociation relies on instcombine to undo the canonicalizations that enable reassocation performed in this pass.  While I'm likely going to abandon this patch in favor of D30228, I wanted to acknowledge that I agree with Chandler's comments.  Specifically, we should improve reassociate to only perform these type of transforms when we know reassociation will be successful.  Of course that is much easier said than done.




https://reviews.llvm.org/D29777