Page MenuHomePhabricator

[mips] Add the SoftFloat MipsSubtarget feature.
ClosedPublic

Authored by tomatabacu on Apr 16 2015, 9:06 AM.

Details

Summary

This will enable the IAS to reject floating point instructions if soft-float is enabled.

Diff Detail

Repository
rL LLVM

Event Timeline

tomatabacu updated this revision to Diff 23864.Apr 16 2015, 9:06 AM
tomatabacu retitled this revision from to [mips] Add the SoftFloat MipsSubtarget feature..
tomatabacu updated this object.
tomatabacu edited the test plan for this revision. (Show Details)
tomatabacu added reviewers: dsanders, echristo.
tomatabacu added subscribers: mpf, Unknown Object (MLST).
echristo edited edge metadata.Apr 20 2015, 2:05 PM

This, in general, looks good to me. You will probably have an issue with LegalizeDAG.cpp where it uses the value of UseSoftFloat from TargetOptions to determine whether/how to legalize something, but it's being actively worked on.

-eric

dsanders accepted this revision.Apr 21 2015, 3:03 AM
dsanders edited edge metadata.

I've got a few comments but nothing major. LGTM with those.

lib/Target/Mips/AsmParser/MipsAsmParser.cpp
90 ↗(On Diff #23864)

I don't think soft-float is an arch related feature. Does '.set mips32' and similar reset it?

lib/Target/Mips/Mips32r6InstrInfo.td
655 ↗(On Diff #23864)

We should extend PredicateControl with a SoftFloatPredicates member and add adjectives (HARDFLOAT/SOFTFLOAT?) to set that.

lib/Target/Mips/MipsInstrFPU.td
120 ↗(On Diff #23864)

This isn't related to your patch so no change required in this patch: NotFP64bit and IsFP64bit belong in PredicateControl.FGRPredicates, not in PredicateControl.AdditionalPredicates. The FGR_64 and FGR_32 adjectives can be used to do this.

There seems to be a few instances of this kind of thing left.

408–409 ↗(On Diff #23864)

Nit: Why break the line?

438 ↗(On Diff #23864)

Does this work with +soft-float ??

They require hardfloat.

This revision is now accepted and ready to land.Apr 21 2015, 3:03 AM
mpf added a comment.Apr 21 2015, 3:10 AM

I've got a few comments but nothing major. LGTM with those.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:90
@@ -90,1 +89,3 @@
+ Mips::FeatureFP64Bit | Mips::FeatureGP64Bit |
Mips::FeatureNaN2008 |
+ Mips::FeatureSoftFloat;


I don't think soft-float is an arch related feature. Does '.set mips32'
and similar reset it?

In general the .set options are supposed to be as independent as possible
there is unfortunate historic entanglement in binutils which means that
we have to allow .set mips<blah> to automatically set gp and fp defaults.
However, with fp=xx I believe the behaviour is that .set mips<blah>
does not force a specific fp setting when the current value is xx. The
other .set options only update one feature even if that produces a
slightly unusual overall configuration.

Thanks,
Matthew

tomatabacu updated this revision to Diff 24544.Apr 28 2015, 5:56 AM
tomatabacu edited edge metadata.

Removed redundant comment and newline.
Removed soft-float from AllArchRelatedMask.
Extended PredicateControl with a HardFloatPredicate, which is set by using HARDFLOAT.

Daniel, do you mind taking a look at that last change ?
I'm not sure if it fully addresses your concerns.

In D9053#158990, @mpf wrote:

I've got a few comments but nothing major. LGTM with those.

================
Comment at: lib/Target/Mips/AsmParser/MipsAsmParser.cpp:90
@@ -90,1 +89,3 @@
+ Mips::FeatureFP64Bit | Mips::FeatureGP64Bit |
Mips::FeatureNaN2008 |
+ Mips::FeatureSoftFloat;


I don't think soft-float is an arch related feature. Does '.set mips32'
and similar reset it?

In general the .set options are supposed to be as independent as possible
there is unfortunate historic entanglement in binutils which means that
we have to allow .set mips<blah> to automatically set gp and fp defaults.
However, with fp=xx I believe the behaviour is that .set mips<blah>
does not force a specific fp setting when the current value is xx. The
other .set options only update one feature even if that produces a
slightly unusual overall configuration.

Thanks,
Matthew

So this behaviour changes between fp=xx and other fp values? We'll need to look into that but I think it's something for a follow-on patch.

Removed redundant comment and newline.
Removed soft-float from AllArchRelatedMask.
Extended PredicateControl with a HardFloatPredicate, which is set by using HARDFLOAT.

Daniel, do you mind taking a look at that last change ?
I'm not sure if it fully addresses your concerns.

I spotted four def's that seem to have lost IsNotSoftFloat but otherwise it LGTM.

The only unresolved bit I can see is that we need to switch AdditionalPredicates<[NotFP64bit]> for FGR_32 and AdditionalPredicates<[IsFP64bit]> for FGR_64 but that's a change for another patch.

lib/Target/Mips/MipsInstrFPU.td
536–556 ↗(On Diff #24544)

We seem to have lost IsNotSoftFloat on these four def's. I don't see it in the base class.

mpf added a comment.May 6 2015, 9:15 AM

> In general the .set options are supposed to be as independent as
> possible there is unfortunate historic entanglement in binutils which
> means that we have to allow .set mips<blah> to automatically set gp
and fp defaults.
> However, with fp=xx I believe the behaviour is that .set mips<blah>
> does not force a specific fp setting when the current value is xx. The
> other .set options only update one feature even if that produces a
> slightly unusual overall configuration.
>
> Thanks,
> Matthew

So this behaviour changes between fp=xx and other fp values? We'll need
to look into that but I think it's something for a follow-on patch.

Yes, that's correct.

Matthew

tomatabacu updated this revision to Diff 25145.May 7 2015, 2:31 AM

Added HARDFLOAT to the 4 instruction definitions mentioned in the review.

tomatabacu closed this revision.May 7 2015, 3:33 AM
This revision was automatically updated to reflect the committed changes.