[PATCH] D38037: [InstCombine] Compacting or instructions whose operands are shift instructions

[Sanjay Patel via Phabricator via llvm-commits]

spatel added reviewers: majnemer, hfinkel, davide.
spatel added a comment.

In https://reviews.llvm.org/D38037#906608, @opaparo wrote:

> In https://reviews.llvm.org/D38037#882371, @spatel wrote:
>
> > I haven't looked much at the reassociation pass, but this seems like something that would be simpler to canonicalize there. Did you look at adding a rule there?
> >
> > Eg, in the "or_or_shift" test, you have a tree of 'or' ops. If you reassociate those so that similar shifts are paired together, instcombine would manage to fold that using existing patterns (although maybe not optimally yet):
> >
> >   define i32 @or_or_shift(i32 %x) local_unnamed_addr #0  {
> >     %1 = and i32 %x, 2
> >     %2 = and i32 %x, 4
> >     %3 = shl nuw nsw i32 %1, 6
> >     %4 = lshr exact i32 %1, 1
> >     %5 = shl nuw nsw i32 %2, 6
> >     %6 = lshr exact i32 %2, 1
> >     %7 = or i32 %3, %5    <--- shl with shl
> >     %8 = or i32 %4, %6    <--- lshr with lshr
> >     %9 = or i32 %7, %8
> >     ret i32 %9
> >   }
> >  
> >
> >
> > $ ./opt -instcombine reass.ll -S
> >
> >   define i32 @or_or_shift(i32 %x) local_unnamed_addr {
> >     %1 = shl i32 %x, 6
> >     %2 = and i32 %1, 384
> >     %3 = lshr i32 %x, 1
> >     %4 = and i32 %3, 3
> >     %5 = or i32 %2, %4
> >     ret i32 %5
> >   }
> >  
> >
>
>
> I've looked into the reassociation pass a bit.
>  The kind of reassociation I'm interested in is somewhat different than the one implemented in this pass:
>  The pass takes a tree of a reassociable operator, and transforms it into a "linear tree", e.g. for addition a_0 + (a_1 + (a_2 + ... (a_n-1 + a_n))...). The tree is built such that the latter operands have lower "ranks". The ranking is such that, for example, constants have rank of zero which means they will be at the end of the tree which will cause them to eventually fold.
>  The kind of reassociation I'm looking for is different and is of the form (a_0 + a_1) + (a_2 + a_3) -> (a_0 + a_2) + (a_1 + a_3).
>  Even when I changed the pass's code such that same-opcode-instructions-tree-operands will be grouped together, I got for my //or_or_shift//, which is:
>
>   define i32 @or_or_shift(i32 %x) local_unnamed_addr #0  {
>     %1 = and i32 %x, 2
>     %2 = and i32 %x, 4
>     %3 = shl nuw nsw i32 %1, 6
>     %4 = lshr exact i32 %1, 1
>     %5 = or i32 %3, %4
>     %6 = shl nuw nsw i32 %2, 6
>     %7 = lshr exact i32 %2, 1
>     %8 = or i32 %6, %7
>     %9 = or i32 %5, %8
>     ret i32 %9
>   }
>
>
> This output:
>
>   define i32 @or_or_shift(i32 %x) local_unnamed_addr {
>     %1 = and i32 %x, 2
>     %2 = and i32 %x, 4
>     %3 = shl nuw nsw i32 %1, 6
>     %4 = lshr exact i32 %1, 1
>     %5 = shl nuw nsw i32 %2, 6
>     %6 = lshr exact i32 %2, 1
>     %7 = or i32 %6, %4
>     %8 = or i32 %7, %3
>     %9 = or i32 %8, %5
>     ret i32 %9
>   }
>
>
> //%7//, being the leaf or in the tree, actually does InstCombine nicely. The rest, however, do not.
>  As I see it, if I wish to embed this change in the reassociation pass, I have two options:
>
> 1. Change the reassociation pass code such that the tree's topology will fit my needs.
> 2. Add more InstCombine code to handle the newly formed structure (Something like //(or (or A (op B C1)) (op D C1)) --> (or A (or (op B C1) (op D C1)))//, which is still a kind of reassociation).
>
>   I strongly prefer not to change the reassociation pass's tree topology as I have a feeling it is greatly relied on. And if I add a reassociation to InstCombine, why not add my original one instead of adding that one in addition to the changes in the reassociation pass?
>
>   Please let me know your thoughts on this matter.


Thanks for looking at and explaining Reassociate's current behavior. I don't know what would be best, so cc'ing some others that might have a better idea.


https://reviews.llvm.org/D38037