[PATCH] D48236: [IR] move shuffle mask queries from TTI to ShuffleVectorInst

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: include/llvm/IR/Instructions.h:2467
+  /// input vectors or it may provide demanded bits knowledge via undef lanes.
+  /// Example: <-1,1,-1,3>
+  static bool isIdentityMask(ArrayRef<int> Mask);
----------------
i think it will be less confusing to replace `-1` with `undef`.


================
Comment at: include/llvm/IR/Instructions.h:2471
+    assert(Mask->getType()->isVectorTy() && "Shuffle needs vector constant.");
+    SmallVector<int, 16> MaskAsInts;
+    getShuffleMask(Mask, MaskAsInts);
----------------
>From where does this 16 come from?
Magical expected maximal number of elements in one vector? (AVX512 / 16 elements = 32 bit/element)
Might be better to `static constexpr int` it beforehand.


================
Comment at: include/llvm/IR/Instructions.h:2487
+  /// equivalent to a vector select with a constant condition operand.
+  /// Example: <4,1,6,-1>
+  /// This returns false if the mask does not choose from both input vectors.
----------------
i think it will be less confusing to replace `-1` with `undef`.


================
Comment at: include/llvm/IR/Instructions.h:2513
+  /// one source vector.
+  /// Example: <7,6,-1,4>
+  /// This assumes that vector operands are the same length as the mask.
----------------
i think it will be less confusing to replace `-1` with `undef`.


================
Comment at: include/llvm/IR/Instructions.h:2532
+  /// vector.
+  /// Example: <7,5,-1,7>
+  /// This assumes that vector operands are the same length as the mask.
----------------
i think it will be less confusing to replace `-1` with `undef`.


================
Comment at: include/llvm/IR/Instructions.h:2551
+  /// as the first element of exactly one source vector.
+  /// Example: <4,-1,-1,4>
+  /// This assumes that vector operands are the same length as the mask.
----------------
i think it will be less confusing to replace `-1` with `undef`.


================
Comment at: lib/IR/Instructions.cpp:1726-1743
+bool ShuffleVectorInst::isReverseMask(ArrayRef<int> Mask) {
+  bool UsesLHS = true;
+  bool UsesRHS = true;
+  int LastEltIndex = Mask.size() - 1;
+  for (int i = 0, NumElts = Mask.size(); i < NumElts; ++i) {
+    int MaskEltVal = Mask[i];
+    if (MaskEltVal == -1)
----------------
Hm, why is this so different from `isIdentityMask()`?
I'm not sure why it couldn't be a copy of `isIdentityMask()` with just changed `Is{LR}HS` logic.


================
Comment at: lib/IR/Instructions.cpp:1763-1777
+bool ShuffleVectorInst::isZeroEltSplatMask(ArrayRef<int> Mask) {
+  bool UsesLHS = true;
+  bool UsesRHS = true;
+  for (int i = 0, NumElts = Mask.size(); i < NumElts; ++i) {
+    int MaskEltVal = Mask[i];
+    if (MaskEltVal == -1)
+      continue;
----------------
Same here. The foundation looks very similar, yet the approach seems different.


https://reviews.llvm.org/D48236