[PATCH] D13979: Introduction of FeatureX87

[Bruno Cardoso Lopes via llvm-commits]

bruno added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:527
@@ -525,3 +526,3 @@
 
-  if (!Subtarget.useSoftFloat() && X86ScalarSSEf64) {
+  if (UseX87 && X86ScalarSSEf64) {
     // f32 and f64 use SSE.
----------------
aturetsk wrote:
> The reason why the float store has appeared is that when we lower call (to the soft float function in this case) we use f80 type for float return if we have SSE. Here is the code snippet from X86ISelLowering.cpp:2428:
> ```
>     // If we prefer to use the value in xmm registers, copy it out as f80 and
>     // use a truncate to move it from fp stack reg to xmm reg.
>     bool RoundAfterCopy = false;
>     if ((VA.getLocReg() == X86::FP0 || VA.getLocReg() == X86::FP1) &&
>         isScalarFPTypeInSSEReg(VA.getValVT())) {
>       CopyVT = MVT::f80;
>       RoundAfterCopy = (CopyVT != VA.getLocVT());
>     }
> ```
> The comment in the code explains what's happened. Thus, currently even when we use SSE2 we still rely on the fact that we have X87 and changing that is not trivial.
> 
> Now looking at this I'm really inclined to follow Simons advice to inherit FeatureX87 in FeatureMMX. This way SSE/SSE2 will imply X87 which makes a perfect sense since they rely on it.
> 
> What do you think?
Is there any actual any hardware that supports -x87,+sse2? In the same line of thought: is there any ABI definition that describes calling convention for this situation?

If not I suggest we don't need to care about this case, though a report_fatal_error sanity check (see the one right above the code snippet you pointed out) in lower call for the "32-bit x86 -x87,+sse2" would be nice, since we don't support it.

Inheriting FeatureX87 in FeatureMMX makes it easier for implementation purposes but they look orthogonal to me; if anyone is willing to support -x87,+sse2, it should be supported with code to handle it plus tests. Your patch seems to add FeatureX87 to all current CPUs we support, and that should be enough to guarantee it won't break anything. That said, you can also remove "-x87,+sse2" from your tests.

================
Comment at: test/CodeGen/X86/x87.ll:11
@@ +10,3 @@
+define void @test(i32 %i, i64 %l, float* %pf, double* %pd, fp128* %pld) nounwind readnone {
+; CHECK-LABEL: test:
+; X87: fild
----------------
There's no FileCheck invocation without check-prefix, so this isn't checking for anything. Use "X86-LABEL" and "NOX87-LABEL" instead.


http://reviews.llvm.org/D13979