[PATCH] D39421: [InstCombine] Extracting common and-mask for shift operands of Or instruction

[Omer Paparo Bivas via Phabricator via llvm-commits]

opaparo added a comment.

In https://reviews.llvm.org/D39421#923308, @spatel wrote:

> Why do we canonicalize shift-left before 'and'?
>
> Ie, shouldn't we prefer this:
>
>   define i8 @andshl(i8 %x) {
>     %and = and i8 %x, 1
>     %shl = shl i8 %and, 3
>     ret i8 %shl
>   }
>  
>
>
> instead of this:
>
>   define i8 @andshl(i8 %x) {
>     %and = shl i8 %x, 3
>     %shl = and i8 %and, 8
>     ret i8 %shl
>   }
>  
>
>
> ...because doing the 'and' before the shift always uses a smaller constant?


Hi,
Sorry for the delayed response.

I'm not sure I understand why the suggested canonization might simplify or obviate the need for this patch.
Consider my use case "multiuse3". Although InstCombine normally recognizes and canonize something of the form

  %1 = and i32 %x, 96
  %2 = shl nuw nsw i32 %1, 6

It will not in this case as '%1' has more than one use, and having one use is a condition for this transformation. If my transformation wouldn't consider the non-canonical in addition to the canonical form it could not handle this case.
One may argue that changing the canonical form will yield this code:

  %1 = and i32 %x, 96
  %2 = shl nuw nsw i32 %1, 6
  %3 = lshr exact i32 %1, 1
  %4 = and i32 %x, 30
  %5 = shl nuw nsw i32 %4, 6
  %6 = or i32 %2, %5
  %7 = lshr exact i32 %4, 1
  %8 = or i32 %3, %7
  %9 = or i32 %8, %6
  ret i32 %9

Which will then simplify the patch as I will not be required to consider the non-canonical form. However:

1. This will be true only if the two shifts are shl. In this example one of them is a lshr, so the transformation will not actually happen and the non-canonical form still needs to be considered.
2. This canonization will only occur if the intermediate results, i.e. "%4 = shl i32 %x, 6" and "%7 = lshr i32 %x, 1" have only one use. Suppose the scenario was a bit different and those values were used somewhere along the road. In this case the canonization would not happen and again I'll have to consider the non-canonical form.

As far as I understand this is the way that the suggested canonization might simplify or obviate the need for this patch. If you meant something else, could you please elaborate?


https://reviews.llvm.org/D39421