<div dir="ltr">So, this patch didn't really fix all of the problems with this code and introduced new ones.S ee r214011 for my analysis of exactly what went wrong and why. It essentially reverts part of your patch so I wanted to mention it here so you would see. Both PR20421 and PR20355 are fixed by r214011.<div>
<br></div><div>A couple of points on this patch that I wanted to mention here for future reference:</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Jul 11, 2014 at 5:08 AM, Quentin Colombet <span dir="ltr"><<a href="mailto:qcolombet@apple.com" target="_blank">qcolombet@apple.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Author: qcolombet<br>
Date: Fri Jul 11 07:08:23 2014<br>
New Revision: 212808<br>
<br>
URL: <a href="http://llvm.org/viewvc/llvm-project?rev=212808&view=rev" target="_blank">http://llvm.org/viewvc/llvm-project?rev=212808&view=rev</a><br>
Log:<br>
[X86] Fix the inversion of low and high bits for the lowering of MUL_LOHI.<br>
Also add a few comments.<br>
<br>
<rdar://problem/17581756><br>
<br>
Modified:<br>
    llvm/trunk/lib/Target/X86/X86ISelLowering.cpp<br>
    llvm/trunk/test/CodeGen/X86/vector-idiv.ll<br></blockquote><div><br></div><div><snip></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Modified: llvm/trunk/test/CodeGen/X86/vector-idiv.ll<br>

URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vector-idiv.ll?rev=212808&r1=212807&r2=212808&view=diff" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vector-idiv.ll?rev=212808&r1=212807&r2=212808&view=diff</a><br>

==============================================================================<br>
--- llvm/trunk/test/CodeGen/X86/vector-idiv.ll (original)<br>
+++ llvm/trunk/test/CodeGen/X86/vector-idiv.ll Fri Jul 11 07:08:23 2014<br>
@@ -132,9 +132,6 @@ define <4 x i32> @test8(<4 x i32> %a) {<br>
 ; SSE41: padd<br>
<br>
 ; SSE-LABEL: test8:<br>
-; SSE: psrad $31<br>
-; SSE: pand<br>
-; SSE: paddd<br>
 ; SSE: pmuludq<br>
 ; SSE: pshufd  $49<br>
 ; SSE-NOT: pshufd      $49<br></blockquote><div><br></div><div>Especially if there isn't a public PR for whatever it is you're fixing, I would really appreciate a more thorough test case or a more thorough explanation of what was wrong with the existing test case.</div>
<div><br></div><div><br></div><div>In general, I'm really strongly opposed to testing x86 vector code with open-ended instruction tests like this. An added shuffle can show up anywhere in the sequence without breaking the test case. While I know it creates some fragility with the scheduler, I think it would be much better to write tests with -NEXT to assert the exact sequence of instructions.</div>
<div><br></div><div>Also, I'd like to see us switch to using the verbose asm comments to check shuffle operations. This makes it easy to inspect (in code review, or when updating) the actual shuffle operation rather than just having the magic integer. I've been extending this functionality so we can more rigorously use these as the basis for shuffle testsing in FileCheck.</div>
<div><br></div><div>I realize both of these practices are well entrenched in this test before you showed up, but I'd like to advocate for updating any test case you're modifying to follow these patterns. It'll make code review a lot easier for those patches anyways...</div>
</div></div></div>