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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7960
+                                        long unsigned BitWidth) const {
+  if (BitWidth == 8 || BitWidth == 16)
+    return SDValue();
----------------
This needs to handle other types too - just doing `if (BitWidth != 32 && BitWidth != 64)` is probably easiest.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7976
+
+  SmallVector<uint16_t> Table(BitWidth, 0);
+  for (long unsigned i = 0; i < BitWidth; i++) {
----------------
Why has this become uint16_t? I don't think it needs to be any bigger than i8 to hold all the values.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7989
+  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+  SDValue Load = DAG.getLoad(VT, dl, DAG.getEntryNode(),
+                             DAG.getMemBasePlusOffset(CPIdx, Lookup, dl),
----------------
It doesn't need to create a load just to create another load. It can just use the getExtLoad method with the MemoryVT set to MVT::i8. I think the other parameters can be default/left out.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7998
+                     LD->getMemoryVT(), Alignment, MMOFlags, LD->getAAInfo());
+  if (isOperationLegalOrCustom(ISD::CTTZ_ZERO_UNDEF, VT)) {
+    EVT SetCCVT =
----------------
gsocshubham wrote:
> @craig.topper - What should be the check here for case `llvm.cttz(i32 %x, i1 false)`?
> 
> It can't be `isOperationLegalOrCustom(ISD::CTTZ_ZERO_UNDEF, VT)` because that is taken care before this new lowering.
I think, if I understand, this should be `if (Node->getOpcode() != ISD::CTLZ_ZERO_UNDEF) {`
But I'm not sure what this does to the profitability of the transform.

It might be possible to just encode it into the table. It involves changing how the table to have bitwidth+1 elements, and using a different debruijn constant so that the zero element can be the bitwidth.


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

https://reviews.llvm.org/D128911