[PATCH] D113291: [AggressiveInstCombine] Lower Table Based CTTZ and enable it for AARCH64 in -O3

[Djordje Todorovic via Phabricator via llvm-commits]

djtodoro marked 7 inline comments as done.
djtodoro added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:367
+template <class IntType>
+static bool isCTTZTable(const ConstantDataArray &Table, IntType Mul,
+                        IntType Shift) {
----------------
craig.topper wrote:
> Can we pass the Width in and not make this a template function? Maybe making using of APInt to manage the width if needed?
OK, sure.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:447
+  GetElementPtrInst *GEP =
+      dyn_cast_or_null<GetElementPtrInst>(LI->getPointerOperand());
+  if (!GEP || !GEP->isInBounds() || GEP->getNumIndices() != 2)
----------------
craig.topper wrote:
> The _or_null here feels overly paranoid. A load won't ever have a null pointer operand will it? So dyn_cast should be ok
> 
> Same with the other dyn_cast_or_null. I don't think any of them should ever be null to start.
Agree with this.


================
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.
----------------
craig.topper wrote:
> Isn't it usually spelled AArch64 with 2 capital As?
Yep.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:519
+  // produce the value from the table.
+  auto Cmp = B.CreateICmpEQ(X1,
+                            ConstantInt::get(XType, 0));
----------------
craig.topper wrote:
> We don't need this ICmp and Select if DefinedForZero is true right?
Actually, we don't need it.


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

https://reviews.llvm.org/D113291