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

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham 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);
----------------
barannikov88 wrote:
> 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.
> 
Moved code above `SDValue Tmp = ` and added into a separate function.

I am using below check to do the optimization -

```if (NumBitsPerElt == 32 && !VT.isVector() &&
      TLI.isOperationExpand(ISD::CTPOP, VT) && !isOperationLegal(ISD::CTLZ, VT))```


================
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);
----------------
barannikov88 wrote:
> `getRawData` is very low level method, you should be able to easily avoid it.
> 
Done. Now I am using - `getZExtValue()`


================
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),
----------------
barannikov88 wrote:
> Be sure to clang-format your changes before submitting a patch. This is required both by the coding style and by the contribution guidelines.
I have formatted code using -

`$INSTALL/bin/clang-format -style=LLVM TargetLowering.cpp -i --lines=7866:7872`


================
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));
----------------
barannikov88 wrote:
> Ditto, don't use `getRawData`.
Updated to `getZExtValue()`


================
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;
----------------
barannikov88 wrote:
> 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.
> 
I am using plain C array and got rid of dynamic memory allocation error.


================
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();
----------------
barannikov88 wrote:
> Ditto here and on the next line. `APInt Lshr = DeBruijn.rotl(i)` should do.
Done.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7856
+      unsigned int Rshr = *Lshr.getRawData() >> *ShiftAmt.getRawData();
+      RshrArr[Rshr] = i;
+    }
----------------
barannikov88 wrote:
> gsocshubham wrote:
> > Regarding this line - `RshrArr[Rshr] = i;`
> > 
> > In debug mode, when I run `SPARC/cttz.ll` which is present in this patch, I get a segmentation fault. but in release it is working fine. 
> > 
> > Why is there a difference in the behaviour?
> Most probably there is a bug in your code. Most probably you haven't enabled assertions in release mode (-DLLVM_ENABLE_ASSERTIONS=ON is the default in debug mode).
> You should be able to find some hints in the printed backtrace, but first make sure that `llvm-symbolizer` target is built.
> 
Thanks. `-DLLVM_ENABLE_ASSERTIONS=ON` this helped.


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

https://reviews.llvm.org/D128911