[PATCH][X86] SSE2 cvtsd2ss instruction expects two operands

[Craig Topper]

For this case you can probably copy the avx-intrinsics.ll test case for
this intrinsic and create an sse2-intrinsics.ll. There appear to be several
more intrinsics that aren't being tested on sse2 so if you want to add
tests for them that would be awesome.

The run line should explicitly have -mattr=+sse2,-avx  This will keep it
from generating AVX instructions if it lands on an AVX enabled buildbot.


On Tue, Oct 22, 2013 at 5:50 PM, Quentin Colombet <qcolombet at apple.com>wrote:

> Hi Cameron,
>
> What you can probably do is having a very small test case like this:
>
> $ cat test.ll
> ; RUN llc -mtriple x86_64 -matrr=+sse2 < %s -o - | FileCheck %s
>
> define @fct(<some args>) {
> ; CHECK-NOT: <expected instruction> <regex op1>, <regex op2>, <regex op3>
> <simple use of your instrinsic>
> }
>
> Basically, you just need to write a test case that use the intrinsic you
> want (no need to be a real workload) and check that it does not have three
> operands.
> It would be best if you can actually check that your intrinsic is
> generated but I am not sure how it would play with CHECK-NOT. You can
> easily figure that out with a quick test :).
>
> Regarding the RUN command, you will probably need to adapt the triple or
> mattr arguments, I did not check if it is the right syntax.
> As soon as you have something you think is okay to go, name the test file
> appropriately and move it into test/CodeGen/X86. Then 'svn add' this file
> and produce the new diff to send to the mailing list.
>
> By doing this, you would have a patch with a test case ;).
>
> Cheers,
> -Quentin
>
> On Oct 22, 2013, at 2:42 PM, Cameron McInally <cameron.mcinally at nyu.edu>
> wrote:
>
> On Tue, Oct 22, 2013 at 2:40 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> Hi Cameron,
>
> On Oct 22, 2013, at 10:02 AM, Cameron McInally <cameron.mcinally at nyu.edu>
> wrote:
>
> Hey Quentin,
>
> On Mon, Oct 21, 2013 at 8:05 PM, Quentin Colombet <qcolombet at apple.com>
> wrote:
>
> Hi Cameron,
>
> I do not think this is quite right.
>
> CVTSD2SS has three operands for the SSE variant.
> Indeed, the converted operand is set to the low quadword (or doubleword) of
> the destination register ($dst), and the high quadword (doublewords) are
> left unchanged ($src1).
>
> CVTSD2SS does what you want for the FP variant (one source one
> destination):
>
> def CVTSD2SSrr  : SDI<0x5A, MRMSrcReg, (outs FR32:$dst), (ins FR64:$src),
>                     "cvtsd2ss\t{$src, $dst|$dst, $src}",
>                     [(set FR32:$dst, (fround FR64:$src))],
>                     IIC_SSE_CVT_Scalar_RR>, Sched<[WriteCvtF2F]>;
>
>
> Am I missing something?
>
>
> This may be a difference in terminology.
>
> Indeed!
>
> I will try to be more explicit.
>
> The SSE2 cvtsd2ss instruction expects two operands according to the
> "Intel Architecture Instructions Set Extensions Programming
> Reference", i.e. 319433-016 Oct 2013. [Note: It may also be of
> interest that the VEX/EVEX encoded variant of this instruction expects
> three+ operands.]
>
> I also agree that LLVM's CVTSD2SSrr pattern is correct. However, the
> Int_CVTSD2SSrr pattern is not correct.
>
> let Constraints = "$src1 = $dst" in {
> def Int_CVTSD2SSrr: I<0x5A, MRMSrcReg,
>                      (outs VR128:$dst), (ins VR128:$src1, VR128:$src2),
>                      "cvtsd2ss\t{$src2, $src1, $dst|$dst, $src1, $src2}",
>                      [(set VR128:$dst,
>                        (int_x86_sse2_cvtsd2ss VR128:$src1, VR128:$src2))],
>                      IIC_SSE_CVT_Scalar_RR>, XD, Requires<[UseSSE2]>,
>                      Sched<[WriteCvtF2F]>;
> }
>
> This pattern will produce an assembly instruction with 3 operands.
> [Note: the third operand is necessary for the VEX-encoded instruction,
> but is superfluous for the SSE2 instruction.]
>
> Incorrect SSE2 usage: cvtsd2ss %xmm0, %xmm2, %xmm2
> Correct SSE2 usage: cvtsd2ss %xmm0, %xmm2
>
> I see, my mistake.
>
>
> My compiler currently uses a version of GNU ld, which issues an error
> with the three operand instruction:
>
> [scrubbed]/sse2-cvtsd2ss-1_1.s: Assembler messages:
> [scrubbed]/sse2-cvtsd2ss-1_1.s:209: Error: number of operands mismatch
> for `cvtsd2ss'
>
> I may have wrongly assumed that this patch was trivial, so did not
> prepare a reduced test case. And, unfortunately, my test case and IR
> is incompatible with ToT Clang/LLVM. If there is any further confusion
> surrounding this patch, I will be happy to create a small compatible
> test case for this problem.
>
> Generally speaking, if you can come up with a test case it would be best
> even for trivial fixes.
> With a test case, this would look good for me.
>
>
> I've attached the .ll output for a rather thorough test case that
> makes use of the llvm.x86.sse2.cvtsd2ss intrinsic with -msse2.
>
> Clang's assembler does indeed complain about the extra operand. There
> is, of course, no such complaint when directly producing the object
> file. ;)
>
> [scrubbed]-MacBook-Air:bin [scrubbed]$ ./clang -msse2 test.c -S
> [scrubbed]-MacBook-Air:bin [scrubbed]$ grep cvtsd2ss test.s
> cvtsd2ss %xmm1, %xmm0, %xmm0
> cvtsd2ss -144(%rbp), %xmm0
> [scrubbed]-MacBook-Air:bin [scrubbed]$ ./clang -msse2 test.s
> test.s:37:25: error: invalid operand for instruction
> cvtsd2ss %xmm1, %xmm0, %xmm0
>                        ^~~~~
> [scrubbed]-MacBook-Air:bin [scrubbed]$ ./clang -msse2 test.c
> [scrubbed]-MacBook-Air:bin [scrubbed]$
>
> Unfortunately, I do not have enough experience with Clang's toolchain
> and test harness to suggest a plan of attack for catching bugs of this
> class. Hopefully someone with a better understanding of the process
> can chime in.
>
> -Cameron
> <test.ll>
>
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>
>


-- 
~Craig
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20131022/05e96829/attachment.html>