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

[Sanjay Patel via Phabricator via llvm-commits]

spatel marked 8 inline comments as done.
spatel added inline comments.


================
Comment at: include/llvm/IR/Instructions.h:2471
+    assert(Mask->getType()->isVectorTy() && "Shuffle needs vector constant.");
+    SmallVector<int, 16> MaskAsInts;
+    getShuffleMask(Mask, MaskAsInts);
----------------
lebedev.ri wrote:
> 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.
Yes - copy/paste of magic constant. This is typical SmallVector usage throughout LLVM. OK, if we make that a follow-up cleanup and zap all of them (at least in this file)?


================
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)
----------------
lebedev.ri wrote:
> 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.
The symmetry will be clear if we have everyone call isSingleSourceMask as a preliminary step. Then the basic logic will be identical across everything except for the isTranspose oddball.


https://reviews.llvm.org/D48236