<div dir="ltr">Ah, of course. Thanks for the patch. The change to the code LGTM, but the test could really be better:<div><br></div><div><div>Index: test/CodeGen/X86/pshufb-intrinsic-crash.ll</div><div>===================================================================</div><div>--- test/CodeGen/X86/pshufb-intrinsic-crash.ll<span class="" style="white-space:pre">   </span>(revision 0)</div><div>+++ test/CodeGen/X86/pshufb-intrinsic-crash.ll<span class="" style="white-space:pre"> </span>(working copy)</div><div>@@ -0,0 +1,11 @@</div><div>+; RUN: llc < %s -march=x86-64 -mcpu=corei7</div><div>+; REQUIRES: asserts</div><div>+</div><div>+; Test that the compiler doesn't assert if we use a pshufb intrinsic with a mask index that</div><div>+; is larger than the size of the vector.</div><div><br></div><div>Don't test for the absence of historical mistakes, test for the successful desired behavior. =]</div><div><br></div><div>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).</div><div><br></div><div>+</div><div>+define <16 x i8> @test(<16 x i8> %V) {</div><div>+  %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>)</div><div>+  ret <16 x i8> %1</div><div>+}</div><div>+declare <16 x i8> @llvm.x86.ssse3.pshuf.b.128(<16 x i8>, <16 x i8>) nounwind readnone</div></div><div><br></div><div><br></div></div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Sep 19, 2014 at 10:46 AM, 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,<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></blockquote></div><br></div>