[PATCH] D113291: [AggressiveInstCombine] Lower Table Based CTTZ and enable it for AARCH64 in -O3
Djordje Todorovic via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Mon Nov 15 04:02:42 PST 2021
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
More information about the llvm-commits
mailing list