[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:7795
+  APInt DeBruijn(32, 0x077CB531U);
+  APInt BitWidth(32, NumBitsPerElt);
+  APInt ShiftAmt(32, (BitWidth - BitWidth.logBase2()).getZExtValue());
----------------
dmgreen wrote:
> Sorry the suggestion for using APInt wasn't very clear. The point is that DeBruijn isn't necessarily a 32bit quantity - there are other values for 8/16/64. We should add i64 at least, so long as they look beneficial (which they should for 64, not sure about the other values). APInt makes that simpler because it can store any size without overflow, and the operations are performed on the right size.
> 
> BitWidth and ShiftAmt can remain as unsigned - the values will always easily fit in an unsigned value. ShiftWidth can be `Bitwidth - Log2_32(BitWidth)`. I would rename the "NumBitsPerElt" argument to "Bitwidth" too, to make it clear what it is and avoid the need for the two.
Here, do you mean to change `APInt DeBruijn(32, 0x077CB531U)` to `APInt DeBruijn(64, 0x0218A392CD3D5DBF)`?

`NumBitsPerElt` is 32 even in the case for `call i64 @llvm.cttz.i64(i64 %x, i1 true)`


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

https://reviews.llvm.org/D128911