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

Dave Green via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jul 8 08:41:59 PDT 2022


dmgreen added a comment.

Remember to add Context to the patch. It makes it much easier to read.

There are some existing tests for various targets, I think many of which will change with this patch. They can usually be updated with the update scripts, and we can check they look like better sequences of instructions. There are Thumb1 tests in llvm/test/CodeGen/ARM/cttz.ll for example. With this being in expandCTTZ they may only test the i32 path though, as other types will already have been legalized.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7840
+  SDValue Neg = DAG.getNode(ISD::SUB, dl, VT, DAG.getConstant(0, dl, VT), Op);
+  SDValue Lookup = DAG.getNode(
+      ISD::SRL, dl, VT,
----------------
This needs to be in a `if (NumBitsPerElt == 32 && !VT.isVector())` block. Hopefully with other types available too. Else it would fall back to the existing CTPop. If the CTPOP is legal it is worth using it too, instead of this table lookup.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7844
+                  DAG.getConstant(0x077CB531U, dl, VT)),
+      DAG.getConstant(27, dl, VT));
+
----------------
I think that some other DeBruijn constants would be:
i8: 0x17
i16: 0x09AF
i64: 0x0218A392CD3D5DBF
Hopefully I didn't get those wrong. The i64 almost certainly better than the alternative, but I haven't looked at whether the other two are useful.


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


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


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7853
+    unsigned int Lshr = DeBrujin << i;
+    unsigned int Rshr = Lshr >> 27;
+    RshrArr[Rshr] = i;
----------------
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.


================
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;
----------------
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.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7876
+  return DAG.getLoad(VT, dl, Lookup, CPIdx,
+                     MachinePointerInfo::getConstantPool(
+                         DAG.getMachineFunction()), Alignment);
----------------
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.


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

https://reviews.llvm.org/D128911



More information about the llvm-commits mailing list