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

Shubham Narlawar via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jul 21 12:57:19 PDT 2022


gsocshubham added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7856
+      unsigned int Rshr = *Lshr.getRawData() >> *ShiftAmt.getRawData();
+      RshrArr[Rshr] = i;
+    }
----------------
Regarding this line - `RshrArr[Rshr] = i;`

In debug mode, when I run `SPARC/cttz.ll` which is present in this patch, I get a segmentation fault. but in release it is working fine. 

Why is there a difference in the behaviour?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7849
+    SmallVector<uint8_t, 32> RshrArr;
+    unsigned int DeBruijn = 0x077CB531U;
+
----------------
dmgreen wrote:
> Move DeBruijn up so that it can be re-used in the creation of Lookup above.
> Perhaps add a variable for the Shift Amount too, which is 27 here but in general is BitWidth - log2(BitWdith), I think.
Done. Can you please give me some suggestions to make usage of APInt minimal and clean?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7853
+    unsigned int Lshr = DeBrujin << i;
+    unsigned int Rshr = Lshr >> 27;
+    RshrArr[Rshr] = i;
----------------
dmgreen wrote:
> gsocshubham wrote:
> > 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`.
> That seems to be mixing up the instructions we want to produce in the output (DAG.getNode) and the calculations we are doing at compile time.
> If DeBruijn is an APInt of the correct size `APInt DeBruijn(32, 0x077CB531U)`, then it will have a rotl method. The advantage of APInt is that they will also be able to work with other bitwidths, once those are added.
Understood. I have used - `APInt DeBruijn(32, 0x077CB531U)`

Is it correct way? Can you suggest a way to simplify below usage of APInt?


================
Comment at: llvm/test/CodeGen/VE/Scalar/cttz.ll:44
 ; CHECK-NEXT:    and %s0, %s0, (32)0
-; CHECK-NEXT:    pcnt %s0, %s0
 ; CHECK-NEXT:    b.l.t (, %s10)
----------------
dmgreen wrote:
> If the target has a ctpop instruction then that should be preferred to the table lookup.
I have added a check - `!isOperationLegal(ISD::CTPOP, VT)` but that does not seem to help.


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

https://reviews.llvm.org/D128911



More information about the llvm-commits mailing list