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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Some longer term comments here, but all of this seems reasonably in-line with how we currently do things in reassociate, but raises some long-standing issues in a potentially more concerning area.

I also think Eli's comment is important.



================
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 &&
----------------
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).


https://reviews.llvm.org/D29777