[PATCH] D47681: [DAGCombiner] Bug 31275- Extract a shift from a constant mul or udiv if a rotate can be formed

[Sam Conrad via Phabricator via llvm-commits]

sameconrad added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/DAGCombiner.cpp:4831-4841
+///   (or (shrl (mul v c0) c1) (mul v c2)) ->
+///   (or (shrl (mul v c0) c1) (shl (mul v c0) c3))
+//
+///   (or (udiv v c0) (shl (udiv v c1) c2)) ->
+///   (or (shrl (udiv v c1) c3) (shl (udiv v c1) c2))
+//
+///   (or (shrl (shl v c0) c1) (shl v c2)) ->
----------------
lebedev.ri wrote:
> This should be:
> ```
> //   (or (shrl (mul v c0) c1)      (mul v c2)) ->
> //   (or (shrl (mul v c0) c1) (shl (mul v c3) c4))
> //
> //   (or       (udiv v c0)     (shl (udiv v c1) c2)) ->
> //   (or (shrl (udiv v c3) c4) (shl (udiv v c1) c2))
> //
> //   (or (shrl (shl v c0) c1)      (shl v c2)) ->
> //   (or (shrl (shl v c0) c1) (shl (shl v c3) c4))
> //
> //   (or       (shrl v c0)     (shl (shrl v c1) c2)) ->
> //   (or (shrl (shrl v c1) c3) (shl (shrl v c1) c2))
> //
> // NEW:
> //   (or (shrl (shl v c0) c1)      (mul v c2)) ->
> //   (or (shrl (shl v c0) c1) (shl (mul v c3) c4))
> //
> //   (or       (shrl v c0)     (shl (div v c1) c2)) ->
> //   (or (shrl (shrl v c1) c3) (shl (div v c1) c2))
> ```
> We may not always succeed in converting `mul`/`div` into a shift,
> 
Could you clarify a bit?  The four cases shown in the original code comment were meant to be the valid expansions the function may produce, whereas the added cases don't seem to be valid expansions that could be used to form rotates.  For example in your comment the first case was changed to show the LHS of the original shift as (mul v c0) but the LHS of the new shift as (mul v c3) which seems to imply c0 != c3.  If the function encountered that it would return an empty SDValue because a rotate can't be formed if the LHS of the existing shift and the leftover LHS of the extracted shift don't match.


> (or (shrl (shl v c0) c1)      (mul v c2)) ->
> //   (or (shrl (shl v c0) c1) (shl (mul v c3) c4))

Would InstCombine would generate a case like this that would still provide a valid expansion after extracting the shift (ie where the resulting (mul v c3) is equivalent to (shl v c0))?  
In its current state this function should see that the existing shift's LHS (shl v c0) has an opcode that doesn't match the extract op (mul v c3) and just return the SDValue(), because it expects that if InstCombine had merged a shl with another shl the result should have been a greater shl rather than a mul (and thus just assumes that after extracting we couldn't have a leftover op that matches (shl v c0)).  I had assumed that if InstCombine merges a mul by a power of 2 with a shift it would produce a greater shift rather than a mul by a greater power of 2 but I would have to test that, if it doesn't and produces a mul then I could probably add some logic to check if the leftover mul can be converted to a shift after extraction to match the existing shift's LHS.


https://reviews.llvm.org/D47681