Page MenuHomePhabricator

Add "x87" in x86 target feature map
ClosedPublic

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

Diff Detail

Repository
rL LLVM

Event Timeline

aturetsk updated this revision to Diff 38114.Oct 22 2015, 5:11 AM
aturetsk retitled this revision from to Add "x87" in x86 target feature map.
aturetsk updated this object.
aturetsk added a reviewer: rsmith.
aturetsk added a subscriber: cfe-commits.
rsmith added inline comments.Nov 3 2015, 2:47 PM
lib/Basic/Targets.cpp
2516–2517 ↗(On Diff #38114)

i386 doesn't necessarily have x87, does it? IIRC the floating point co-processor is only guaranteed to be available in the 486 DX and later.

aturetsk updated this revision to Diff 40645.Nov 19 2015, 7:24 AM

Do not enable X87 for i386

Hello Richard,
Thank for the review.

lib/Basic/Targets.cpp
2538–2539 ↗(On Diff #40645)

You are right. Fixed.

Are there any of the intrinsics in the headers that also depend on x87?

One inline comment.

-eric

lib/Basic/Targets.cpp
2538–2539 ↗(On Diff #40645)

Mind only doing getCPUKind once here? :)

aturetsk updated this revision to Diff 41137.Nov 25 2015, 6:06 AM

Use getCPUKind once

Are there any of the intrinsics in the headers that also depend on x87?

Not that I could find.

lib/Basic/Targets.cpp
2545–2546 ↗(On Diff #41137)

Done.

rsmith added inline comments.Nov 25 2015, 3:27 PM
lib/Basic/Targets.cpp
2548 ↗(On Diff #41137)

What about CK_Generic? Also, if CK_i486 can be used for the 486SX, we need to exclude it here too. It looks (from wikipedia) like all the "WinChip" flavours of 486 have an x87 unit, but it'd be nice if someone could confirm that. Maybe we should have separate CPUKinds for 486 SX versus 486 DX.

aturetsk added inline comments.Nov 30 2015, 10:01 AM
lib/Basic/Targets.cpp
2548 ↗(On Diff #41137)

Before this changeset clang would generate x87 instructions for Generic, i386 and i486. So one can say they had FeatureX87 enabled implicitly. If that was ok formerly why do we want to change that now? Wouldn't it be better to keep clang's behavior unchanged?

rsmith added inline comments.Nov 30 2015, 11:30 AM
lib/Basic/Targets.cpp
2548 ↗(On Diff #41137)

OK, if this means "x87 instructions probably work" (although they may be emulated in software) and not "x87 instructions definitely work", then we shouldn't be checking the CPU kind here at all. It looks like the former is how GCC handles this; it has -mno-80387 to specify that we're building for an i386 with no support for x87 instructions. Should we handle that flag?

aturetsk updated this revision to Diff 41511.Dec 1 2015, 7:42 AM

Enable X87 back for all X86 processors.

aturetsk added inline comments.Dec 1 2015, 7:56 AM
lib/Basic/Targets.cpp
2551 ↗(On Diff #41511)

"x87 instructions probably work" is more like it, and having feature x87 disabled would mean "x87 instructions definitely don't work". That's what I intended. If feature soft float is specified it takes precedence over feature x87 causing compiler to generate calls instead of X87 instructions.
Support of -m80387/-mno-80387 seems to be a good thing to have in Clang to be compatible with GCC (although briefly looking at GCC's sources these options just seem to be synonyms to -mhard-float/-msoft-float). I can add the flags, but I think that's better to be done in a separate patch.

Hi,
The related LLVM patch (http://reviews.llvm.org/D13979) was approved. Is this patch ok for commit?

Can you add an explicit test for soft/hard-float.

One inline comment as well, waiting on Richard to pipe up.

-eric

lib/Basic/Targets.cpp
2603 ↗(On Diff #48904)

Waiting for rsmith to comment here.

rsmith added inline comments.Feb 26 2016, 1:13 AM
lib/Basic/Targets.cpp
2603 ↗(On Diff #48904)

This seems fine to me.

echristo accepted this revision.Feb 26 2016, 1:25 AM
echristo added a reviewer: echristo.

LGTM.

Thanks!

-eric

This revision is now accepted and ready to land.Feb 26 2016, 1:25 AM
This revision was automatically updated to reflect the committed changes.

The test Eric asked for was added.