This is an archive of the discontinued LLVM Phabricator instance.

[ARM] Handle conflicts between -mfpu and -mfloat-abi options
ClosedPublic

Authored by rmaprath on Sep 23 2014, 2:55 AM.

Details

Reviewers
rengolin
Summary

Currently we allow combinations of options like the ones below:

./clang -target <...> -mfpu=vfp4 -mfloat-abi=soft ...
./clang -target <...> -mfloat-abi=hard -mfpu=none ...

Such usage should be warned / prevented because those options conflict in meaning. This patch implements the following scheme when presented with such options:

-mfloat-abi=soft -mfpu=FPU => warn, ignore the FPU, use soft-float

-mfloat-abi=softfp -mfpu=none => warn, use soft-float

-mfloat-abi=hard -mfpu=none => downgradable error, use soft-float

Diff Detail

Event Timeline

rmaprath updated this revision to Diff 13980.Sep 23 2014, 2:55 AM
rmaprath retitled this revision from to [ARM] Handle conflicts between -mfpu and -mfloat-abi options.
rmaprath updated this object.
rmaprath edited the test plan for this revision. (Show Details)
rmaprath added a subscriber: Unknown Object (MLST).
rengolin added inline comments.
lib/Driver/Tools.cpp
578

I'd rather fix this than warn on inconsistent behaviour of an inconsistent set of flags.

rmaprath added inline comments.Sep 24 2014, 6:43 AM
lib/Driver/Tools.cpp
578

Hi Renato,

Thank you for the comment.

I don't think fixing this will help us get rid of the warnings, because someone can still provide -msoft-float and -mfpu=<fpu_name> even if we separate out the meanings of -mfloat-abi and -msoft-float / -mhard-float on the clang interface.

WDYT?

Thanks.

I agree, but making these warnings on -mfloat-abi doesn't make sense.

In D5460#7, @rengolin wrote:

I agree, but making these warnings on -mfloat-abi doesn't make sense.

This would mean getting rid of the -mfloat-abi=softfp option, which is kind of a hack to piggyback -mhard-float into the -mfloat-abi option. I will look into this and see what I can come up with.

Thank you for the comments.

  • Asiri
In D5460#7, @rengolin wrote:

I agree, but making these warnings on -mfloat-abi doesn't make sense.

Hi Renato,

After a quick look at gcc options [1], it seems getting rid of -mfloat-abi=softfp would break compatibility. So, something like -mfloat-abi=softfp -mfpu=none still deserves a warning. Even worse for -mfloat-abi=hard -mfpu=none.

GCC gets away with this because they do not have a -mfpu=none option.

WDYT?

[1] https://gcc.gnu.org/onlinedocs/gcc/ARM-Options.html

In D5460#9, @rmaprath wrote:

After a quick look at gcc options [1], it seems getting rid of -mfloat-abi=softfp would break compatibility. So, something like -mfloat-abi=softfp -mfpu=none still deserves a warning. Even worse for -mfloat-abi=hard -mfpu=none.

We shouldn't get rid of -mfloat-abi=softfp, I agree. The warning is relevant there, as this is *also* a hack in GCC.

cheers,
--renato

We shouldn't get rid of -mfloat-abi=softfp, I agree. The warning is relevant there, as this is *also* a hack in GCC.

Ok to commit then? The remaining warning is for:

-mfloat-abi=soft -mfpu=<some_fpu>

Here we're warning that -mfpu will be ignored, because at the moment -mfloat-abi=soft also means -msoft-float. Perhaps I should get rid of this warning, as the "correct" interpretation of the two flags actual makes sense. WDYT?

Thanks.

  • Asiri
In D5460#11, @rmaprath wrote:

We shouldn't get rid of -mfloat-abi=softfp, I agree. The warning is relevant there, as this is *also* a hack in GCC.

Ok to commit then? The remaining warning is for:

-mfloat-abi=soft -mfpu=<some_fpu>

Here we're warning that -mfpu will be ignored, because at the moment -mfloat-abi=soft also means -msoft-float. Perhaps I should get rid of this warning, as the "correct" interpretation of the two flags actual makes sense. WDYT?

Perhaps it's better to remove this warning (from the patch) and interpret this combination as: -mfloat-abi=soft -mhard-float -mfpu=<some_fpu> which is what the original combination really intends to achieve.

Hope this sounds good.

Cheers!

\-\- Asiri

Yes, that'd be the correct assumption, I think.

rmaprath updated this revision to Diff 14072.Sep 25 2014, 7:31 AM

Hi Renato,

I've updated the patch with the [-mfloat-abi=soft -mfpu=<fpu>] warning removed and strengthening the test cases.

Will commit if there's no objection.

Best,

--Asiri

  • if (Arg *A = Args.getLastArg(options::OPT_msoft_float,
  • options::OPT_mhard_float,
  • options::OPT_mfloat_abi_EQ)) {

+ const Arg *A = Args.getLastArg(options::OPT_msoft_float,
+ options::OPT_mhard_float,
+ options::OPT_mfloat_abi_EQ);
+ if (A) {

if (A->getOption().matches(options::OPT_msoft_float))
  FloatABI = "soft";
else if (A->getOption().matches(options::OPT_mhard_float))

@@ -591,6 +601,31 @@

  }
}

+ Some -mfpu=... options are incompatible with some mfloat-abi=... options
+ if (Arg *B = Args.getLastArg(options::OPT_mfpu_EQ)) {
+ StringRef FPU = B->getValue();
+ if (FPU == "none") {
+
Signal incompatible -mfloat-abi=... options
+ if (FloatABI == "hard")
+ D.Diag(diag::warn_drv_implied_soft_float_conflict)
+ << B->getAsString(Args) << A->getAsString(Args);
+ else if (FloatABI == "softfp")
+ D.Diag(diag::warn_drv_implied_soft_float_assumed)
+ << B->getAsString(Args) << A->getAsString(Args);

It might be that I'm not seeing quite enough context here without applying the
patch, but how do you prevent dereferencing nullptr in A when there's neither
OPT_msoft_float, OPT_mhard_float, nor OPT_mfloat_abi_EQ?

Cheers,

Jon

Hi Jon,

+ Some -mfpu=... options are incompatible with some mfloat-abi=... options
+ if (Arg *B = Args.getLastArg(options::OPT_mfpu_EQ)) {
+ StringRef FPU = B->getValue();
+ if (FPU == "none") {
+
Signal incompatible -mfloat-abi=... options
+ if (FloatABI == "hard")
+ D.Diag(diag::warn_drv_implied_soft_float_conflict)
+ << B->getAsString(Args) << A->getAsString(Args);
+ else if (FloatABI == "softfp")
+ D.Diag(diag::warn_drv_implied_soft_float_assumed)
+ << B->getAsString(Args) << A->getAsString(Args);

It might be that I'm not seeing quite enough context here without applying the
patch, but how do you prevent dereferencing nullptr in A when there's neither
OPT_msoft_float, OPT_mhard_float, nor OPT_mfloat_abi_EQ?

FloatABI would only be set if A is not null at this point. Although, I could include A in the if condition to make this code less brittle. But it would be redundant as it stands.

I'm thinking of leaving the code as it is. In case if a future change means FloatABI could be set at this point without A being set, the tests would crash. WDYT?

Thank you for the catch!

--Asiri

Cheers,

Jon

Overall looks good, but I think that this change will also change other ARM architectures (v4~v7) and would be good to add the same login on the prev8 side of things.

Also, can you generate the diff with "-U99" so we can see the rest of the context?

Thanks!
--renato

lib/Driver/Tools.h
617

why not put this at the end of the argument list and make it NULL by default?

rmaprath updated this revision to Diff 14218.Sep 30 2014, 5:45 AM

Hi Renato,

I've been away and only now managed to pull this back. I've addressed your comment on the code but I don't understand what you mean by:

"but I think that this change will also change other ARM architectures (v4~v7) and would be good to add the same login on the prev8 side of things"

AFAICS, this code gets executed for all ARM architectures. Can you kindly explain what you meant by "add the same login"?

Cheers!

--Asiri

In D5460#20, @rmaprath wrote:

AFAICS, this code gets executed for all ARM architectures. Can you kindly explain what you meant by "add the same login"?

I have no idea, it makes no sense, probably dyslexia. Sorry about that.

My concern was that you've only added tests for armv8, so for example, the CHECK-SOFT-FLOAT should probably check for -vfp* too.

cheers,
--renato

Hi Renato,

My concern was that you've only added tests for armv8, so for example, the CHECK-SOFT-FLOAT should probably check for -vfp* too.

I already have the following tests:

// SOFT-ABI-FP: "-target-feature" "-neon"                                                                                                                                         
// SOFT-ABI-FP: "-target-feature" "-crypto"                                                                                                                                       
// SOFT-ABI-FP: "-target-feature" "-vfp2"                                                                                                                                         
// SOFT-ABI-FP: "-target-feature" "-vfp3"                                                                                                                                         
// SOFT-ABI-FP: "-target-feature" "-vfp4"

AFAICS The architecture does not play a role in determining the floating point ABI / ops. The underlying platform sometimes infer floating point ABI / ops, and I have a test for that (DEFAULT-FP).

Do you want me to add tests for pre-v8 architectures anyways?

Thanks!

--Asiri

In D5460#23, @rmaprath wrote:

AFAICS The architecture does not play a role in determining the floating point ABI / ops. The underlying platform sometimes infer floating point ABI / ops, and I have a test for that (DEFAULT-FP).

Not yet, but this might change in the future by a broken patch checking for "if (v8) ...".

Do you want me to add tests for pre-v8 architectures anyways?

Yes, just add the same checks below:

// CHECK-SOFT-FLOAT: "-target-feature" "-neon"
rmaprath updated this revision to Diff 14313.Oct 2 2014, 2:15 AM

Hi Renato,

I've attached an updated patch with extra test cases (pre-v8).

Best,

--Asiri

rengolin accepted this revision.Oct 2 2014, 2:23 AM
rengolin added a reviewer: rengolin.

LGTM, Thanks!

This revision is now accepted and ready to land.Oct 2 2014, 2:23 AM

Thanks Renato!

Committed as r218863.

Best,

--Asiri

rmaprath closed this revision.Oct 2 2014, 3:15 AM

Broken build-bot. I'm on it.

Best,

--Asiri