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

[Shubham Narlawar via Phabricator via llvm-commits]

gsocshubham marked an inline comment as not done.
gsocshubham added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7849
+  unsigned int RshrArr[NumBitsPerElt];
+  unsigned int DeBrujin = 0x077CB531U;
+
----------------
dmgreen wrote:
> Use a SmallVector for the constant values, probably with uint8_t elements.
Done.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7850
+  unsigned int DeBrujin = 0x077CB531U;
+
+  for (unsigned int i = 0; i < NumBitsPerElt; i++) {
----------------
dmgreen wrote:
> "DeBruijn"
Done.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7860
+    ConstantSDNode *IndexNode = dyn_cast<ConstantSDNode>(Index);
+    ConstantInt *temp = const_cast<ConstantInt *>(IndexNode->getConstantIntValue());
+    Constant *ConstantData =  dyn_cast<Constant >(temp);
----------------
craig.topper wrote:
> `dyn_cast` should only be used if the cast can fail. Otherwise use `cast`
Understood. Changed it to `cast`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7862
+    Constant *ConstantData =  dyn_cast<Constant >(temp);
+    Elt.push_back(ConstantData);
+  }
----------------
craig.topper wrote:
> Same comment about `dyn_cast`
Changed it to `cast`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7865
+  ArrayRef<Constant *> Elts = Elt;
+  auto *FPTy = Elts[0]->getType();
+  const DataLayout &TD = DAG.getDataLayout();
----------------
craig.topper wrote:
> Why do we need to create an explicit ArrayRef here?
Not required. I have removed it.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7876
+  return DAG.getLoad(VT, dl, Lookup, CPIdx,
+                     MachinePointerInfo::getConstantPool(
+                         DAG.getMachineFunction()), Alignment);
----------------
dmgreen wrote:
> I think this should be an extload from the i8 elements in the table to the original VT.
> Chain can be DAG.getEntryNode(), and the Ptr we load from needs to be CPIdx + Lookup.
Done. Please check it.


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

https://reviews.llvm.org/D128911