[PATCH] Fix assert when decoding PSHUFB mask

[Chandler Carruth]

Ah, of course. Thanks for the patch. The change to the code LGTM, but the
test could really be better:

Index: test/CodeGen/X86/pshufb-intrinsic-crash.ll
===================================================================
--- test/CodeGen/X86/pshufb-intrinsic-crash.ll (revision 0)
+++ test/CodeGen/X86/pshufb-intrinsic-crash.ll (working copy)
@@ -0,0 +1,11 @@
+; RUN: llc < %s -march=x86-64 -mcpu=corei7
+; REQUIRES: asserts
+
+; Test that the compiler doesn't assert if we use a pshufb intrinsic with
a mask index that
+; is larger than the size of the vector.

Don't test for the absence of historical mistakes, test for the successful
desired behavior. =]

This test shouldn't require asserts and should check that the comment
prints the mask actually used (IE, masking off the high bits). Then you can
name the test 'pshufb-mask-comments.ll' or something generic (potentially
merging other tests for the comment printing into it, or merging it into
existing tests for the comment printing).

+
+define <16 x i8> @test(<16 x i8> %V) {
+  %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V, <16 x
i8> <i8 0, i8 50, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8
0, i8 0, i8 0, i8 0, i8 0>)
+  ret <16 x i8> %1
+}
+declare <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8>, <16 x i8>)
nounwind readnone



On Fri, Sep 19, 2014 at 10:46 AM, Robert Lougher <rob.lougher at gmail.com>
wrote:

> Hi,
>
> Revision 213986 added code to decode PSHUFB masks in order to print
> shuffle comments for PSHUFB instructions that have a constant mask.
> This decode logic was then used in the new shuffle lowering code
> (r214628).
>
> The mask decode routine asserts if the mask index is out of range (<0
> or greater than the vector size).  The problem is, we can legitimately
> have a PSHUFB with a large index using the intrinsic:
>
> define <16 x i8> @test(<16 x i8> %V) {
>   %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V,
> <16 x i8> <i8 0, i8 50, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8
> 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0>)
>   ret <16 x i8> %1
> }
>
> Here we have an index of 50, which will assert.  The instruction,
> however, takes the least significant 4 bits, so this is index 2.
>
> The attached patch matches this behaviour, and removes the assert (the
> index can never be negative).  The test simply checks that we don't
> assert.
>
> Please review and let me know if it is OK to commit.
>
> Thanks,
> Rob.
>
> --
>
> Robert Lougher
> SN Systems - Sony Computer Entertainment Group
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140919/94adcb55/attachment.html>