[PATCH] DAGCombiner: Add another rotate pattern.

[Jay Foad]

On 18 July 2013 13:50, David Majnemer <david.majnemer at gmail.com> wrote:
> On Thu, Jul 18, 2013 at 4:40 AM, Jay Foad <jay.foad at gmail.com> wrote:
>>
>> > +    // fold (or (shl x, (*ext y)), (srl x, (*ext (and (sub 32, y),
>> > 31)))) ->
>> > +    //   (rotl x, y)
>> > +    // fold (or (shl x, (*ext y)), (srl x, (*ext (and (sub 32, y),
>> > 31)))) ->
>> > +    //   (rotr x, (sub 32, y))
>>
>> "and (sub 32, y), 31" should be "and (sub 0, y), 31". There are four
>> occurrences of this in the whole patch.
>
>
> Sorry about that, the comment is fixed in the attached patch.
>
>>
>>
>> > +          if (SUBC->getAPIntValue() == 0 &&
>> > +              ANDC->getAPIntValue() == OpSizeInBits - 1 &&
>> > +              RExtOp0.getOperand(0).getOperand(1) == LExtOp0)
>>
>> Do you need an explicit check that OpSizeInBits is a power of two?
>
>
> Good catch, we do indeed need such a check, fixed in the attached patch.

I'm not familiar with the DAG combiner, but the logic in your patch now LGTM.

For further improvements, would it be possible to run the test case on
more targets than just X86? And could your new transformation also be
applied to the non-sign/zero/any-extend/truncate part of MatchRotate
(i.e. the cases starting at line 3346) ?

Jay.