[PATCH] D123782: [AArch64] Generate AND in place of CSEL for Table Based CTTZ lowering in -O3

[Rahul via Phabricator via llvm-commits]

rahular-rrlogic added a comment.

In D123782#3451501 <https://reviews.llvm.org/D123782#3451501>, @dmgreen wrote:

> Can you add some tests?
>
> As far as I understand this should be testing for `select (icmp eq X, 0), 0, cttz X` and converting it to `and(cttz X, #bw-1)`. If so there are more conditions that need to be checked.

What other conditions do you think should be added other than checking for 0 and cttz?

In D123782#3451500 <https://reviews.llvm.org/D123782#3451500>, @djtodoro wrote:

> Thanks for this! Please add test.

I will add test with the changes



================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17460
+  SDValue N2 = N->getOperand(2);
+  bool isZero = cast<ConstantSDNode>(N2.getNode())->isZero();
+  if (N1.getOpcode() == ISD::CTTZ && isZero) {
----------------
dmgreen wrote:
> As far as I can tell this is checking that the condition code is 0? It would be better to check that it is equal to AArch64CC::EQ.
No, this is a mistake. I was intending to check if the operands are 0 and cttz. I will change that. Is a check for the condition being EQ really required, though?


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:17462
+  if (N1.getOpcode() == ISD::CTTZ && isZero) {
+    SDValue thirtyOne = DAG.getConstant(31, SDLoc(N), N1.getValueType());
+    return DAG.getNode(ISD::AND, SDLoc(N), N1.getValueType(), N1, thirtyOne);
----------------
dmgreen wrote:
> Variables in llvm start with capital letters. We should make sure that i64 work too, it needs a different constant (there should be tests too).
I will do the capitalization and add support for i64


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D123782/new/

https://reviews.llvm.org/D123782