[PATCH] Fix assert when decoding PSHUFB mask

[Chandler Carruth]

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
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140919/28f7866f/attachment.html>