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

[Sergei Barannikov via Phabricator via llvm-commits]

barannikov88 added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7838
 
+  if (NumBitsPerElt == 32 && !VT.isVector() && !isOperationLegal(ISD::CTPOP, VT)) {
+    APInt DeBruijn(32, 0x077CB531U);
----------------
I think your code should be moved a little higher, before `SDValue Tmp = `. Otherwise the `Tmp` node will be unused if your optimization has been applied.
The check will probably need some corrections. Note the comments above, they may help deduce the correct condition.
Would also be better if you extract your implementation into separate function.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7841
+    APInt BitWidth(32, NumBitsPerElt);
+    APInt ShiftAmt(32, *(BitWidth - BitWidth.logBase2()).getRawData());
+    SDValue Neg = DAG.getNode(ISD::SUB, dl, VT, DAG.getConstant(0, dl, VT), Op);
----------------
`getRawData` is very low level method, you should be able to easily avoid it.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7844
+    SDValue Lookup = DAG.getNode(
+	ISD::SRL, dl, VT,
+	DAG.getNode(ISD::MUL, dl, VT, DAG.getNode(ISD::AND, dl, VT, Op, Neg),
----------------
Be sure to clang-format your changes before submitting a patch. This is required both by the coding style and by the contribution guidelines.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7846
+	DAG.getNode(ISD::MUL, dl, VT, DAG.getNode(ISD::AND, dl, VT, Op, Neg),
+		    DAG.getConstant(*DeBruijn.getRawData(), dl, VT)),
+	DAG.getConstant(*ShiftAmt.getRawData(), dl, VT));
----------------
Ditto, don't use `getRawData`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7850
+    // Create a table in constant data pool
+    std::vector<Constant *> Elts;
+    SmallVector<uint8_t, 32> RshrArr;
----------------
There are always exactly 32 elements, you can use plain C array and avoid dynamic memory allocation at all.
This is a bit of hard-code though, so SmallVector with 32 minimum size might be better.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7854
+    for (unsigned int i = 0; i < NumBitsPerElt; i++) {
+      APInt Lshr(32, *DeBruijn.rotl(i).getRawData());
+      unsigned int Rshr = *Lshr.getRawData() >> *ShiftAmt.getRawData();
----------------
Ditto here and on the next line. `APInt Lshr = DeBruijn.rotl(i)` should do.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7870
+    SDValue CPIdx = DAG.getConstantPool(CA, getPointerTy(TD),
+					TD.getPrefTypeAlign(Elts[0]->getType()));
+    Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
----------------
You should use the alignment requirement of the array (i.e. CA), not of its element. They may differ.


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

https://reviews.llvm.org/D128911