[x86] Enable f128 as a legal type in 64-bit mode if SSE is enabled rather than if MMX is enabled.

[craig.topper]

Summary

Currently if you use f128 in 64-bit mode with sse disabled and mmx enabled, you get an error about not being able to use an SSE register for a return. And then later we throw an assert trying to emit an xmm register copy.

Changing this to check SSE instead of MMX at least stops us from printing the error and asserting.

I'm still not sure this is correct though. Looking at some assembly with sse/mmx enabled we think the f128 library functions like add return their result in xmm0. But if you disable sse/mmx we then think they return their result in rax/rdx. So unless the library functions return their result both ways, I don't think this is correct.

Here's an example that currently crashes when compiled with "clang -c -mno-mmx"

void foo(__float128 a, __float128 b, __float128 *c) {
    *c = a + b;
}

[chh]

I cannot get clang to crash with "clang -c -mno-mmx" and the given example.
Maybe I missed something?

It also looks opposite to the problem of "... with sse disabled and mmx
enabled, you get an error about not being able to use an SSE register
for a return ..."
Could you include an example to reproduce that error?

Although replacing hasMMX with hasSSE1 looked logical and it passed
most Android used modes, the failed unit test cases showed
incompatible ABI generated for some other modes like SSE and AVX.
We need to find out if those before change output were correct or not.
Even if they were incorrect, I am not sure about breaking the compatibility.

Some of the unit test checks in extract-store.ll were removed.
They should be kept and changed to check new output pattern if necessary.

[craig.topper]

Oops I had that backwards, it crashes with clang -c -mmmx -mno-sse

I didn't remove any of the checks in extract_store. They just collapsed into a common check-prefix.

[chh]

Okay, I see the change in extract_store.ll now.
The checks were not reduced.
The test mode +mmx was changed to +sse,
and expected output of +sse2, +sse4.1, +avx were changed.

With -mmmx -mno-sse, I can see a clang assertion fault following an error message:

  .... error: SSE register return with SSE disabled
void foo(__float128 a, __float128 b, __float128 *c) {
     ^
clang-5.0: .../llvm/lib/CodeGen/TargetRegisterInfo.cpp:173: const llvm::TargetRegisterClass* llvm::TargetRegisterInfo::getMinimalPhysRegClass(unsigned int, llvm::MVT) const: Assertion `BestRC && "Couldn't find the register class"' failed.

Which target/plaftform needs "-mmmx -mno-sse"?
Which uses only mmx or sse?

I saw some other code that checked hasMMX() only,
so I added test cases with +sse2 and without +mmx.
I think they should not use the mmx registers.

How about making only the following change?

if (Subtarget.is64Bit() && Subtarget.hasMMX())

to

if (Subtarget.is64Bit() && Subtarget.hasMMX() && Subtarget.hasSSE1())

That will pass existing unit tests and your -mmx -mno-sse test case.

[craig.topper]

-mno-sse and -mmmx are driver options a user can specify on the clang command line. They aren't a platform requirement.

Did you look at the assembly output for the test case i gave with and without "-mno-sse -mmmx"? I'm pretty sure even with my change or your change we still miscompile the code when SSE and MMX are disabled. As you can see here https://godbolt.org/g/ELe3CW with them enabled we think addtf3 returns in xmm0, but if we disable mmx and sse we expect the return in rax/rdx. addtf3 is a library function that we didn't compile. Does it return both ways or is the rax/rdx version a miscompile? I believe gcc just throws an error for this case.

[chh]

My original change in lib/Target/X86/X86ISelLowering.cpp
was part of https://reviews.llvm.org/D15134 to fix
calling convention bug of f128 type mentioned in
https://bugs.llvm.org/show_bug.cgi?id=23897.

Android's x64 target assumes both -msse and -mmmx.
So it works with

if (Subtarget.is64Bit() && Subtarget.hasMMX())

or

if (Subtarget.is64Bit() && Subtarget.hasSSE1())

or

if (Subtarget.is64Bit() && Subtarget.hasMMX() && Subtarget.hasSSE1())

I see now with -msse and -mno-mmx, gcc still uses %xmm0.
That seems to be compatible with AMD64 ABI, too.
So your change in X86ISelLowering.cpp:611 is good:

if (Subtarget.is64Bit() && Subtarget.hasSSE1())

Could you change the 3 affected test files to include both
(1) original +mmx mode, (no sse) but expected output should be changed.
(2) add +sse mode, and the expected output is to using %xmm0 for __float128

The remaining big question is what to do with __float128 when there is no SSE.
I think gcc's output is still better matching AMD64 ABI.
Without SSE,

gcc passes __float128 on stack, but reject source code that returns __float128.

$ gcc -S -O2 m.c -mno-sse -mno-mmx -o m.g.s

1. or gcc -S -O2 m.c -mno-sse -o m.g.mmx.s

m.c: In function 'get1':
m.c:5:1: error: SSE register return with SSE disabled
float128 get1() { return x; }
m.c:13:58: error: SSE register return with SSE disabled
void foo(float128 a, float128 b, float128 *c) { *c = a + b; }

It should take more changes (and maybe library functions) to make clang's output
match gcc's under -mno-sse. We should fix that problem later in a separate CL.

[RKSimon]

@craig.topper I noticed in D67128 that it looks like this was dealt with at rL370682?

[craig.topper]

As @RKSimon said this has been fixed in rL370682

lib/Target/X86/X86ISelLowering.cpp

if (!Subtarget.useSoftFloat() && X86ScalarSSEf64) {
     addLegalFPImmediate(APFloat(-0.0f)); // FLD0/FCHS
     addLegalFPImmediate(APFloat(-1.0f)); // FLD1/FCHS
   }
 
   // We don't support FMA.
   setOperationAction(ISD::FMA, MVT::f64, Expand);
   setOperationAction(ISD::FMA, MVT::f32, Expand);
 
-  // Long double always uses X87, except f128 in MMX.
+  // Long double always uses X87, except f128 in SSE.
   if (UseX87) {
-    if (Subtarget.is64Bit() && Subtarget.hasMMX()) {
+    if (Subtarget.is64Bit() && Subtarget.hasSSE1()) {
       addRegisterClass(MVT::f128, &X86::FR128RegClass);
       ValueTypeActions.setTypeAction(MVT::f128, TypeSoftenFloat);
       setOperationAction(ISD::FABS , MVT::f128, Custom);
       setOperationAction(ISD::FNEG , MVT::f128, Custom);
       setOperationAction(ISD::FCOPYSIGN, MVT::f128, Custom);
     }
 
     addRegisterClass(MVT::f80, &X86::RFP80RegClass);

test/CodeGen/X86/extract-store.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=i686-unknown -mattr=+sse2     | FileCheck %s --check-prefix=X32 --check-prefix=SSE-X32 --check-prefix=SSE2-X32
 ; RUN: llc < %s -mtriple=x86_64-unknown -mattr=+sse2   | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE2-X64
 ; RUN: llc < %s -mtriple=i686-unknown -mattr=+sse4.1   | FileCheck %s --check-prefix=X32 --check-prefix=SSE-X32 --check-prefix=SSE41-X32
 ; RUN: llc < %s -mtriple=x86_64-unknown -mattr=+sse4.1 | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE41-X64
 ; RUN: llc < %s -mtriple=i686-unknown -mattr=+avx      | FileCheck %s --check-prefix=X32 --check-prefix=AVX-X32
 ; RUN: llc < %s -mtriple=x86_64-unknown -mattr=+avx    | FileCheck %s --check-prefix=X64 --check-prefix=AVX-X64
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+mmx -enable-legalize-types-checking | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE-F128
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+mmx -enable-legalize-types-checking | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE-F128
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+sse -enable-legalize-types-checking | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE-F128
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+sse -enable-legalize-types-checking | FileCheck %s --check-prefix=X64 --check-prefix=SSE-X64 --check-prefix=SSE-F128
 
 define void @extract_i8_0(i8* nocapture %dst, <16 x i8> %foo) nounwind {
 ; SSE2-X32-LABEL: extract_i8_0:
 ; SSE2-X32:       # BB#0:
 ; SSE2-X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; SSE2-X32-NEXT:    movd %xmm0, %ecx
 ; SSE2-X32-NEXT:    movb %cl, (%eax)
 ; SSE2-X32-NEXT:    retl

 ; SSE-X32-NEXT:    movl %esi, 12(%edi)
 ; SSE-X32-NEXT:    movl %edx, 8(%edi)
 ; SSE-X32-NEXT:    movl %ecx, 4(%edi)
 ; SSE-X32-NEXT:    movl %eax, (%edi)
 ; SSE-X32-NEXT:    popl %esi
 ; SSE-X32-NEXT:    popl %edi
 ; SSE-X32-NEXT:    retl
 ;
-; SSE2-X64-LABEL: extract_f128_0:
-; SSE2-X64:       # BB#0:
-; SSE2-X64-NEXT:    movq %rdx, 8(%rdi)
-; SSE2-X64-NEXT:    movq %rsi, (%rdi)
-; SSE2-X64-NEXT:    retq
-;
-; SSE41-X64-LABEL: extract_f128_0:
-; SSE41-X64:       # BB#0:
-; SSE41-X64-NEXT:    movq %rdx, 8(%rdi)
-; SSE41-X64-NEXT:    movq %rsi, (%rdi)
-; SSE41-X64-NEXT:    retq
+; X64-LABEL: extract_f128_0:
+; X64:       # BB#0:
+; X64-NEXT:    movaps %xmm0, (%rdi)
+; X64-NEXT:    retq
 ;
 ; AVX-X32-LABEL: extract_f128_0:
 ; AVX-X32:       # BB#0:
 ; AVX-X32-NEXT:    vmovups {{[0-9]+}}(%esp), %xmm0
 ; AVX-X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; AVX-X32-NEXT:    vmovups %xmm0, (%eax)
 ; AVX-X32-NEXT:    retl
-;
-; AVX-X64-LABEL: extract_f128_0:
-; AVX-X64:       # BB#0:
-; AVX-X64-NEXT:    movq %rdx, 8(%rdi)
-; AVX-X64-NEXT:    movq %rsi, (%rdi)
-; AVX-X64-NEXT:    retq
-;
-; SSE-F128-LABEL: extract_f128_0:
-; SSE-F128:       # BB#0:
-; SSE-F128-NEXT:    movaps %xmm0, (%rdi)
-; SSE-F128-NEXT:    retq
   %vecext = extractelement <2 x fp128> %foo, i32 0
   store fp128 %vecext, fp128* %dst, align 1
   ret void
 }
 
 define void @extract_f128_1(fp128* nocapture %dst, <2 x fp128> %foo) nounwind {
 ; SSE-X32-LABEL: extract_f128_1:
 ; SSE-X32:       # BB#0:
 ; SSE-X32-NEXT:    pushl %edi
 ; SSE-X32-NEXT:    pushl %esi
 ; SSE-X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; SSE-X32-NEXT:    movl {{[0-9]+}}(%esp), %ecx
 ; SSE-X32-NEXT:    movl {{[0-9]+}}(%esp), %edx
 ; SSE-X32-NEXT:    movl {{[0-9]+}}(%esp), %esi
 ; SSE-X32-NEXT:    movl {{[0-9]+}}(%esp), %edi
 ; SSE-X32-NEXT:    movl %esi, 12(%edi)
 ; SSE-X32-NEXT:    movl %edx, 8(%edi)
 ; SSE-X32-NEXT:    movl %ecx, 4(%edi)
 ; SSE-X32-NEXT:    movl %eax, (%edi)
 ; SSE-X32-NEXT:    popl %esi
 ; SSE-X32-NEXT:    popl %edi
 ; SSE-X32-NEXT:    retl
 ;
-; SSE2-X64-LABEL: extract_f128_1:
-; SSE2-X64:       # BB#0:
-; SSE2-X64-NEXT:    movq %r8, 8(%rdi)
-; SSE2-X64-NEXT:    movq %rcx, (%rdi)
-; SSE2-X64-NEXT:    retq
-;
-; SSE41-X64-LABEL: extract_f128_1:
-; SSE41-X64:       # BB#0:
-; SSE41-X64-NEXT:    movq %r8, 8(%rdi)
-; SSE41-X64-NEXT:    movq %rcx, (%rdi)
-; SSE41-X64-NEXT:    retq
+; X64-LABEL: extract_f128_1:
+; X64:       # BB#0:
+; X64-NEXT:    movaps %xmm1, (%rdi)
+; X64-NEXT:    retq
 ;
 ; AVX-X32-LABEL: extract_f128_1:
 ; AVX-X32:       # BB#0:
 ; AVX-X32-NEXT:    vmovups {{[0-9]+}}(%esp), %xmm0
 ; AVX-X32-NEXT:    movl {{[0-9]+}}(%esp), %eax
 ; AVX-X32-NEXT:    vmovups %xmm0, (%eax)
 ; AVX-X32-NEXT:    retl
-;
-; AVX-X64-LABEL: extract_f128_1:
-; AVX-X64:       # BB#0:
-; AVX-X64-NEXT:    movq %r8, 8(%rdi)
-; AVX-X64-NEXT:    movq %rcx, (%rdi)
-; AVX-X64-NEXT:    retq
-;
-; SSE-F128-LABEL: extract_f128_1:
-; SSE-F128:       # BB#0:
-; SSE-F128-NEXT:    movaps %xmm1, (%rdi)
-; SSE-F128-NEXT:    retq
   %vecext = extractelement <2 x fp128> %foo, i32 1
   store fp128 %vecext, fp128* %dst, align 1
   ret void
 }
 
 define void @extract_i8_undef(i8* nocapture %dst, <16 x i8> %foo) nounwind {
 ; X32-LABEL: extract_i8_undef:
 ; X32:       # BB#0:

test/CodeGen/X86/fp128-select.ll

 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+mmx \
-; RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+mmx \
-; RUN:     -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-android \
-; RUN:     -enable-legalize-types-checking | FileCheck %s
-; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu \
-; RUN:     -enable-legalize-types-checking | FileCheck %s
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=+sse -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=+sse -enable-legalize-types-checking | FileCheck %s --check-prefix=MMX
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-android -mattr=-sse -enable-legalize-types-checking | FileCheck %s
+; RUN: llc < %s -O2 -mtriple=x86_64-linux-gnu -mattr=-sse -enable-legalize-types-checking | FileCheck %s
 
 define void @test_select(fp128* %p, fp128* %q, i1 zeroext %c) {
 ; MMX-LABEL: test_select:
 ; MMX:       # BB#0:
 ; MMX-NEXT:    testb %dl, %dl
 ; MMX-NEXT:    jne .LBB0_1
 ; MMX-NEXT:  # BB#2:
 ; MMX-NEXT:    movaps {{.*}}(%rip), %xmm0

test/CodeGen/X86/vec_fp_to_int.ll

; AVX-NEXT:    retq
   %cvt = fptosi <2 x x86_fp80> %a to <2 x i32>
   %ext = shufflevector <2 x i32> %cvt, <2 x i32> zeroinitializer, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   ret <4 x i32> %ext
 }
 
 define <4 x i32> @fptosi_2f128_to_4i32(<2 x fp128> %a) nounwind {
 ; SSE-LABEL: fptosi_2f128_to_4i32:
 ; SSE:       # BB#0:
-; SSE-NEXT:    pushq %r14
-; SSE-NEXT:    pushq %rbx
-; SSE-NEXT:    subq $24, %rsp
-; SSE-NEXT:    movq %rsi, %r14
-; SSE-NEXT:    movq %rdi, %rbx
-; SSE-NEXT:    movq %rdx, %rdi
-; SSE-NEXT:    movq %rcx, %rsi
+; SSE-NEXT:    subq $40, %rsp
+; SSE-NEXT:    movaps %xmm0, {{[0-9]+}}(%rsp) # 16-byte Spill
+; SSE-NEXT:    movaps %xmm1, %xmm0
 ; SSE-NEXT:    callq __fixtfdi
 ; SSE-NEXT:    movq %rax, %xmm0
 ; SSE-NEXT:    movdqa %xmm0, (%rsp) # 16-byte Spill
-; SSE-NEXT:    movq %rbx, %rdi
-; SSE-NEXT:    movq %r14, %rsi
+; SSE-NEXT:    movaps {{[0-9]+}}(%rsp), %xmm0 # 16-byte Reload
 ; SSE-NEXT:    callq __fixtfdi
 ; SSE-NEXT:    movq %rax, %xmm0
 ; SSE-NEXT:    punpcklqdq (%rsp), %xmm0 # 16-byte Folded Reload
 ; SSE-NEXT:    # xmm0 = xmm0[0],mem[0]
 ; SSE-NEXT:    xorps %xmm1, %xmm1
 ; SSE-NEXT:    shufps {{.*#+}} xmm0 = xmm0[0,2],xmm1[2,3]
-; SSE-NEXT:    addq $24, %rsp
-; SSE-NEXT:    popq %rbx
-; SSE-NEXT:    popq %r14
+; SSE-NEXT:    addq $40, %rsp
 ; SSE-NEXT:    retq
 ;
 ; AVX-LABEL: fptosi_2f128_to_4i32:
 ; AVX:       # BB#0:
-; AVX-NEXT:    pushq %r14
-; AVX-NEXT:    pushq %rbx
-; AVX-NEXT:    subq $24, %rsp
-; AVX-NEXT:    movq %rsi, %r14
-; AVX-NEXT:    movq %rdi, %rbx
-; AVX-NEXT:    movq %rdx, %rdi
-; AVX-NEXT:    movq %rcx, %rsi
+; AVX-NEXT:    subq $40, %rsp
+; AVX-NEXT:    vmovaps %xmm0, {{[0-9]+}}(%rsp) # 16-byte Spill
+; AVX-NEXT:    vmovaps %xmm1, %xmm0
 ; AVX-NEXT:    callq __fixtfdi
 ; AVX-NEXT:    vmovq %rax, %xmm0
 ; AVX-NEXT:    vmovdqa %xmm0, (%rsp) # 16-byte Spill
-; AVX-NEXT:    movq %rbx, %rdi
-; AVX-NEXT:    movq %r14, %rsi
+; AVX-NEXT:    vmovaps {{[0-9]+}}(%rsp), %xmm0 # 16-byte Reload
 ; AVX-NEXT:    callq __fixtfdi
 ; AVX-NEXT:    vmovq %rax, %xmm0
 ; AVX-NEXT:    vpunpcklqdq (%rsp), %xmm0, %xmm0 # 16-byte Folded Reload
 ; AVX-NEXT:    # xmm0 = xmm0[0],mem[0]
 ; AVX-NEXT:    vinsertps {{.*#+}} xmm0 = xmm0[0,2],zero,zero
-; AVX-NEXT:    addq $24, %rsp
-; AVX-NEXT:    popq %rbx
-; AVX-NEXT:    popq %r14
+; AVX-NEXT:    addq $40, %rsp
 ; AVX-NEXT:    retq
   %cvt = fptosi <2 x fp128> %a to <2 x i32>
   %ext = shufflevector <2 x i32> %cvt, <2 x i32> zeroinitializer, <4 x i32> <i32 0, i32 1, i32 2, i32 3>
   ret <4 x i32> %ext
 }