[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:7970
+      DAG.getShiftAmountConstant(ShiftAmt, VT, dl));
+  SDValue TargetLookup = DAG.getSExtOrTrunc(Lookup, dl, VT);
+
----------------
You overwrite `Lookup` here instead of creating `TargetLookup`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7983
+      CA, getPointerTy(TD),
+      TD.getPrefTypeAlign(VT.getTypeForEVT(*DAG.getContext())));
+  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
----------------
The ConstantDataArray is an array of bytes, but the alignment here is based on VT. Should be MVT::i8


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7986
+  SDValue Load = DAG.getLoad(
+      VT, dl, DAG.getEntryNode(),
+      DAG.getMemBasePlusOffset(CPIdx, TargetLookup, dl),
----------------
The data in the array is only 8-bits. The load type shouldn't be VT.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7990
+  SDValue ZExtOrTrunc = DAG.getZExtOrTrunc(Load, dl, VT);
+  EVT NewVT = ZExtOrTrunc->getValueType(0);
+  if (isTypeLegal(NewVT))
----------------
NewVT is the same as VT.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7994
+  else {
+    LoadSDNode *LD = cast<LoadSDNode>(ZExtOrTrunc);
+    return DAG.getExtLoad(
----------------
ZExtOrTrunc and Load are unused if this else is taken. We shouldn't create dead nodes if it can be avoided.


================
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))
----------------
gsocshubham wrote:
> craig.topper wrote:
> > 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.
> Can you point me to target and test where above scenario occurs? I will update it accordingly.
Compiling llvm.cttz.i32 for riscv32 with -O0 should do it I think. -O0 will disable codegen prepare.


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

https://reviews.llvm.org/D128911