[PATCH] Fix assert when decoding PSHUFB mask
Robert Lougher
rob.lougher at gmail.com
Fri Sep 19 13:31:19 PDT 2014
Hi Chandler,
Thanks for the feedback. Yes, a test checking the comment is much
more useful. I had a look around, and couldn't find any tests
specifically checking the comment (rather than checking the result of
a shuffle, etc.) so I've added a new file as you suggested. Could be
more tests, but I've tested that large indexes are shown masked, and
that bit-7-set is shown as zero. I tried a 32-byte (256-bit) AVX2
example, but it didn't show a comment so I restricted it to SSSE3.
Let me know if it's OK.
Thanks,
Rob.
On 19 September 2014 19:02, Chandler Carruth <chandlerc at gmail.com> wrote:
> 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 --------------
Index: lib/Target/X86/Utils/X86ShuffleDecode.cpp
===================================================================
--- lib/Target/X86/Utils/X86ShuffleDecode.cpp (revision 218123)
+++ lib/Target/X86/Utils/X86ShuffleDecode.cpp (working copy)
@@ -247,9 +247,8 @@
if (Element & (1 << 7))
ShuffleMask.push_back(SM_SentinelZero);
else {
- int Index = Base + Element;
- assert((Index >= 0 && Index < NumElements) &&
- "Out of bounds shuffle index for pshub instruction!");
+ // Only the least significant 4 bits of the byte are used.
+ int Index = Base + (Element & 0xf);
ShuffleMask.push_back(Index);
}
}
@@ -266,9 +265,8 @@
if (M & (1 << 7))
ShuffleMask.push_back(SM_SentinelZero);
else {
- int Index = Base + M;
- assert((Index >= 0 && (unsigned)Index < RawMask.size()) &&
- "Out of bounds shuffle index for pshub instruction!");
+ // Only the least significant 4 bits of the byte are used.
+ int Index = Base + (M & 0xf);
ShuffleMask.push_back(Index);
}
}
Index: test/CodeGen/X86/pshufb-mask-comments.ll
===================================================================
--- test/CodeGen/X86/pshufb-mask-comments.ll (revision 0)
+++ test/CodeGen/X86/pshufb-mask-comments.ll (working copy)
@@ -0,0 +1,30 @@
+; RUN: llc < %s -march=x86-64 -mattr=+ssse3 | FileCheck %s
+
+; Test that the pshufb mask comment is correct.
+
+define <16 x i8> @test1(<16 x i8> %V) {
+; CHECK-LABEL: test1:
+; CHECK: pshufb {{.*}} # xmm0 = xmm0[1,0,0,0,0,2,0,0,0,0,3,0,0,0,0,4]
+ %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V, <16 x i8> <i8 1, i8 0, i8 0, i8 0, i8 0, i8 2, i8 0, i8 0, i8 0, i8 0, i8 3, i8 0, i8 0, i8 0, i8 0, i8 4>)
+ ret <16 x i8> %1
+}
+
+; Test that indexes larger than the size of the vector are shown masked (bottom 4 bits).
+
+define <16 x i8> @test2(<16 x i8> %V) {
+; CHECK-LABEL: test2:
+; CHECK: pshufb {{.*}} # xmm0 = xmm0[15,0,0,0,0,0,0,0,0,0,1,0,0,0,0,2]
+ %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V, <16 x i8> <i8 15, i8 0, i8 0, i8 0, i8 0, i8 16, i8 0, i8 0, i8 0, i8 0, i8 17, i8 0, i8 0, i8 0, i8 0, i8 50>)
+ ret <16 x i8> %1
+}
+
+; Test that indexes with bit seven set are shown as zero.
+
+define <16 x i8> @test3(<16 x i8> %V) {
+; CHECK-LABEL: test3:
+; CHECK: pshufb {{.*}} # xmm0 = xmm0[1,0,0,15,0,2,0,0],zero,xmm0[0,3,0,0],zero,xmm0[0,4]
+ %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V, <16 x i8> <i8 1, i8 0, i8 0, i8 127, i8 0, i8 2, i8 0, i8 0, i8 128, i8 0, i8 3, i8 0, i8 0, i8 255, i8 0, i8 4>)
+ ret <16 x i8> %1
+}
+
+declare <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8>, <16 x i8>) nounwind readnone
More information about the llvm-commits
mailing list