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

[Quentin Colombet]

Hi Chandler,

Sorry for the delay, I was in vacations.

On Jul 25, 2014, at 9:02 PM, Chandler Carruth <chandlerc at google.com> wrote:

> 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.

Thanks for the actual fix.
> 
> 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.

Honestly, I found the high-low inversion just by code inspection (after working on a fix on the original masks that were wrong).
When I made the change, I asked Benjamin if the new code was ok on the test case that was changing.

I should have been more careful, apologies for that.
> 
> 
> 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.

Agree.

> 
> 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...

Noted!

Thanks,
-Quentin
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140728/04794014/attachment.html>