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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/IR/Instructions.cpp:1693-1694
+    int MaskEltVal = getMaskValue(Mask, i);
+    if (MaskEltVal < 0)
+      continue;
+    bool IsLHS = (unsigned)MaskEltVal == i;
----------------
lebedev.ri wrote:
> Negative values represent `undef`?
Good question/catch! Officially, only -1 represents undef:
  /// Return the shuffle mask value for the specified element of the mask.
  /// Return -1 if the element is undef.
  static int getMaskValue(Constant *Mask, unsigned Elt);

 ...and I wish we weren't using a magic number rather than a named constant, but that's a separate patch.

This code was copied/translated from the existing code, but I'll fix it.


================
Comment at: lib/IR/Instructions.cpp:1704-1705
+      return false;
+  }
+  return true;
+}
----------------
lebedev.ri wrote:
> Just as a sanity check, i would have expected to see an assert here:
> ```
> assert((FoundLHS ^ FoundRHS) && "Should have selected from exactly one source");
> ```
Agreed, and we should assert other assumptions (eg, the input Constant is a vector).


================
Comment at: lib/IR/Instructions.cpp:1735-1738
+    ReverseLHS &= ((unsigned)MaskEltVal == (NumElts - 1 - i));
+    ReverseRHS &= ((unsigned)MaskEltVal == (NumElts + NumElts - 1 - i));
+    if (!ReverseLHS && !ReverseRHS)
+      return false;
----------------
lebedev.ri wrote:
> This method was declared with:
> ```
>   /// Return true if this shuffle mask swaps the order of elements from exactly
>   /// one source vector.
>   /// Example: <7,6,undef,4>
>   /// This assumes that vector operands are the same length as the mask.
> ```
> You are sure this does what that comment says it does?
This was copied from the existing code, so unless I introduced a typo, that's the correct logic.

Note that this patch is intended to be no-functional-change for any existing users of the TTI code, so there are existing regression tests in place (though if typical, they are far from thorough).

I'll definitely add more unit tests to exercise this further.


================
Comment at: lib/IR/Instructions.cpp:1783-1787
+  // 2. The first element of the mask must be either a zero (for the
+  // even-numbered vector elements) or a one (for the odd-numbered vector
+  // elements).
+  if (getMaskValue(Mask, 0) != 0 && getMaskValue(Mask, 0) != 1)
+    return false;
----------------
lebedev.ri wrote:
> ```
> unsigned NumSourceElts = Op<0>()->getType()->getVectorNumElements();
> ```
> 
> But this does not seem to take the `NumSourceElts & 1` into account, like the comment says?
I don't understand this comment. Can you provide more detail/example of the possible logic bug?


https://reviews.llvm.org/D48236