[ARM] Implement -Wa,-mfpu and friends for assemblers

[rengolin]

Summary

This patch allows Clang to pass on -Wa,-mfpu, -Wa,-mhwdiv and
-Wa,-mcpu to the integrated assembler (via target-features), but
-march is still not being passed, but validated.

Fixes PR20700. Depends on D11147.

[rengolin]

More info.

[rengolin]

Gentle ping. I'd like to get this into 3.7.

[joerg]

I find it confusing that both -mcpu and -Wa,-mcpu are honoured. Shouldn't be warn about conflicts?

[rengolin]

In D11148#207698, @joerg wrote:

I find it confusing that both -mcpu and -Wa,-mcpu are honoured. Shouldn't be warn about conflicts?

My reasoning is that in build systems, CFLAGS is set differently, and where people use the compiler to assemble files, it's common to have ASFLAGS as a composition of CFLAGS and extra options, like -Wa flags. FWIW, GCC doesn't warn either.

[joerg]

My point remains that -mcpu is already passed to the assembler, so having both -Wa,-mcpu and -mcpu and using one over the other *independent* of order is violates POLA.

[rengolin]

So, what do you suggest?

A warning might break -Werror builds that already depend on this option to work.

Checking the last option is not possible using filters, since they have different types.

Replacing -Wa,-mfpu by -mfpu Arg inplace (to allow for last-of filtering) is not possible, since the ArgList is const.

When there is only -Wa and no -mfpu, not doing anything gets the wrong behaviour.

Any other ideas?

cheers,
--renato

[rnk]

Sorry for ignoring this. I looked at it, didn't like the approach, but couldn't think of a better one.

Here's a dumb idea: what if we alias -Wa,-mcpu= to -mcpu=? This would have the unintended side effect of making -Wa,-mcpu affect the CPU used for *compilation* in addition to assembly, but that seems like not a very big deal. If that's OK, it solves the whole getLastArg problem without complicating the driver C++ code.

lib/Driver/Tools.cpp

536

You probably want to clang-format the patch.

[rengolin]

Now warning over double use of naked -mxxx and asm -Wa,-mxxx.

Also, improving the tests.

[rengolin]

In D11148#209873, @rnk wrote:

Sorry for ignoring this. I looked at it, didn't like the approach, but couldn't think of a better one.

Welcome to my world... This is not my first attempt... :)

Here's a dumb idea: what if we alias -Wa,-mcpu= to -mcpu=? This would have the unintended side effect of making -Wa,-mcpu affect the CPU used for *compilation* in addition to assembly, but that seems like not a very big deal. If that's OK, it solves the whole getLastArg problem without complicating the driver C++ code.

It would, but the behaviour that makes most sense if for the -Wa to override the naked options for the assembler, and never be used for the compiler. If we merge them, -Wa options will work for compilation, too, and won't override naked options when it should.

[rengolin]

Clang format

[rengolin]

Ping?

I know this is boring, but it's an important missing feature.

[rnk]

lgtm

lib/Driver/Tools.cpp

742–743

Can you make the capitalization of the 'd' in HDivArg and WaHdiv match? I don't care which way you go.

[rengolin]

Thanks! r243353

lib/Driver/Tools.cpp

 // Get Arch/CPU from args.
 static void getARMArchCPUFromArgs(const ArgList &Args,
                                   llvm::StringRef &Arch,
-                                  llvm::StringRef &CPU) {
+                                  llvm::StringRef &CPU,
+                                  bool FromAs=false) {
   if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ))
     CPU = A->getValue();
   if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
     Arch = A->getValue();
+  if (!FromAs)
+    return;
+
+  for (const Arg *A : Args.filtered(options::OPT_Wa_COMMA,
+                                    options::OPT_Xassembler)) {
+    StringRef Value = A->getValue();
+    if (Value.startswith("-mcpu="))
+        CPU = Value.substr(6);
+    if (Value.startswith("-march="))
+        Arch = Value.substr(7);
+  }
 }
 
 // Handle -mhwdiv=.
+// FIXME: Use ARMTargetParser.
 static void getARMHWDivFeatures(const Driver &D, const Arg *A,
                                 const ArgList &Args,
                                 StringRef HWDiv,

     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 }
 
-// Check -march=.
+// Check if -march is valid by checking if it can be canonicalised and parsed.
+// getARMArch is used here instead of just checking the -march value in order
+// to handle -march=native correctly.
 static void checkARMArchName(const Driver &D, const Arg *A,
                              const ArgList &Args,
                              llvm::StringRef ArchName,

     D.Diag(diag::err_drv_clang_unsupported) << A->getAsString(Args);
 }
 
-// Check -mcpu=.
+// Check -mcpu=. Needs ArchName to handle -mcpu=generic.
 static void checkARMCPUName(const Driver &D, const Arg *A,
                             const ArgList &Args,
                             llvm::StringRef CPUName,

   bool KernelOrKext =
       Args.hasArg(options::OPT_mkernel, options::OPT_fapple_kext);
   StringRef FloatABI = tools::arm::getARMFloatABI(D, Args, Triple);
+  const Arg *WaCPU = nullptr, *WaFPU = nullptr;
+  const Arg *WaHdiv = nullptr, *WaArch = nullptr;
+
   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

     // Use software floating point argument passing?
     if (FloatABI != "hard")
       Features.push_back("+soft-float-abi");
+  } else {
+    // Here, we make sure that -Wa,-mfpu/cpu/arch/hwdiv will be passed down
+    // to the assembler correctly.
+    for (const Arg *A : Args.filtered(options::OPT_Wa_COMMA,
+                                      options::OPT_Xassembler)) {
+      StringRef Value = A->getValue();
+      if (Value.startswith("-mfpu=")) {
+        WaFPU = A;
+      } else if (Value.startswith("-mcpu=")) {
+        WaCPU = A;
+      } else if (Value.startswith("-mhwdiv=")) {
+        WaHdiv = A;
+      } else if (Value.startswith("-march=")) {
+        WaArch = A;
+      }
+    }
   }
 
-  // Honor -mfpu=.
-  if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ))
+  // Honor -mfpu=. ClangAs gives preference to -Wa,-mfpu=.
+  if (WaFPU)
+    getARMFPUFeatures(D, WaFPU, Args, StringRef(WaFPU->getValue()).substr(6), Features);
+  else if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ))
     getARMFPUFeatures(D, A, Args, A->getValue(), Features);
-  // Honor -mhwdiv=
-  if (const Arg *A = Args.getLastArg(options::OPT_mhwdiv_EQ))
+  // Honor -mhwdiv=. ClangAs gives preference to -Wa,-mhwdiv=.
+  if (WaHdiv)
+    getARMHWDivFeatures(D, WaHdiv, Args, StringRef(WaHdiv->getValue()).substr(8), Features);
+  else if (const Arg *A = Args.getLastArg(options::OPT_mhwdiv_EQ))
     getARMHWDivFeatures(D, A, Args, A->getValue(), Features);
 
-  // Check if -march is valid by checking if it can be canonicalised and parsed.
-  // getARMArch is used here instead of just checking the -march value in order
-  // to handle -march=native correctly.
+  // Check -march. ClangAs gives preference to -Wa,-march=.
   StringRef ArchName;
-  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
+  if (WaArch) {
+    ArchName = StringRef(WaArch->getValue()).substr(7);
+    checkARMArchName(D, WaArch, Args, ArchName, Triple);
+    // FIXME: Set Arch.
+  } else if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
     ArchName = A->getValue();
     checkARMArchName(D, A, Args, ArchName, Triple);
   }
 
-  // We do a similar thing with -mcpu, but here things are complicated because
-  // the only function we have to check if a cpu is valid is
-  // getLLVMArchSuffixForARM which also needs an architecture.
+  // Check -mcpu. ClangAs gives preference to -Wa,-mcpu=.
   StringRef CPUName;
-  if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ)) {
+  if (WaCPU) {
+    CPUName = StringRef(WaCPU->getValue()).substr(6);
+    checkARMCPUName(D, WaCPU, Args, CPUName, ArchName, Triple);
+    // FIXME: Set CPU.
+  } else if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ)) {
     CPUName = A->getValue();
     checkARMCPUName(D, A, Args, CPUName, ArchName, Triple);
   }

   }
 }
 
-static std::string getCPUName(const ArgList &Args, const llvm::Triple &T) {
+static std::string getCPUName(const ArgList &Args,
+                              const llvm::Triple &T,
+                              bool FromAs=false) {
   switch (T.getArch()) {
   default:
     return "";

   case llvm::Triple::thumb:
   case llvm::Triple::thumbeb: {
     StringRef MArch, MCPU;
-    getARMArchCPUFromArgs(Args, MArch, MCPU);
+    getARMArchCPUFromArgs(Args, MArch, MCPU, FromAs);
     return arm::getARMTargetCPU(MCPU, MArch, T);
   }
   case llvm::Triple::mips:

           TakeNextArg = true;
       } else if (Value.startswith("-gdwarf-")) {
         CmdArgs.push_back(Value.data());
+      } else if (Value.startswith("-mcpu") ||
+                 Value.startswith("-mfpu") ||
+                 Value.startswith("-mhwdiv") ||
+                 Value.startswith("-march")) {
+        // Do nothing, we'll validate it later.
       } else {
         D.Diag(diag::err_drv_unsupported_option_argument)
             << A->getOption().getName() << Value;

   }
 
   // Add the target cpu
-  std::string CPU = getCPUName(Args, Triple);
+  std::string CPU = getCPUName(Args, Triple, /*FromAs*/ false);
   if (!CPU.empty()) {
     CmdArgs.push_back("-target-cpu");
     CmdArgs.push_back(Args.MakeArgString(CPU));

 
   // Add the target cpu
   const llvm::Triple Triple(TripleStr);
-  std::string CPU = getCPUName(Args, Triple);
+  std::string CPU = getCPUName(Args, Triple, /*FromAs*/ true);
   if (!CPU.empty()) {
     CmdArgs.push_back("-target-cpu");
     CmdArgs.push_back(Args.MakeArgString(CPU));

   case llvm::Triple::thumb:
   case llvm::Triple::thumbeb: {
     StringRef MArch, MCPU;
-    getARMArchCPUFromArgs(Args, MArch, MCPU);
+    getARMArchCPUFromArgs(Args, MArch, MCPU, /*FromAs*/ true);
     std::string Arch = arm::getARMTargetCPU(MCPU, MArch, getToolChain().getTriple());
     CmdArgs.push_back(Args.MakeArgString("-mcpu=" + Arch));
     break;

test/Driver/arm-ias-Wa.s

+// Test that different values of -Wa,-mcpu/mfpu/march/mhwdiv pick correct ARM target-feature(s).
+// Complete tests about -mcpu/mfpu/march/mhwdiv on other files.
+
+// CHECK: "cc1as"
+// ================================================================= CPU
+// RUN: %clang -target arm-linux-gnueabi -Wa,-mcpu=cortex-a15 -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-CPU %s
+// CHECK-CPU: "-target-cpu" "cortex-a15"
+
+// RUN: %clang -target arm -Wa,-mcpu=bogus -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-BOGUS-CPU %s
+// CHECK-BOGUS-CPU: error: {{.*}} does not support '-Wa,-mcpu=bogus'
+
+// ================================================================= FPU
+// RUN: %clang -target arm-linux-eabi -Wa,-mfpu=neon %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NEON %s
+// CHECK-NEON: "-target-feature" "+neon"
+
+// RUN: %clang -target arm-linux-eabi -Wa,-mfpu=bogus %s -### -o %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-BOGUS-FPU %s
+// CHECK-BOGUS-FPU: error: {{.*}} does not support '-Wa,-mfpu=bogus'
+
+// ================================================================= Arch
+// Arch validation only for now, in case we're passing to an external asm
+
+// RUN: %clang -target arm -Wa,-march=armbogusv6 -### -c %s 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-BOGUS %s
+// CHECK-BOGUS: error: {{.*}} does not support '-Wa,-march=armbogusv6'
+
+// ================================================================= HD Div
+// RUN: %clang -### -target arm %s -Wa,-mhwdiv=arm -o %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM %s
+// CHECK-ARM: "-target-feature" "+hwdiv-arm"
+// CHECK-ARM: "-target-feature" "-hwdiv"
+
+// RUN: %clang -### -target arm %s -Wa,-mhwdiv=thumb -o %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-THUMB %s
+// CHECK-THUMB: "-target-feature" "-hwdiv-arm"
+// CHECK-THUMB: "-target-feature" "+hwdiv"
+
+// RUN: %clang -### -target arm %s -Wa,-mhwdiv=bogus -o %t.o 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-BOGUS-HDIV %s
+// CHECK-BOGUS-HDIV: error: {{.*}} does not support '-Wa,-mhwdiv=bogus'
+