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




================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7983
+      CA, getPointerTy(TD),
+      TD.getPrefTypeAlign(VT.getTypeForEVT(*DAG.getContext())));
+  Align Alignment = cast<ConstantPoolSDNode>(CPIdx)->getAlign();
----------------
craig.topper wrote:
> The ConstantDataArray is an array of bytes, but the alignment here is based on VT. Should be MVT::i8
I tried using `MVT::i8` by converting it to Value Type to be used instead of VT but there seems some compatibility issue.


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


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


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

https://reviews.llvm.org/D128911