Ping 2: [PATCH] DAGCombiner: Recognise rotates by X+C

[Richard Sandiford]

Thanks for the reviews.  Committed as r198860 and r198861.

Richard

Owen Anderson <resistor at mac.com> writes:
> Looks great.  IMO it’s always better to over-document these kinds of
> fiddly transforms.  If they’re wrong somehow, they can be a real pain to
> track down, so having a good explanation of the intended functionality
> is incredibly useful.
>
> —Owen
>
> On Jan 8, 2014, at 7:52 AM, Richard Sandiford
> <rsandifo at linux.vnet.ibm.com> wrote:
>
>> Owen Anderson <resistor at mac.com> writes:
>>> Patch 1 LGTM.
>> 
>> r198768, thanks.
>> 
>>> Patch 2:
>>> 
>>>> +// Check whether Neg == OpSize - Pos.
>>> 
>>> Please add a more explanatory function comment.
>>> 
>>>> +  // Check for cases where Pos = NegOp1 + PosC and where:
>>>> +  //    Neg = OpSize - Pos
>>>> +  //        = OpSize - (NegOp1 + PosC)
>>>> +  //        = (OpSize - PosC) - NegOp1
>>>> +  //   NegC = OpSize - PosC
>>>> +  // OpSize = NegC + PosC
>>> 
>>> I think this comment could do with an s-expression-style example of the
>>> overall pattern you’re trying to match.
>>> 
>>> Patch 3:
>>> 
>>>> +  // Remove a mask of OpSize - 1.
>>>> +  unsigned LoBits = 0;
>>>> +  if (Neg.getOpcode() == ISD::AND &&
>>>> +      isPowerOf2_64(OpSize) &&
>>>> +      Neg.getOperand(1).getOpcode() == ISD::Constant &&
>>>> + cast<ConstantSDNode>(Neg.getOperand(1))->getAPIntValue() == OpSize -
>>>> 1) {
>>>> +    Neg = Neg.getOperand(0);
>>>> +    LoBits = Log2_64(OpSize);
>>>> +  }
>>>> +
>>> 
>>> Again, please explain the logic here.
>>> 
>>>> +  if (LoBits)
>>>> +    return Width.getLoBits(LoBits) == 0;
>>>> +  return Width == OpSize;
>>> 
>>> ENOCOMMENT
>> 
>> OK, here's patches 2 and 3 with extra (and in a couple of cases, fixed)
>> commentary.  Hope this isn't going too far the other way.
>> 
>> Thanks,
>> Richard
>> 
>> <rotate-2.diff><rotate-3.diff>