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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

When we get that far, we might need to add a way for targets to opt out of the new lowering if it will cause them problems.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7795
+  APInt DeBruijn(32, 0x077CB531U);
+  APInt BitWidth(32, NumBitsPerElt);
+  APInt ShiftAmt(32, (BitWidth - BitWidth.logBase2()).getZExtValue());
----------------
Sorry the suggestion for using APInt wasn't very clear. The point is that DeBruijn isn't necessarily a 32bit quantity - there are other values for 8/16/64. We should add i64 at least, so long as they look beneficial (which they should for 64, not sure about the other values). APInt makes that simpler because it can store any size without overflow, and the operations are performed on the right size.

BitWidth and ShiftAmt can remain as unsigned - the values will always easily fit in an unsigned value. ShiftWidth can be `Bitwidth - Log2_32(BitWidth)`. I would rename the "NumBitsPerElt" argument to "Bitwidth" too, to make it clear what it is and avoid the need for the two.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7801
+      DAG.getNode(ISD::MUL, dl, VT, DAG.getNode(ISD::AND, dl, VT, Op, Neg),
+                  DAG.getConstant(DeBruijn.getZExtValue(), dl, VT)),
+      DAG.getConstant(ShiftAmt.getZExtValue(), dl, VT));
----------------
I think we can make a constant from a APInt directly.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7806
+  std::vector<Constant *> Elts;
+  uint8_t RshrArr[32];
+
----------------
This shouldn't be a plain C array. It's size is dependant on the BitWidth. `SmallVector<uint8_t> RshrArr(BitWidth, 0)` should create an array that is initialized to 0's with the correct size.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7810
+    APInt Lshr = DeBruijn.rotl(i);
+    unsigned int Rshr = Lshr.getZExtValue() >> ShiftAmt.getZExtValue();
+    RshrArr[Rshr] = i;
----------------
`APInt Rshr = Lshr.lshr(ShiftAmt)`, then use `Rshr.getZExtValue()` in the line below. It is a little strange to use getZExtValue in an array index, but so long as the array is a safe type, it should complain if the value is out of bounds.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7814-7819
+  for (unsigned int i = 0; i < NumBitsPerElt; i++) {
+    SDValue Index = DAG.getConstant(RshrArr[i], dl, VT);
+    ConstantSDNode *IndexNode = cast<ConstantSDNode>(Index);
+    ConstantInt *CI =
+        const_cast<ConstantInt *>(IndexNode->getConstantIntValue());
+    Elts.push_back(CI);
----------------
Do we need this loop, or can we create the array from the constant pool directly? The elements should be MVT::i8.
```
auto *CA = ConstantDataArray::get(*DAG.getContext(), RshrArr);
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7829
+  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
+  return DAG.getLoad(
+      VT, dl, DAG.getEntryNode(), DAG.getMemBasePlusOffset(CPIdx, Lookup, dl),
----------------
The load should be loading MVT::i8, extended the result into VT.


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

https://reviews.llvm.org/D128911