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

[rmaprath]

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

[rengolin]

lib/Driver/Tools.cpp

578

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

[rmaprath]

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.

[rengolin]

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

[rmaprath]

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

[rmaprath]

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

[rengolin]

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

[rmaprath]

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

[rmaprath]

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

[rengolin]

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

[rmaprath]

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

[jroelofs]

• 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

[rmaprath]

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

[rengolin]

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]

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

[rengolin]

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

[rmaprath]

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

[rengolin]

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]

Hi Renato,

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

Best,

--Asiri

[rengolin]

LGTM, Thanks!

[rmaprath]

Thanks Renato!

Committed as r218863.

Best,

--Asiri

[rmaprath]

Broken build-bot. I'm on it.

Best,

--Asiri

include/clang/Basic/DiagnosticDriverKinds.td

   InGroup<UnusedCommandLineArgument>;
 def warn_drv_empty_joined_argument : Warning<
   "joined argument expects additional value: '%0'">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_clang_unsupported : Warning<
   "the clang compiler does not support '%0'">;
 def warn_drv_assuming_mfloat_abi_is : Warning<
   "unknown platform, assuming -mfloat-abi=%0">;
+def warn_drv_implied_soft_float_assumed : Warning<
+  "%0 implies soft-float, ignoring conflicting option '%1'">,
+  InGroup<DiagGroup<"implied-soft-float-assumed">>;
+def warn_drv_implied_soft_float_conflict : Warning<
+  "%0 implies soft-float, which conflicts with option '%1'">,
+  InGroup<DiagGroup<"implied-soft-float-conflict">>, DefaultError;
 def warn_ignoring_ftabstop_value : Warning<
   "ignoring invalid -ftabstop value '%0', using default value %1">;
 def warn_drv_overriding_flag_option : Warning<
   "overriding '%0' option with '%1'">,
   InGroup<DiagGroup<"overriding-t-option">>;
 def warn_drv_treating_input_as_cxx : Warning<
   "treating '%0' input as '%1' when in C++ mode, this behavior is deprecated">,
   InGroup<Deprecated>;

lib/Driver/Tools.h

                                         const InputInfoList &Inputs,
                                         const llvm::opt::ArgList &TCArgs,
                                         const char *LinkingOutput) const;
   };
 } // end namespace visualstudio
 
 namespace arm {
   StringRef getARMFloatABI(const Driver &D, const llvm::opt::ArgList &Args,
-                         const llvm::Triple &Triple);
+                           const llvm::Triple &Triple,
+                           bool *ExplicitNoFloat = NULL);
 }
 namespace XCore {
   // For XCore, we do not need to instantiate tools for PreProcess, PreCompile and Compile.
   // We simply use "clang -cc1" for those actions.
   class LLVM_LIBRARY_VISIBILITY Assemble : public Tool {
   public:
     Assemble(const ToolChain &TC) : Tool("XCore::Assemble",
       "XCore-as", TC) {}

lib/Driver/Tools.cpp

     Features.push_back("-vfp4");
     Features.push_back("-fp-armv8");
     Features.push_back("-crypto");
     Features.push_back("-neon");
   } else
     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 }
 
-// Select the float ABI as determined by -msoft-float, -mhard-float, and
-// -mfloat-abi=.
+// Select the float ABI as determined by -msoft-float, -mhard-float,
+// -mfpu=... and -mfloat-abi=...
 StringRef tools::arm::getARMFloatABI(const Driver &D, const ArgList &Args,
-                                     const llvm::Triple &Triple) {
+                                     const llvm::Triple &Triple,
+                                     bool *ExplicitNoFloat) {
+
+  // FIXME: -msoft-float and -mhard-float identify the nature of floating point
+  // ops, whereas -mfloat-abi=... identifies the floating point argument passing
+  // convention. But here we are mushing them together into FloatABI = soft/
+  // softfp/hard, which is a bit confusing. For example, this means -msoft-float
+  // and -mfloat-abi=soft are equivalent options to clang. But when we pass
+  // arguments to the backend (AddARMTargetArgs), these two options take their
+  // original interpretations.
   StringRef FloatABI;
-  if (Arg *A = Args.getLastArg(options::OPT_msoft_float,
+  const Arg *A = Args.getLastArg(options::OPT_msoft_float,
                                  options::OPT_mhard_float,
-                               options::OPT_mfloat_abi_EQ)) {
+                                 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))
       FloatABI = "hard";
     else {
       FloatABI = A->getValue();
       if (FloatABI != "soft" && FloatABI != "softfp" && FloatABI != "hard") {
         D.Diag(diag::err_drv_invalid_mfloat_abi)
           << A->getAsString(Args);
         FloatABI = "soft";
       }
     }
   }
 
+  // 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);
+      // Assume soft-float
+      FloatABI = "soft";
+    } else if (FloatABI.empty() || FloatABI == "soft") {
+      // Need -mhard-float for floating point ops
+      FloatABI = "softfp";
+    } 
+  }
+
+  // This allows us to differentiate between a user specified soft-float and
+  // an inferred soft-float. The latter may be overridden under other conditions
+  // but the former has higher priority.
+  if (FloatABI == "soft" && ExplicitNoFloat)
+    *ExplicitNoFloat = true;
+
   // If unspecified, choose the default based on the platform.
   if (FloatABI.empty()) {
     switch (Triple.getOS()) {
     case llvm::Triple::Darwin:
     case llvm::Triple::MacOSX:
     case llvm::Triple::IOS: {
       // Darwin defaults to "softfp" for v6 and v7.
       //

 
   return FloatABI;
 }
 
 static void getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                                  const ArgList &Args,
                                  std::vector<const char *> &Features,
                                  bool ForAS) {
-  StringRef FloatABI = tools::arm::getARMFloatABI(D, Args, Triple);
+  bool ExplicitNoFloat = false;
+  StringRef FloatABI = tools::arm::getARMFloatABI(D, Args, Triple, &ExplicitNoFloat);
   if (!ForAS) {
     // FIXME: Note, this is a hack, the LLVM backend doesn't actually use these
     // yet (it uses the -mfloat-abi and -msoft-float options), and it is
     // stripped out by the ARM target. We should probably pass this a new
     // -target-option, which is handled by the -cc1/-cc1as invocation.
     //
     // FIXME2:  For consistency, it would be ideal if we set up the target
     // machine state the same when using the frontend or the assembler. We don't

     getARMHWDivFeatures(D, A, Args, Features);
 
   // Setting -msoft-float effectively disables NEON because of the GCC
   // implementation, although the same isn't true of VFP or VFP3.
   if (FloatABI == "soft") {
     Features.push_back("-neon");
     // Also need to explicitly disable features which imply NEON.
     Features.push_back("-crypto");
+    // Disable remaining floating-point features if soft-float is explicitly
+    // requested.
+    if (ExplicitNoFloat) {
+      Features.push_back("-vfp2");
+      Features.push_back("-vfp3");
+      Features.push_back("-vfp4");
+      Features.push_back("-fp-armv8");
+    }
   }
 
   // En/disable crc
   if (Arg *A = Args.getLastArg(options::OPT_mcrc,
                                options::OPT_mnocrc)) {
     if (A->getOption().matches(options::OPT_mcrc))
       Features.push_back("+crc");
     else

test/Driver/arm-mfpu.c

 // RUN: %clang -target armv8-linux-gnueabihf -mfpu=crypto-neon-fp-armv8 %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-CRYPTO-NEON-FP-ARMV8 %s
 // CHECK-CRYPTO-NEON-FP-ARMV8: "-target-feature" "+fp-armv8"
 // CHECK-CRYPTO-NEON-FP-ARMV8: "-target-feature" "+neon"
 // CHECK-CRYPTO-NEON-FP-ARMV8: "-target-feature" "+crypto"
 
 // RUN: %clang -target armv8-linux-gnueabi -mfpu=none %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-NO-FP %s
+// CHECK-NO-FP: "-target-feature" "-neon"
+// CHECK-NO-FP: "-target-feature" "-crypto"
 // CHECK-NO-FP: "-target-feature" "-vfp2"
 // CHECK-NO-FP: "-target-feature" "-vfp3"
 // CHECK-NO-FP: "-target-feature" "-vfp4"
 // CHECK-NO-FP: "-target-feature" "-fp-armv8"
-// CHECK-NO-FP: "-target-feature" "-crypto"
-// CHECK-NO-FP: "-target-feature" "-neon"
 
 // RUN: %clang -target arm-linux-gnueabihf %s -### 2>&1 \
 // RUN:   | FileCheck --check-prefix=CHECK-HF %s
 // CHECK-HF: "-target-cpu" "arm1136jf-s"
 
 // RUN: %clang -target armv7-apple-darwin -x assembler %s -### -c 2>&1 \
 // RUN:   | FileCheck --check-prefix=ASM %s
 // ASM-NOT: -target-feature
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none -mfloat-abi=soft %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI-FP %s
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none -msoft-float %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI-FP %s
+// RUN: %clang -target armv8-linux-gnueabi -mfloat-abi=soft %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI-FP %s
+// RUN: %clang -target armv8-linux-gnueabi -msoft-float %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI-FP %s
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI-FP %s
+// SOFT-ABI-FP-NOT: error:
+// SOFT-ABI-FP-NOT: warning:
+// SOFT-ABI-FP-NOT: "-target-feature" "+{{[^ ]*fp[^ ]*}}"
+// 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"
+// SOFT-ABI-FP: "-target-feature" "-fp-armv8"
+// SOFT-ABI-FP-NOT: "-target-feature" "+{{[^ ]*fp[^ ]*}}"
+// SOFT-ABI-FP: "-msoft-float"
+// SOFT-ABI-FP: "-mfloat-abi" "soft"
+// SOFT-ABI-FP-NOT: "-target-feature" "+{{[^ ]*fp[^ ]*}}"
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none -mfloat-abi=softfp %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=ABI-IGNORE %s
+// ABI-IGNORE: warning: -mfpu=none implies soft-float, ignoring conflicting option '-mfloat-abi=softfp'
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none -mfloat-abi=hard %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=ABI-ERROR %s
+// ABI-ERROR: error: -mfpu=none implies soft-float, which conflicts with option '-mfloat-abi=hard'
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=none -mhard-float %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=FP-ERROR %s
+// FP-ERROR: error: -mfpu=none implies soft-float, which conflicts with option '-mhard-float'
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=vfp4 -mfloat-abi=soft %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI %s
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=vfp4 -mfloat-abi=softfp %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI %s
+// RUN: %clang -target armv8-freebsd-gnueabi -mfpu=vfp4 %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=SOFT-ABI %s
+// SOFT-ABI-NOT: error:
+// SOFT-ABI-NOT: warning:
+// SOFT-ABI-NOT: "-msoft-float"
+// SOFT-ABI: "-target-feature" "+vfp4"
+// SOFT-ABI-NOT: "-msoft-float"
+// SOFT-ABI: "-mfloat-abi" "soft"
+// SOFT-ABI-NOT: "-msoft-float"
+
+// Floating point features should be disabled only when explicitly requested,
+// otherwise we must respect target inferred defaults.
+//
+// RUN: %clang -target armv8-freebsd-gnueabi %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=DEFAULT-FP %s
+// DEAFULT-FP-NOT: error:
+// DEFAULT-FP-NOT: warning:
+// DEFAULT-FP-NOT: "-target-feature" "-{{[^ ]*fp[^ ]*}}"
+// DEFAULT-FP: "-msoft-float" "-mfloat-abi" "soft"
+// DEFAULT-FP-NOT: "-target-feature" "-{{[^ ]*fp[^ ]*}}"
+
+// RUN: %clang -target armv8-linux-gnueabi -mfpu=vfp4 -mfloat-abi=hard %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=VFP-ABI %s
+// VFP-ABI-NOT: error:
+// VFP-ABI-NOT: warning:
+// VFP-ABI-NOT: "-msoft-float"
+// VFP-ABI: "-target-feature" "+vfp4"
+// VFP-ABI-NOT: "-msoft-float"
+// VFP-ABI: "-mfloat-abi" "hard"
+// VFP-ABI-NOT: "-msoft-float"