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

[Owen Anderson]

Patch 1 LGTM.

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

—Owen

On Dec 23, 2013, at 7:24 AM, Richard Sandiford <rsandifo at linux.vnet.ibm.com> wrote:

> Kay Tiong Khoo <kkhoo at perfwizard.com> writes:
>> Hi Richard,
>> 
>> Could this patch be enhanced to recognize this case too:
>> http://llvm.org/bugs/show_bug.cgi?id=17332#c12
>> 
>> Maybe it already does? Sorry, I can't test it myself at the moment.
> 
> Hi, thanks for the pointer.  The patch didn't handle that case but I agree
> it'd be a good thing to add.
> 
> Here's a version that does both.  I've split it into three patches:
> 
> (1) is just a refactoring exercise.  It's easier to make changes to this
>    code if there's only one copy of it.  No behavioural change intended.
> 
> (2) is my original patch adjusted for (1).  E.g. we now recognise:
>       (or (shl X, (add Y, 10)), (shr X, (sub 22, Y)))
>    as a rotate by Y + 10.
> 
> (3) also handles cases where the shift amount is ANDed.  E.g. we now
>    recognise:
>       (or (shl X, Y), (shr X, (and (sub 0, Y), 31)))
>       (or (shl X, Y), (shr X, (and (sub 32, Y), 31)))
>    as rotates by Y.  We can also handle redundant ANDs for (2) too:
>       (or (shl X, (add Y, 10)), (shr X, (and (sub 22, Y), 31)))
> 
> OK to commit?
> 
> Thanks,
> Richard
> 
> 
> <rotate-1.diff><rotate-2.diff><rotate-3.diff>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits