[llvm] r212808 - [X86] Fix the inversion of low and high bits for the lowering of MUL_LOHI.

[Chandler Carruth]

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.

A couple of points on this patch that I wanted to mention here for future
reference:

On Fri, Jul 11, 2014 at 5:08 AM, Quentin Colombet <qcolombet at apple.com>
wrote:

> Author: qcolombet
> Date: Fri Jul 11 07:08:23 2014
> New Revision: 212808
>
> URL: http://llvm.org/viewvc/llvm-project?rev=212808&view=rev
> Log:
> [X86] Fix the inversion of low and high bits for the lowering of MUL_LOHI.
> Also add a few comments.
>
> <rdar://problem/17581756>
>
> Modified:
>     llvm/trunk/lib/Target/X86/X86ISelLowering.cpp
>     llvm/trunk/test/CodeGen/X86/vector-idiv.ll
>

<snip>


> Modified: llvm/trunk/test/CodeGen/X86/vector-idiv.ll
> URL:
> http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/X86/vector-idiv.ll?rev=212808&r1=212807&r2=212808&view=diff
>
> ==============================================================================
> --- llvm/trunk/test/CodeGen/X86/vector-idiv.ll (original)
> +++ llvm/trunk/test/CodeGen/X86/vector-idiv.ll Fri Jul 11 07:08:23 2014
> @@ -132,9 +132,6 @@ define <4 x i32> @test8(<4 x i32> %a) {
>  ; SSE41: padd
>
>  ; SSE-LABEL: test8:
> -; SSE: psrad $31
> -; SSE: pand
> -; SSE: paddd
>  ; SSE: pmuludq
>  ; SSE: pshufd  $49
>  ; SSE-NOT: pshufd      $49
>

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.


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.

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.

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