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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added subscribers: spatel, lebedev.ri.
craig.topper 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) {
----------------
Can we pass the Width in and not make this a template function? Maybe making using of APInt to manage the width if needed?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:371
+  IntType IntWidth = sizeof(IntType);
+  IntType Bits = IntWidth * 8;
+  if (Length < Bits || Length > Bits * 2)
----------------
8 should be `CHAR_BIT`, but if we can do this without using a template function I'd rather do that.


================
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)
----------------
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.


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


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:484
+                 m_ZExtOrSelf(m_Mul(
+                     m_c_And(m_Neg(m_Value(X1)), m_Value(X2)),
+                     m_ConstantInt(MulConst))),
----------------
I think you can use m_Deferred(X1) in place of m_Value(X2), but @lebedev.ri or @spatel would know better.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:514
+  IRBuilder<> B(LI);
+  ConstantInt *BoolConst = DefinedForZero ?  B.getFalse() : B.getTrue();
+  auto Cttz = B.CreateIntrinsic(Intrinsic::cttz, {XType}, {X1, BoolConst});
----------------
`B.getInt1(!DefinedForZero);`


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


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AARCH64/lower-table-based-ctz-basics.ll:109
+
+ at ctz2.table = internal unnamed_addr constant [64 x i16] [i16 32, i16 0, i16 1, i16 12, i16 2, i16 6, i16 0, i16 13, i16 3, i16 0, i16 7, i16 0, i16 0, i16 0, i16 0, i16 14, i16 10, i16 4, i16 0, i16 0, i16 8, i16 0, i16 0, i16 25, i16 0, i16 0, i16 0, i16 0, i16 0, i16 21, i16 27, i16 15, i16 31, i16 11, i16 5, i16 0, i16 0, i16 0, i16 0, i16 0, i16 9, i16 0, i16 0, i16 24, i16 0, i16 0, i16 20, i16 26, i16 30, i16 0, i16 0, i16 0, i16 0, i16 23, i16 0, i16 19, i16 29, i16 0, i16 22, i16 18, i16 28, i16 17, i16 16, i16 0], align 2
+ at ctz3.table = internal unnamed_addr constant [32 x i32] [i32 0, i32 1, i32 2, i32 24, i32 3, i32 19, i32 6, i32 25, i32 22, i32 4, i32 20, i32 10, i32 16, i32 7, i32 12, i32 26, i32 31, i32 23, i32 18, i32 5, i32 21, i32 9, i32 15, i32 11, i32 30, i32 17, i32 8, i32 14, i32 29, i32 13, i32 28, i32 27], align 4
----------------
Can we put the tables adjacent to the function that uses them and use the update_test_checks.py script to generate the checks. I think that will make it easy to review each test case in isolation without scrolling around the file.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AARCH64/lower-table-based-ctz-basics.ll:131
+
+; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone uwtable willreturn
+define dso_local i32 @ctz2(i32 %x) local_unnamed_addr #0 {
----------------
Drop the Function Attrs comments.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AARCH64/lower-table-based-ctz-basics.ll:132
+; Function Attrs: mustprogress nofree norecurse nosync nounwind readnone uwtable willreturn
+define dso_local i32 @ctz2(i32 %x) local_unnamed_addr #0 {
+entry:
----------------
Drop dso_local and local_unnamed_addr


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AARCH64/lower-table-based-ctz-basics.ll:231
+
+attributes #0 = { mustprogress nofree norecurse nosync nounwind readnone uwtable willreturn "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" }
+attributes #1 = { mustprogress nofree norecurse nosync nounwind readonly uwtable willreturn "frame-pointer"="non-leaf" "min-legal-vector-width"="0" "no-trapping-math"="true" "stack-protector-buffer-size"="8" "target-cpu"="generic" "target-features"="+neon" }
----------------
Are these attributes needed?


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/AARCH64/lower-table-based-ctz-basics.ll:247
+!9 = !{!"omnipotent char", !10, i64 0}
+!10 = !{!"Simple C/C++ TBAA"}
+!11 = !{!12, !12, i64 0}
----------------
Most of this metadata isn't needed


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

https://reviews.llvm.org/D113291