[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: lib/IR/Instructions.cpp:1704-1705
+      return false;
+  }
+  return true;
+}
----------------
spatel wrote:
> 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).
Yes, assertions[/contracts] are nice.


================
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;
----------------
spatel wrote:
> 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.
I understand that it is //meant// to be a NFC, but since all the relevant changes
are nicely layed out in a single diff, it is really easy to look them through,
and note **possible** discrepancies.

Otherwise, if this is fully NFC, you could just commit as is :)


================
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;
----------------
spatel wrote:
> 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?
I can try.
The comment states:
```
  // 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).
```
But the check is:
```
  if (getMaskValue(Mask, 0) != 0 && getMaskValue(Mask, 0) != 1)
    return false;
```
I.e. if the first element is not `0` and not `1`, we bailout.
But the **comment** suggests that the logic should be:
```
unsigned NumSourceElts = Op<0>()->getType()->getVectorNumElements();
if (!(((NumSourceElts % 2 == 0) && (getMaskValue(Mask, 0) == 0)) ||
      ((NumSourceElts % 2 == 1) && (getMaskValue(Mask, 0) == 1))))
    return false;
```
Is this any clearer?

Or, the comment is wrong.


https://reviews.llvm.org/D48236