Add FeatureX87 in X86 backend.
This is a preparatory changes for introducing a new CPU - Lakemont - which doesn't support X87 instructions.
Details
Diff Detail
Event Timeline
Hi Andrey,
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | 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. |
lib/Target/X86/X86.td | ||
---|---|---|
62 | 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. |
Hi Bruno and Simon,
Thanks for the review,
lib/Target/X86/X86.td | ||
---|---|---|
62 | 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. | |
lib/Target/X86/X86Subtarget.h | ||
406 | 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. |
You've done this on an old version of the sources, you'll need to rebase.
One other reply inline.
Thanks!
-eric
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | I agree with Bruno here. It also might be a good idea to not add this conditional at all and just use the legalizer. Thoughts? |
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | For now I fixed the condition as suggested. |
Hi Andrey,
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | 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. | |
test/CodeGen/X86/x87.ll | ||
6 ↗ | (On Diff #41387) | This is missing appropriate checks for instructions you want (or not) to be present in the output. |
Hi Bruno,
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | Thanks for the explanation. ; 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). | |
test/CodeGen/X86/x87.ll | ||
6 ↗ | (On Diff #41387) | 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? |
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | 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... |
lib/Target/X86/X86Subtarget.h | ||
---|---|---|
406 | 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. | |
test/CodeGen/X86/x87.ll | ||
6 ↗ | (On Diff #41387) | Ok. Please add simple cases when +x86 it's used as well! |
Hello Bruno,
I've added more logic in X86TargetLowering::X86TargetLowering as you suggested and improved the test covering different combinations of x87 and sse. Also I included a plenty of float convert instructions in the test since X86TargetLowering::X86TargetLowering contains non-trivial logic to handle them.
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
526 ↗ | (On Diff #45388) | Why check Subtarget->hasX87() here? We're using SSE for f32 and f64 anyways |
560 ↗ | (On Diff #45388) | Factor out "!Subtarget->useSoftFloat() && Subtarget->hasX87()" with bool UseX87 = !Subtarget->useSoftFloat() && Subtarget->hasX87(); and use that throughout the checks. |
596 ↗ | (On Diff #45388) | UseX87 here |
630 ↗ | (On Diff #45388) | UseX87 here |
test/CodeGen/X86/x87.ll | ||
7 ↗ | (On Diff #45388) | This test needs improvements; you can make it tighter by removing the allocas and other unnecessary instructions. Please explicitly check for all the specific instructions you want to match. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #46784) | I get X87 load and store instructions in x87.ll if I don't check hasX87 here. I think changing that would require significant efforts. Since we don't have a CPU which has -x87 but +sse2, I just left the check here. |
561 ↗ | (On Diff #46784) | Done. |
test/CodeGen/X86/x87.ll | ||
8 ↗ | (On Diff #46784) | Done. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #46784) | This looks odd, do you know why it happens? in which specific target feature combination? |
test/CodeGen/X86/x87.ll | ||
31 ↗ | (On Diff #46784) | Please place the checks above the IR instructions you intend to match. Also put a check-label in the beginning of the function. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #46784) | This happens with -x87,+sse2 in 32 bit mode (64 bit mode seems to be OK). if(Subtarget.hasX87() || Subtarget.is64Bit()) setOperationAction(ISD::SINT_TO_FP , MVT::i64 , Custom); However I still get the wrong fstp instruction: calll __floatdisf fstps 20(%esp) movss 20(%esp), %xmm0 # xmm0 = mem[0],zero,zero,zero But what worries me the most is that there may be more such non-obvious cases, even in the current version of the patch having the test passed. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #48485) | 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? |
test/CodeGen/X86/x87.ll | ||
32 ↗ | (On Diff #48485) | Done. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #48485) | 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. |
test/CodeGen/X86/x87.ll | ||
11 ↗ | (On Diff #48485) | There's no FileCheck invocation without check-prefix, so this isn't checking for anything. Use "X86-LABEL" and "NOX87-LABEL" instead. |
lib/Target/X86/X86ISelLowering.cpp | ||
---|---|---|
527 ↗ | (On Diff #48589) | I believe there is no such hardware and ABI and I agree with not bothering with "-x87,+sse2" case until it's really needed. |
test/CodeGen/X86/x87.ll | ||
12 ↗ | (On Diff #48589) | Fixed. |
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.