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

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
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))),
----------------
djtodoro wrote:
> dmgreen wrote:
> > 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.
> It does not happen in all the cases.
Do you have a test case where the extend is between the shift and the mul?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:493
+
+  return Matched == InputBits;
+}
----------------
If the length of the table is larger than the InputBits, how are we sure that the matched elements will be the correct ones? Is this always guaranteed?
I think I would have expected a check that `for each i=0..InputBits-1, Table[(Mul<<i)>>Shift] == i`. With a check that the index is in range. Are they always equivalent with the larger tables too?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:552
+      LI->getPointerOperand()->getType()->isOpaquePointerTy()
+          ? LI->getType()
+          : LI->getPointerOperandType()->getNonOpaquePointerElementType();
----------------
Can this always just use the Load type?


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:564
+
+  User *GEPUser = dyn_cast<User>(GEP->getPointerOperand());
+  if (!GEPUser) return false;
----------------
I think User here could be GlobalVariable


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:568
+  ConstantDataArray *ConstData =
+      dyn_cast<ConstantDataArray>(GEPUser->getOperand(0));
+  if (!ConstData) return false;
----------------
GEPUser->getOperand(0) -> Global->getInitializer(). It is worth adding a test where the global is extern.


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:590
+
+  unsigned InputBits = ConstIsWide ? 64 : 32;
+
----------------
It might be better to switch this logical around -
```
unsigned InputBits = X1->getType()->getScalarSizeInBits();
if (InputBits != 32 && InputBits != 64)
  return false;
```


================
Comment at: llvm/lib/Transforms/AggressiveInstCombine/AggressiveInstCombine.cpp:593-594
+  // Shift should extract top 5..7 bits.
+  if (InputBits - Log2_32_Ceil(InputBits) != ShiftConst &&
+      InputBits - Log2_32_Ceil(InputBits) - 1 != ShiftConst)
+    return false;
----------------
Log2_32_Ceil -> Log2_Ceil if we know the InputBits is a power of 2.

The -1 case is for a larger table with more elements but that can handle zero values?


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/lower-table-based-cttz-non-argument-value.ll:78
+  call void @llvm.lifetime.start.p0i8(i64 4, i8* nonnull %0)
+  %call = call i32 (i8*, ...) @__isoc99_scanf(i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), i32* nonnull %x)
+  %1 = load i32, i32* %x, align 4
----------------
It is better to pass x as a parameter, although I'm not sure it matter much where x comes from for the rest of the pattern.


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

https://reviews.llvm.org/D113291