[PATCH] D13979: Introduction of FeatureX87

[Andrey Turetskiy via llvm-commits]

aturetsk added inline comments.

================
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(); }
 
----------------
aturetsk wrote:
> 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?
To sum up, the approach you're suggesting does make the compiler to behave as I want, since right now I'm only interested in having -mattr=-x87 to work correctly. And I'm ready to submit the updated patch. I'm just not sure that this approach is right from the general point of view...


http://reviews.llvm.org/D13979