[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

   llvm::StringRef Arch = Triple.getArchName();
   unsigned Profile = llvm::ARMTargetParser::parseArchProfile(Arch);
   return Profile == llvm::ARM::PK_M;
 }
 
 // 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,
                                 std::vector<const char *> &Features) {
   if (HWDiv == "arm") {
     Features.push_back("+hwdiv-arm");
     Features.push_back("-hwdiv");
   } else if (HWDiv == "thumb") {

                               const ArgList &Args,
                               StringRef FPU,
                               std::vector<const char *> &Features) {
   unsigned FPUID = llvm::ARMTargetParser::parseFPU(FPU);
   if (!llvm::ARMTargetParser::getFPUFeatures(FPUID, Features))
     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,
                              const llvm::Triple &Triple) {
   std::string MArch = arm::getARMArch(ArchName, Triple);
   if (llvm::ARMTargetParser::parseArch(MArch) == llvm::ARM::AK_INVALID)
     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,
                             llvm::StringRef ArchName,
                             const llvm::Triple &Triple) {
   std::string CPU = arm::getARMTargetCPU(CPUName, ArchName, Triple);
   std::string Arch = arm::getARMArch(ArchName, Triple);
   if (strcmp(arm::getLLVMArchSuffixForARM(CPU, Arch), "") == 0)

 
 static void getARMTargetFeatures(const Driver &D, const llvm::Triple &Triple,
                                  const ArgList &Args,
                                  std::vector<const char *> &Features,
                                  bool ForAS) {
   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
     // 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
     // currently do that for the assembler, we pass the options directly to the
     // backend and never even instantiate the frontend TargetInfo. If we did,
     // and used its handleTargetFeatures hook, then we could ensure the
     // assembler and the frontend behave the same.
 
     // Use software floating point operations?
     if (FloatABI == "soft")
       Features.push_back("+soft-float");
 
     // 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))
-    getARMFPUFeatures(D, A, Args, A->getValue(), Features);
-  // Honor -mhwdiv=
-  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.
-  StringRef ArchName;
-  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
-    ArchName = A->getValue();
-    checkARMArchName(D, A, Args, ArchName, Triple);
+  // Honor -mfpu=. ClangAs gives preference to -Wa,-mfpu=.
+  const Arg *FPUArg = Args.getLastArg(options::OPT_mfpu_EQ);
+  if (WaFPU) {
+    if (FPUArg)
+      D.Diag(clang::diag::warn_drv_unused_argument) << FPUArg->getAsString(Args);
+    getARMFPUFeatures(D, WaFPU, Args, StringRef(WaFPU->getValue()).substr(6), Features);
+  } else if (FPUArg) {
+    getARMFPUFeatures(D, FPUArg, Args, FPUArg->getValue(), Features);
   }
 
-  // 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.
+  // Honor -mhwdiv=. ClangAs gives preference to -Wa,-mhwdiv=.
+  const Arg *HDivArg = Args.getLastArg(options::OPT_mhwdiv_EQ);
+  if (WaHdiv) {
+    if (HDivArg)
+      D.Diag(clang::diag::warn_drv_unused_argument) << HDivArg->getAsString(Args);
+    getARMHWDivFeatures(D, WaHdiv, Args, StringRef(WaHdiv->getValue()).substr(8), Features);
+  } else if (HDivArg)
+    getARMHWDivFeatures(D, HDivArg, Args, HDivArg->getValue(), Features);
+
+  // Check -march. ClangAs gives preference to -Wa,-march=.
+  const Arg *ArchArg = Args.getLastArg(options::OPT_march_EQ);
+  StringRef ArchName;
+  if (WaArch) {
+    if (ArchArg)
+      D.Diag(clang::diag::warn_drv_unused_argument) << ArchArg->getAsString(Args);
+    ArchName = StringRef(WaArch->getValue()).substr(7);
+    checkARMArchName(D, WaArch, Args, ArchName, Triple);
+    // FIXME: Set Arch.
+    D.Diag(clang::diag::warn_drv_unused_argument) << WaArch->getAsString(Args);
+  } else if (ArchArg) {
+    ArchName = ArchArg->getValue();
+    checkARMArchName(D, ArchArg, Args, ArchName, Triple);
+  }
+
+  // Check -mcpu. ClangAs gives preference to -Wa,-mcpu=.
+  const Arg *CPUArg = Args.getLastArg(options::OPT_mcpu_EQ);
   StringRef CPUName;
-  if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ)) {
-    CPUName = A->getValue();
-    checkARMCPUName(D, A, Args, CPUName, ArchName, Triple);
+  if (WaCPU) {
+    if (CPUArg)
+      D.Diag(clang::diag::warn_drv_unused_argument) << CPUArg->getAsString(Args);
+    CPUName = StringRef(WaCPU->getValue()).substr(6);
+    checkARMCPUName(D, WaCPU, Args, CPUName, ArchName, Triple);
+  } else if (CPUArg) {
+    CPUName = CPUArg->getValue();
+    checkARMCPUName(D, CPUArg, Args, CPUName, ArchName, Triple);
   }
 
   // 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");

   case llvm::Triple::Bitrig:
     return "i686";
   default:
     // Fallback to p4.
     return "pentium4";
   }
 }
 
-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::aarch64:
   case llvm::Triple::aarch64_be:
     return getAArch64TargetCPU(Args);
 
   case llvm::Triple::arm:
   case llvm::Triple::armeb:
   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:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:
   case llvm::Triple::mips64el: {
     StringRef CPUName;
     StringRef ABIName;

       } else if (Value.startswith("-I")) {
         CmdArgs.push_back(Value.data());
         // We need to consume the next argument if the current arg is a plain
         // -I. The next arg will be the include directory.
         if (Value == "-I")
           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;
       }
     }
   }
   if (CompressDebugSections) {
     if (llvm::zlib::isAvailable())

   (void)Args.hasArg(options::OPT_mtune_EQ);
 
   if (Arg *A = Args.getLastArg(options::OPT_mcmodel_EQ)) {
     CmdArgs.push_back("-mcode-model");
     CmdArgs.push_back(A->getValue());
   }
 
   // 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));
   }
 
   if (const Arg *A = Args.getLastArg(options::OPT_mfpmath_EQ)) {
     CmdArgs.push_back("-mfpmath");
     CmdArgs.push_back(A->getValue());

 
   // Set the main file name, so that debug info works even with
   // -save-temps or preprocessed assembly.
   CmdArgs.push_back("-main-file-name");
   CmdArgs.push_back(Clang::getBaseInputName(Args, Input));
 
   // 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));
   }
 
   // Add the target features
   const Driver &D = getToolChain().getDriver();
   getTargetFeatures(D, Triple, Args, CmdArgs, true);

   case llvm::Triple::x86:
     CmdArgs.push_back("--32");
     break;
   case llvm::Triple::arm:
   case llvm::Triple::armeb:
   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;
   }
 
   case llvm::Triple::mips:
   case llvm::Triple::mipsel:
   case llvm::Triple::mips64:

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-DUP-CPU: warning: argument unused during compilation: '-mcpu=cortex-a8'
+// CHECK-DUP-FPU: warning: argument unused during compilation: '-mfpu=vfpv3'
+// CHECK-DUP-ARCH: warning: argument unused during compilation: '-march=armv7'
+// CHECK-DUP-HDIV: warning: argument unused during compilation: '-mhwdiv=arm'
+
+// 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'
+
+// RUN: %clang -target arm -mcpu=cortex-a8 -Wa,-mcpu=cortex-a15 -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-DUP-CPU %s
+// CHECK-DUP-CPU: "-target-cpu" "cortex-a15"
+
+// ================================================================= FPU
+// RUN: %clang -target arm-linux-eabi -Wa,-mfpu=neon -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-NEON %s
+// CHECK-NEON: "-target-feature" "+neon"
+
+// RUN: %clang -target arm-linux-eabi -Wa,-mfpu=bogus -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-BOGUS-FPU %s
+// CHECK-BOGUS-FPU: error: {{.*}} does not support '-Wa,-mfpu=bogus'
+
+// RUN: %clang -target arm-linux-eabi -mfpu=vfpv3 -Wa,-mfpu=neon -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-DUP-FPU %s
+// CHECK-DUP-FPU: "-target-feature" "+neon"
+
+// ================================================================= 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-ARCH %s
+// CHECK-BOGUS-ARCH: error: {{.*}} does not support '-Wa,-march=armbogusv6'
+
+// RUN: %clang -target arm -march=armv7 -Wa,-march=armv6 -c %s -### 2>&1 \
+// RUN:   | FileCheck -check-prefix=CHECK-DUP-ARCH %s
+
+// ================================================================= HD Div
+// RUN: %clang -target arm -Wa,-mhwdiv=arm -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-ARM %s
+// CHECK-ARM: "-target-feature" "+hwdiv-arm"
+// CHECK-ARM: "-target-feature" "-hwdiv"
+
+// RUN: %clang -target arm -Wa,-mhwdiv=thumb -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-THUMB %s
+// CHECK-THUMB: "-target-feature" "-hwdiv-arm"
+// CHECK-THUMB: "-target-feature" "+hwdiv"
+
+// RUN: %clang -target arm -Wa,-mhwdiv=bogus -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-BOGUS-HDIV %s
+// CHECK-BOGUS-HDIV: error: {{.*}} does not support '-Wa,-mhwdiv=bogus'
+
+// RUN: %clang -target arm -mhwdiv=arm -Wa,-mhwdiv=thumb -c %s -### 2>&1 \
+// RUN:   | FileCheck --check-prefix=CHECK-DUP-HDIV %s
+// CHECK-DUP-HDIV: "-target-feature" "-hwdiv-arm"
+// CHECK-DUP-HDIV: "-target-feature" "+hwdiv"