[PATCH] D113291: [AggressiveInstCombine] Lower Table Based CTTZ

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

In D113291#3670999 <https://reviews.llvm.org/D113291#3670999>, @gsocshubham wrote:

> Is this patch ready to be merged? This is parent patch of `https://reviews.llvm.org/D128911`. Does child get merged before parent?
>
> Anyways, `https://reviews.llvm.org/D128911` can be merged independently of this patch.

D128911 <https://reviews.llvm.org/D128911> needs to go in first, once that is done we can move forward with this one. It could do with a rebase and a clang-format.



================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:479
+  uint64_t MulConst, ShiftConst;
+  // FIXME: Aarch64 has i64 type for the GEP index, so this match will
+  // probably fail for other targets.
----------------
djtodoro wrote:
> craig.topper wrote:
> > Isn't it usually spelled AArch64 with 2 capital As?
> Yep.
It's probably better to just say "64bit targets" as opposed to a specific target.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:443
+
+  // TODO: Support opaque pointers.
+  Type *PtrTy = LI->getPointerOperand()->getType();
----------------
We will need to support opaque pointers nowadays.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:485
+             m_ZExtOrSelf(m_LShr(
+                 m_ZExtOrSelf(m_Mul(m_c_And(m_Neg(m_Value(X1)), m_Deferred(X1)),
+                                    m_ConstantInt(MulConst))),
----------------
Does the extend between the lshr and the mul every happen? From what I can tell, the type of the VT should be based on the type of these operations.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:493
+  // Shift should extract top 5..7 bits.
+  if (ShiftConst < InputBits - 7 || ShiftConst > InputBits - 5) return false;
+
----------------
I believe it's the top `Bitwidth - Log2(Bitwidth)` bits.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:577
   AU.addRequired<DominatorTreeWrapperPass>();
+  AU.addRequired<TargetTransformInfoWrapperPass>();
   AU.addRequired<TargetLibraryInfoWrapperPass>();
----------------
The TTI's can be removed now (and if you rebase they may already be present, but still are not needed by the new code any more).


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AArch64/lower-table-based-ctz-basics.ll:1
+; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
+; RUN: opt -aggressive-instcombine -mtriple aarch64-linux-gnu -S < %s | FileCheck %s
----------------
The tests can be moved out of the AArch64 directory, so long as they drop the AArch64 triple.


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

https://reviews.llvm.org/D113291