[PATCH] D119010: [AggressiveInstCombine] Recognize table-based ctz implementation and enable it for AARCH64 at -O3

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:442
+
+  Type *ElType = LI->getPointerOperandType()->getPointerElementType();
+  if (!ElType->isIntegerTy())
----------------
getPointerElementType() will be deleted when we move to OpaquePtr. Use LI->getType()?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:450
+
+  Type *GEPPointeeType = GEP->getPointerOperandType()->getPointerElementType();
+  if (!GEPPointeeType->isArrayTy())
----------------
GEP->getSourceElementType() I think.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:468
+  Value *Idx1 = GEP->idx_begin()->get();
+  Constant *Zero = dyn_cast<Constant>(Idx1);
+  if (!Zero || !Zero->isZeroValue())
----------------
Constant->ConstantInt? And isZeroValue->isZero


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:478
+  uint64_t MulConst, ShiftConst;
+  // FIXME: AArch64 has i64 type for the GEP index, so this match will
+  // probably fail for other targets.
----------------
What does it take to address this FIXME so this will work on other targets?


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AArch64/lower-table-based-ctz.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
----------------
Why are there less tests than the other patch?


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

https://reviews.llvm.org/D119010