<div dir="ltr">Yes, this looks excellent, thanks! Please commit (or let me know if you need me to commit).</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 19, 2014 at 1:31 PM, Robert Lougher <span dir="ltr"><<a href="mailto:rob.lougher@gmail.com" target="_blank">rob.lougher@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Chandler,<br>
<br>
Thanks for the feedback.  Yes, a test checking the comment is much<br>
more useful.  I had a look around, and couldn't find any tests<br>
specifically checking the comment (rather than checking the result of<br>
a shuffle, etc.) so I've added a new file as you suggested.  Could be<br>
more tests, but I've tested that large indexes are shown masked, and<br>
that bit-7-set is shown as zero.  I tried a 32-byte (256-bit) AVX2<br>
example, but it didn't show a comment so I restricted it to SSSE3.<br>
Let me know if it's OK.<br>
<br>
Thanks,<br>
Rob.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
On 19 September 2014 19:02, Chandler Carruth <<a href="mailto:chandlerc@gmail.com">chandlerc@gmail.com</a>> wrote:<br>
> Ah, of course. Thanks for the patch. The change to the code LGTM, but the<br>
> test could really be better:<br>
><br>
> Index: test/CodeGen/X86/pshufb-intrinsic-crash.ll<br>
> ===================================================================<br>
> --- test/CodeGen/X86/pshufb-intrinsic-crash.ll (revision 0)<br>
> +++ test/CodeGen/X86/pshufb-intrinsic-crash.ll (working copy)<br>
> @@ -0,0 +1,11 @@<br>
> +; RUN: llc < %s -march=x86-64 -mcpu=corei7<br>
> +; REQUIRES: asserts<br>
> +<br>
> +; Test that the compiler doesn't assert if we use a pshufb intrinsic with a<br>
> mask index that<br>
> +; is larger than the size of the vector.<br>
><br>
> Don't test for the absence of historical mistakes, test for the successful<br>
> desired behavior. =]<br>
><br>
> This test shouldn't require asserts and should check that the comment prints<br>
> the mask actually used (IE, masking off the high bits). Then you can name<br>
> the test 'pshufb-mask-comments.ll' or something generic (potentially merging<br>
> other tests for the comment printing into it, or merging it into existing<br>
> tests for the comment printing).<br>
><br>
> +<br>
> +define <16 x i8> @test(<16 x i8> %V) {<br>
> +  %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V, <16 x<br>
> i8> <i8 0, i8 50, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8<br>
> 0, i8 0, i8 0, i8 0, i8 0>)<br>
> +  ret <16 x i8> %1<br>
> +}<br>
> +declare <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8>, <16 x i8>)<br>
> nounwind readnone<br>
><br>
><br>
><br>
> On Fri, Sep 19, 2014 at 10:46 AM, Robert Lougher <<a href="mailto:rob.lougher@gmail.com">rob.lougher@gmail.com</a>><br>
> wrote:<br>
>><br>
>> Hi,<br>
>><br>
>> Revision 213986 added code to decode PSHUFB masks in order to print<br>
>> shuffle comments for PSHUFB instructions that have a constant mask.<br>
>> This decode logic was then used in the new shuffle lowering code<br>
>> (r214628).<br>
>><br>
>> The mask decode routine asserts if the mask index is out of range (<0<br>
>> or greater than the vector size).  The problem is, we can legitimately<br>
>> have a PSHUFB with a large index using the intrinsic:<br>
>><br>
>> define <16 x i8> @test(<16 x i8> %V) {<br>
>>   %1 = tail call <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8> %V,<br>
>> <16 x i8> <i8 0, i8 50, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8<br>
>> 0, i8 0, i8 0, i8 0, i8 0, i8 0, i8 0>)<br>
>>   ret <16 x i8> %1<br>
>> }<br>
>><br>
>> Here we have an index of 50, which will assert.  The instruction,<br>
>> however, takes the least significant 4 bits, so this is index 2.<br>
>><br>
>> The attached patch matches this behaviour, and removes the assert (the<br>
>> index can never be negative).  The test simply checks that we don't<br>
>> assert.<br>
>><br>
>> Please review and let me know if it is OK to commit.<br>
>><br>
>> Thanks,<br>
>> Rob.<br>
>><br>
>> --<br>
>><br>
>> Robert Lougher<br>
>> SN Systems - Sony Computer Entertainment Group<br>
>><br>
>> _______________________________________________<br>
>> llvm-commits mailing list<br>
>> <a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
>><br>
><br>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br></div>