[PATCH] D13979: Introduction of FeatureX87

[Andrey Turetskiy via llvm-commits]

aturetsk added a comment.

Hi Bruno,


================
Comment at: lib/Target/X86/X86Subtarget.h:403
@@ -398,3 +402,3 @@
   bool isSLM() const { return X86ProcFamily == IntelSLM; }
-  bool useSoftFloat() const { return UseSoftFloat; }
+  bool useSoftFloat() const { return UseSoftFloat || !hasX87(); }
 
----------------
bruno wrote:
> As Eric mentioned, better to live the option check alone: "bool useSoftFloat() const { return UseSoftFloat }". Then change X86ISelLowering.cpp:589 this way:
> 
> Change
> 
>   } else if (!Subtarget->useSoftFloat()) {
> 
> To
> 
>   } else if (!Subtarget->useSoftFloat() && Subtarget->hasX87()) {
> 
> This should be enough to get the behaviour you want.
Thanks for the explanation.
I have some concern about such approach. Look at the test example:

```
; RUN: llc < %s -march=x86 -mattr=-x87,+sse

define float @test32(float %a, float %b) nounwind readnone {
entry:
        %0 = fadd float %a, %b
        ret float %0
; Generated assembly:
; pushl   %eax
; movss   8(%esp), %xmm0
; addss   12(%esp), %xmm0
; movss   %xmm0, (%esp)
; flds    (%esp)
; popl    %eax
; retl
}

define double @test64(double %a, double %b) nounwind readnone {
entry:
        %0 = fadd double %a, %b
        ret double %0
; Generated assembly:
; fldl    4(%esp)
; faddl   12(%esp)
; retl
}
```

The approach you suggest would generate x87 instructions (you can see the generated assembly in the comments), which is wrong since we have FeatureX87 disabled. The current version of the patch makes compiler to generate soft float calls in test32 and test64 (probably that's not the best assembly for test32 since we enabled sse, but at least it's correct).
I understand, that the combination "-x87,+sse" does not correspond to any existing CPU, yet llc gives an opportunity for user to use it through -mattr option. So shouldn't we care to generate correct assembly even in this case?

================
Comment at: test/CodeGen/X86/x87.ll:6
@@ +5,3 @@
+
+define float @foo(float %a, float %b) nounwind readnone {
+entry:
----------------
bruno wrote:
> This is missing appropriate checks for instructions you want (or not) to be present in the output.
In this test I just want to make sure that x87's fadd won't be generated. So why is "CHECK-NOT: fadd{{.*}}" not enough?


http://reviews.llvm.org/D13979