[PATCH] D39421: [InstCombine] Extracting common and-mask for shift operands of Or instruction
Omer Paparo Bivas via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sun Nov 26 05:19:53 PST 2017
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
More information about the llvm-commits
mailing list