Page MenuHomePhabricator

Introduction of FeatureX87
ClosedPublic

Authored by aturetsk on Oct 22 2015, 5:00 AM.

Details

Summary

Add FeatureX87 in X86 backend.
This is a preparatory changes for introducing a new CPU - Lakemont - which doesn't support X87 instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

aturetsk updated this revision to Diff 38112.Oct 22 2015, 5:00 AM
aturetsk retitled this revision from to Introduction of FeatureX87.
aturetsk updated this object.
aturetsk added a reviewer: nadav.
aturetsk added a subscriber: llvm-commits.
nadav edited edge metadata.Nov 2 2015, 7:28 AM
nadav added a subscriber: nadav.

LGTM.

bruno added a subscriber: bruno.Nov 2 2015, 10:05 AM

Hi Andrey,

lib/Target/X86/X86Subtarget.h
406 ↗(On Diff #38112)

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.

RKSimon added a subscriber: RKSimon.Nov 2 2015, 1:57 PM
RKSimon added inline comments.
lib/Target/X86/X86.td
62 ↗(On Diff #38112)

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 ↗(On Diff #38112)

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.

lib/Target/X86/X86Subtarget.h
406 ↗(On Diff #38112)

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 ↗(On Diff #38112)

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?

aturetsk updated this revision to Diff 41138.Nov 25 2015, 6:13 AM
aturetsk edited edge metadata.

Rebased and fixed questionable condition in useSoftFloat.

aturetsk added inline comments.Nov 25 2015, 6:27 AM
lib/Target/X86/X86Subtarget.h
403 ↗(On Diff #41138)

For now I fixed the condition as suggested.
Eric, could you please explain in more details how you suggest to use legalizar?

aturetsk updated this revision to Diff 41387.Nov 30 2015, 4:51 AM

Added the test.

bruno added a comment.Dec 14 2015, 9:55 AM

Hi Andrey,

lib/Target/X86/X86Subtarget.h
403 ↗(On Diff #41387)

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
403 ↗(On Diff #41387)

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?

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?

aturetsk added inline comments.Dec 15 2015, 7:56 AM
lib/Target/X86/X86Subtarget.h
403 ↗(On Diff #41387)

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...

bruno added a reviewer: bruno.Jan 5 2016, 4:55 PM
bruno added inline comments.
lib/Target/X86/X86Subtarget.h
403 ↗(On Diff #41387)

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!

aturetsk updated this revision to Diff 45388.Jan 20 2016, 7:24 AM

Fixed and rebased.

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.

bruno added inline comments.Jan 20 2016, 9:41 AM
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.

aturetsk updated this revision to Diff 46784.Feb 3 2016, 7:38 AM

Fix remarks.

aturetsk added inline comments.Feb 3 2016, 8:24 AM
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.

bruno added inline comments.Feb 3 2016, 11:35 AM
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.

aturetsk added inline comments.Feb 4 2016, 6:05 AM
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).
The instruction to blame is "sitofp i64 %l to float". Actually I was able to get rid of fild instruction and have a soft float call by adding the condition at line 213:

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.
Probably we should use more straight-forward and easier way (until we have a real case where the best code for -x87,+sse/sse2 is required) - to have a variable "UseX87 = !useSoftFloat() && hasX87()" and replace useSoftFloat() with !useX87 absolutely everywhere (similar to what was done initially, but keep useSoftFloat() unchanged through the use of a new variable).
This way would guarantee that the compiler generates correct code with disabled x87.

aturetsk updated this revision to Diff 48485.Feb 19 2016, 6:41 AM

Rebase and improve the test.

aturetsk added inline comments.Feb 19 2016, 7:10 AM
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.

bruno added inline comments.Feb 19 2016, 10:17 AM
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.

echristo edited edge metadata.Feb 19 2016, 4:24 PM

(I've been quiet since I haven't had anything to add to Bruno's review)

aturetsk updated this revision to Diff 48589.Feb 20 2016, 5:03 AM
aturetsk edited edge metadata.

Fix the test, add sanity check in the legalizer.

aturetsk added inline comments.Feb 20 2016, 5:10 AM
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.
So I left UseX87 checks in legalizer only in the places where X87 float register classes are used and added a sanity check (note that it should not be 32-bit specific, it's just happened so that the issue I described had appeared in my testcase in 32 bit mode).

test/CodeGen/X86/x87.ll
12 ↗(On Diff #48589)

Fixed.

bruno accepted this revision.Feb 22 2016, 10:18 AM
bruno edited edge metadata.

Thanks Andrey, LGTM

This revision is now accepted and ready to land.Feb 22 2016, 10:18 AM
This revision was automatically updated to reflect the committed changes.