[PATCH] Fix assert when decoding PSHUFB mask

[Robert Lougher]

Thanks for the review!  Commited r218242.

On 19 September 2014 21:36, Chandler Carruth <chandlerc at gmail.com> wrote:
> Yes, this looks excellent, thanks! Please commit (or let me know if you need
> me to commit).
>
> On Fri, Sep 19, 2014 at 1:31 PM, Robert Lougher <rob.lougher at gmail.com>
> wrote:
>>
>> 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
>> >>
>> >
>>
>> _______________________________________________
>> llvm-commits mailing list
>> llvm-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>