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

[Owen Anderson]

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>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140108/f2d223e5/attachment.html>