[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: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:
> 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.
In other words, either the comment should not talk about even/odd -numbered vector elements,
or the check should actually consider even/odd -ness.


https://reviews.llvm.org/D48236