[PATCH] D13979: Introduction of FeatureX87

[Bruno Cardoso Lopes via llvm-commits]

bruno added a reviewer: 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(); }
 
----------------
aturetsk wrote:
> 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...
Hi Andrey,

Sorry for not mentioning it explicitly but the idea here is that we should only use "Subtarget->useSoftFloat()" to represent the state for the soft-float feature whereas the logic of selecting what is actually supported should be done in X86TargetLowering::X86TargetLowering (something along the lines of the snippet I used, but if that's not enough, please add more logic to guarantee it). Does that make sense?

Feel free to address it in a upcoming patch if you wish but remember to add the "-mattr=-x87,+sse" tests when you do so. 

================
Comment at: test/CodeGen/X86/x87.ll:6
@@ +5,3 @@
+
+define float @foo(float %a, float %b) nounwind readnone {
+entry:
----------------
aturetsk wrote:
> 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?
Ok. Please add simple cases when +x86 it's used as well!


http://reviews.llvm.org/D13979