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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7968
+                  DAG.getConstant(DeBruijn.getZExtValue(), dl, VT)),
+      DAG.getConstant(ShiftAmt, dl, VT));
+
----------------
This should be getShiftAmountConstant.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7978
+      cast<ConstantSDNode>(DAG.getConstant(RshrArr[0], dl, VT));
+  ConstantInt *CI = const_cast<ConstantInt *>(Index->getConstantIntValue());
+
----------------
We shouldn't need a ConstantSDNode and ConstantInt to get the Type*. You can use `VT.getTypeForEVT(*DAG.getContext())` in place of `CI->getType()`


Though really we should be using an array of Int8Ty and doing a zextload from i8 to VT.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:8025
+  // Emit Table Lookup if ISD::CTLZ and ISD::CTPOP are not legal.
+  if (NumBitsPerElt == 32 && !VT.isVector() &&
+      TLI.isOperationExpand(ISD::CTPOP, VT) && !isOperationLegal(ISD::CTLZ, VT))
----------------
Should this be checking CTTZ_ZERO_UNDEF? The zero case is not handled correctly by the table lookup. For CTTZ we need a select. CodeGenPrepare rewrites llvm.cttz(i32 %x, i1 false) into a branch around llvm.cttz(i32 %x, i1 true) on some targets. So the difference might be hard to test.


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

https://reviews.llvm.org/D128911