[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:7853
+    unsigned int Lshr = DeBrujin << i;
+    unsigned int Rshr = Lshr >> 27;
+    RshrArr[Rshr] = i;
----------------
dmgreen wrote:
> I think this should technically be a rotl, because the DeBruijn sequences are cyclic. The top element in the sequence is always 0 though, so it probably doesn't make a difference in practice.
> 
> It may be better to us APInt for all the DeBrujin constants though. It is easier to be more precise with them, and it's useful when i64 constants are supported.
I can use `DAG.getNode(ISD::ROTL, dl, VT, Op, DAG.getConstant(i, dl, SHVT));` instead of `<<` but this changes variable from `unsigned int` to `SDNode`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7858-7867
+    SDValue Index = DAG.getConstant(RshrArr[i], dl, VT);
+    ConstantSDNode *IndexNode = dyn_cast<ConstantSDNode>(Index);
+    ConstantInt *temp = const_cast<ConstantInt *>(IndexNode->getConstantIntValue());
+    Constant *ConstantData =  dyn_cast<Constant >(temp);
+    Elt.push_back(ConstantData);
+  }
+  ArrayRef<Constant *> Elts = Elt;
----------------
dmgreen wrote:
> I think a lot of this may simplify into `auto *CA = ConstantDataArray::get(*DAG.getContext(), RshrArr);` or something like it. I don't think we need to create ConstantSDNode, just to get the Constant's from them.
I tried `ConstantDataArray::get(*DAG.getContext(), RshrArr)` but it does not generate table in constant pool since `RshrArr` is not array of `unsigned int` and not `Constant*`

I think there might be better alternative to simplify above. I am exploring it.


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

https://reviews.llvm.org/D128911