[PATCH] D13979: Introduction of FeatureX87

[Andrey Turetskiy via llvm-commits]

aturetsk added a comment.

Hi Bruno and Simon,
Thanks for the review,


================
Comment at: lib/Target/X86/X86.td:62
@@ -58,3 +61,3 @@
 def FeatureMMX     : SubtargetFeature<"mmx","HasMMX", "true",
                                       "Enable MMX instructions">;
 
----------------
RKSimon wrote:
> You can reduce the size of the diff if you inherit FeatureX87 in FeatureMMX - all targets which declare FeatureMMX wouldn't then need to be altered.
I was thinking about it, but decided not to do so. From the technical point of view X87 has nothing to do with MMX so it makes sense to keep them unbound. Also I think it'd be good to have an opportunity to enable MMX/SSE/etc while having X87 disabled - I have no idea which features will be supported in Lakemont successors, but I don't see why that can't be the case.
So if you don't mind I'd stick to the current version of the patch.

================
Comment at: lib/Target/X86/X86Subtarget.h:406
@@ -401,3 +405,3 @@
   bool isSLM() const { return X86ProcFamily == IntelSLM; }
-  bool useSoftFloat() const { return UseSoftFloat; }
+  bool useSoftFloat() const { return UseSoftFloat || (!hasX87() && !hasSSE2()); }
 
----------------
bruno wrote:
> This looks odd since we do support f32 (not f64) with SSE1. See X86ISelLowering.cpp:553
> 
> } else if (!Subtarget->useSoftFloat() && X86ScalarSSEf32) {
>     // Use SSE for f32, x87 for f64.
>     // Set up the FP register classes.
>     addRegisterClass(MVT::f32, &X86::FR32RegClass);
>     addRegisterClass(MVT::f64, &X86::RFP64RegClass);
> ...
> 
> Since not having SSE at all fallbacks to x87, why not only check for "UseSoftFloat ||  !hasX87())" ? Anyway, I think those should come in a separate patch with appropriate feature testcases. 
My logic came from the hypothetical situation when we have SSE/SSE2, but not X87. If we have SSE there is no instructions to handle f64, thus without X87 we should use soft floats. SSE2 can handle f64 so there is no need to do that.


http://reviews.llvm.org/D13979