[PATCH] D48236: [IR] move shuffle mask queries from TTI to ShuffleVectorInst
Roman Lebedev via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Sat Jun 16 13:43:50 PDT 2018
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
More information about the llvm-commits
mailing list