[PATCH] D128911: Emit table lookup from TargetLowering::expandCTTZ()

Mon Jul 18 02:51:49 PDT 2022

dmgreen added inline comments.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7853
+    unsigned int Lshr = DeBrujin << i;
+    unsigned int Rshr = Lshr >> 27;
+    RshrArr[Rshr] = i;
----------------
gsocshubham wrote:
> dmgreen wrote:
> > I think this should technically be a rotl, because the DeBruijn sequences are cyclic. The top element in the sequence is always 0 though, so it probably doesn't make a difference in practice.
> > 
> > It may be better to us APInt for all the DeBrujin constants though. It is easier to be more precise with them, and it's useful when i64 constants are supported.
> I can use `DAG.getNode(ISD::ROTL, dl, VT, Op, DAG.getConstant(i, dl, SHVT));` instead of `<<` but this changes variable from `unsigned int` to `SDNode`.
That seems to be mixing up the instructions we want to produce in the output (DAG.getNode) and the calculations we are doing at compile time.
If DeBruijn is an APInt of the correct size `APInt DeBruijn(32, 0x077CB531U)`, then it will have a rotl method. The advantage of APInt is that they will also be able to work with other bitwidths, once those are added.

================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7849
+    SmallVector<uint8_t, 32> RshrArr;
+    unsigned int DeBruijn = 0x077CB531U;
+
----------------
Move DeBruijn up so that it can be re-used in the creation of Lookup above.
Perhaps add a variable for the Shift Amount too, which is 27 here but in general is BitWidth - log2(BitWdith), I think.

================
Comment at: llvm/test/CodeGen/VE/Scalar/cttz.ll:44
 ; CHECK-NEXT:    and %s0, %s0, (32)0
-; CHECK-NEXT:    pcnt %s0, %s0
 ; CHECK-NEXT:    b.l.t (, %s10)
----------------
If the target has a ctpop instruction then that should be preferred to the table lookup.

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

https://reviews.llvm.org/D128911