[Clang][LoongArch] Consume and check -mabi and -mfpu even if -m*-float is present

[xen0n]

Summary

This kind of CLI flags duplication can sometimes be convenient for build
systems that may have to tinker with these.

For example, in the Linux kernel we almost always want to ensure no FP
instruction is emitted, so -msoft-float is present by default; but
sometimes we do want to allow FPU usage (e.g. certain parts of amdgpu DC
code), in which case we want the -msoft-float stripped and -mfpu=64
added. Here we face a dilemma without this change:

• Either -mabi is not supplied by arch/loongarch Makefile, in which case the correct ABI has to be supplied by the driver Makefile (otherwise the ABI will become double-float due to -mfpu), which is arguably not appropriate for a driver;
• Or -mabi is still supplied by arch/loongarch Makefile, and the build immediately errors out because -Werror=unused-command-line-argument is unconditionally set for Clang builds.

To solve this, simply make sure to check -mabi and -mfpu (and gain
some useful diagnostics in case of conflicting settings) when
-m*-float is successfully parsed.

[xen0n]

Fix faulty refactor (unintended always-error case).

[SixWeining]

LGTM. Thanks.

clang/lib/Driver/ToolChains/Arch/LoongArch.cpp

72

Seems that this condition is always true.

91–97

Without default, there may be compiling warning?

[xen0n]

clang/lib/Driver/ToolChains/Arch/LoongArch.cpp

72

Yes you're right. I'll simplify it (and add comment detailing why it's safe to assume non-emptiness) shortly.

91–97

I didn't remember seeing such warnings; plus the case's body would be empty anyway...

[xen0n]

Refactored to reduce duplication of flag processing logic and save one level of indent.

[xen0n]

Grammatical fix of comment text ("is" -> "are").

[MaskRay]

clang/include/clang/Basic/DiagnosticDriverKinds.td

732

This is not called setting. Use value

It seems that ignoring -mabi value '%0' as it conflicts ... is more conventional.

clang/test/Driver/loongarch-msingle-float.c

10–12

Just use NOWARN-NOT: warning: to assert that there is no diagnostic

[xen0n]

Address @MaskRay's review comments.

[xen0n]

clang/include/clang/Basic/DiagnosticDriverKinds.td

732

Sounds reasonable. I've checked other similar diagnostics text and it seems your suggestion is stylistically more consistent.

[MaskRay]

clang/include/clang/Basic/DiagnosticDriverKinds.td

732

You can make -mabi/-mfpu an argument as well so that we can create just one def.
It would be good to expand -m*-float.

[xen0n]

Unify the diagnostic messages and show the actual -m*-float option being passed.

[xen0n]

Refactor more to be able to show the exact -mabi and -mfpu argument given.

clang/include/clang/Basic/DiagnosticDriverKinds.td

 
 def warn_drv_sarif_format_unstable : Warning<
   "diagnostic formatting in SARIF mode is currently unstable">,
   InGroup<DiagGroup<"sarif-format-unstable">>;
 
 def err_drv_riscv_unsupported_with_linker_relaxation : Error<
   "%0 is unsupported with RISC-V linker relaxation (-mrelax)">;
 
+def warn_drv_loongarch_conflicting_implied_val : Warning<
+  "ignoring '%0' as it conflicts with that implied by '%1' (%2)">,
+  InGroup<OptionIgnored>;
 def err_drv_loongarch_invalid_mfpu_EQ : Error<
   "invalid argument '%0' to -mfpu=; must be one of: 64, 32, none, 0 (alias for none)">;
 
 def err_drv_expand_response_file : Error<
   "failed to expand response file: %0">;
 
 def err_drv_no_matching_multilib : Error<
   "no multilib found matching flags: %0">;
 def note_drv_available_multilibs : Note<
   "available multilibs are:%0">;
 }

clang/lib/Driver/ToolChains/Arch/LoongArch.cpp

 
 StringRef loongarch::getLoongArchABI(const Driver &D, const ArgList &Args,
                                      const llvm::Triple &Triple) {
   assert((Triple.getArch() == llvm::Triple::loongarch32 ||
           Triple.getArch() == llvm::Triple::loongarch64) &&
          "Unexpected triple");
   bool IsLA32 = Triple.getArch() == llvm::Triple::loongarch32;
 
+  // Record -mabi value for later use.
+  const Arg *MABIArg = Args.getLastArg(options::OPT_mabi_EQ);
+  StringRef MABIValue;
+  if (MABIArg) {
+    MABIValue = MABIArg->getValue();
+  }
+
+  // Parse -mfpu value for later use.
+  const Arg *MFPUArg = Args.getLastArg(options::OPT_mfpu_EQ);
+  int FPU = -1;
+  if (MFPUArg) {
+    StringRef V = MFPUArg->getValue();
+    if (V == "64")
+      FPU = 64;
+    else if (V == "32")
+      FPU = 32;
+    else if (V == "0" || V == "none")
+      FPU = 0;
+    else
+      D.Diag(diag::err_drv_loongarch_invalid_mfpu_EQ) << V;
+  }
+
   // Check -m*-float firstly since they have highest priority.
   if (const Arg *A = Args.getLastArg(options::OPT_mdouble_float,
                                      options::OPT_msingle_float,
                                      options::OPT_msoft_float)) {
-    if (A->getOption().matches(options::OPT_mdouble_float))
-      return IsLA32 ? "ilp32d" : "lp64d";
-    if (A->getOption().matches(options::OPT_msingle_float))
-      return IsLA32 ? "ilp32f" : "lp64f";
-    if (A->getOption().matches(options::OPT_msoft_float))
-      return IsLA32 ? "ilp32s" : "lp64s";
+    StringRef ImpliedABI;
+    int ImpliedFPU = -1;
+    if (A->getOption().matches(options::OPT_mdouble_float)) {
+      ImpliedABI = IsLA32 ? "ilp32d" : "lp64d";
+      ImpliedFPU = 64;
+    }
+    if (A->getOption().matches(options::OPT_msingle_float)) {
+      ImpliedABI = IsLA32 ? "ilp32f" : "lp64f";
+      ImpliedFPU = 32;
+    }
+    if (A->getOption().matches(options::OPT_msoft_float)) {
+      ImpliedABI = IsLA32 ? "ilp32s" : "lp64s";
+      ImpliedFPU = 0;
+    }
+
+    // Check `-mabi=` and `-mfpu=` settings and report if they conflict with
+    // the higher-priority settings implied by -m*-float.
+    //
+    // ImpliedABI and ImpliedFPU are guaranteed to have valid values because
+    // one of the match arms must match if execution can arrive here at all.
+    if (!MABIValue.empty() && ImpliedABI != MABIValue)
+      D.Diag(diag::warn_drv_loongarch_conflicting_implied_val)
+          << MABIArg->getAsString(Args) << A->getAsString(Args) << ImpliedABI;
+
+    if (FPU != -1 && ImpliedFPU != FPU)
+      D.Diag(diag::warn_drv_loongarch_conflicting_implied_val)
+          << MFPUArg->getAsString(Args) << A->getAsString(Args) << ImpliedFPU;
+
+    return ImpliedABI;
   }
 
   // If `-mabi=` is specified, use it.
-  if (const Arg *A = Args.getLastArg(options::OPT_mabi_EQ))
-    return A->getValue();
+  if (!MABIValue.empty())
+    return MABIValue;
 
   // Select abi based on -mfpu=xx.
-  if (const Arg *A = Args.getLastArg(options::OPT_mfpu_EQ)) {
-    StringRef FPU = A->getValue();
-    if (FPU == "64")
-      return IsLA32 ? "ilp32d" : "lp64d";
-    if (FPU == "32")
-      return IsLA32 ? "ilp32f" : "lp64f";
-    if (FPU == "0" || FPU == "none")
-      return IsLA32 ? "ilp32s" : "lp64s";
-    D.Diag(diag::err_drv_loongarch_invalid_mfpu_EQ) << FPU;
+  switch (FPU) {
+  case 64:
+  case 32:
+  case 0:
   }
 
   // Choose a default based on the triple.
   // Honor the explicit ABI modifier suffix in triple's environment part if
   // present, falling back to {ILP32,LP64}D otherwise.
   switch (Triple.getEnvironment()) {
   case llvm::Triple::GNUSF:
     return IsLA32 ? "ilp32s" : "lp64s";

clang/test/Driver/loongarch-mdouble-float.c

 // RUN: %clang --target=loongarch64 -mdouble-float -fsyntax-only %s -### 2>&1 | \
 // RUN:   FileCheck %s --check-prefix=CC1
+// RUN: %clang --target=loongarch64 -mdouble-float -mfpu=64 -mabi=lp64d -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CC1,NOWARN
 // RUN: %clang --target=loongarch64 -mdouble-float -mfpu=0 -mabi=lp64s -fsyntax-only %s -### 2>&1 | \
-// RUN:   FileCheck %s --check-prefixes=CC1,WARN
+// RUN:   FileCheck %s --check-prefixes=CC1,WARN,WARN-FPU0
+// RUN: %clang --target=loongarch64 -mdouble-float -mfpu=none -mabi=lp64s -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CC1,WARN,WARN-FPUNONE
 // RUN: %clang --target=loongarch64 -mdouble-float -S -emit-llvm %s -o - | \
 // RUN:   FileCheck %s --check-prefix=IR
 
-// WARN: warning: argument unused during compilation: '-mfpu=0'
-// WARN: warning: argument unused during compilation: '-mabi=lp64s'
+// NOWARN-NOT: warning:
+// WARN: warning: ignoring '-mabi=lp64s' as it conflicts with that implied by '-mdouble-float' (lp64d)
+// WARN-FPU0: warning: ignoring '-mfpu=0' as it conflicts with that implied by '-mdouble-float' (64)
+// WARN-FPUNONE: warning: ignoring '-mfpu=none' as it conflicts with that implied by '-mdouble-float' (64)
 
 // CC1: "-target-feature" "+f"{{.*}} "-target-feature" "+d"
 // CC1: "-target-abi" "lp64d"
 
 // IR: attributes #[[#]] ={{.*}}"target-features"="{{(.*,)?}}+d,{{(.*,)?}}+f{{(,.*)?}}"
 
 int foo(void) {
   return 3;
 }

clang/test/Driver/loongarch-msingle-float.c

 // RUN: %clang --target=loongarch64 -msingle-float -fsyntax-only %s -### 2>&1 | \
 // RUN:   FileCheck %s --check-prefix=CC1
-// RUN: %clang --target=loongarch64 -msingle-float -mfpu=0 -mabi=lp64s -fsyntax-only %s -### 2>&1 | \
+// RUN: %clang --target=loongarch64 -msingle-float -mfpu=32 -mabi=lp64f -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CC1,NOWARN
+// RUN: %clang --target=loongarch64 -msingle-float -mfpu=64 -mabi=lp64s -fsyntax-only %s -### 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CC1,WARN
 // RUN: %clang --target=loongarch64 -msingle-float -S -emit-llvm %s -o - | \
 // RUN:   FileCheck %s --check-prefix=IR
 
-// WARN: warning: argument unused during compilation: '-mfpu=0'
-// WARN: warning: argument unused during compilation: '-mabi=lp64s'
+// NOWARN-NOT: warning:
+// WARN: warning: ignoring '-mabi=lp64s' as it conflicts with that implied by '-msingle-float' (lp64f)
+// WARN: warning: ignoring '-mfpu=64' as it conflicts with that implied by '-msingle-float' (32)
 
 // CC1: "-target-feature" "+f"{{.*}} "-target-feature" "-d"
 // CC1: "-target-abi" "lp64f"
 
 // IR: attributes #[[#]] ={{.*}}"target-features"="{{(.*,)?}}+f,{{(.*,)?}}-d"
 
 int foo(void) {
   return 3;
 }

clang/test/Driver/loongarch-msoft-float.c

 // RUN: %clang --target=loongarch64 -msoft-float -fsyntax-only %s -### 2>&1 | \
 // RUN:   FileCheck %s --check-prefix=CC1
+// RUN: %clang --target=loongarch64 -msoft-float -mfpu=0 -mabi=lp64s -fsyntax-only %s -### 2>&1 | \
+// RUN:   FileCheck %s --check-prefixes=CC1,NOWARN
 // RUN: %clang --target=loongarch64 -msoft-float -mfpu=64 -mabi=lp64d -fsyntax-only %s -### 2>&1 | \
 // RUN:   FileCheck %s --check-prefixes=CC1,WARN
 // RUN: %clang --target=loongarch64 -msoft-float -S -emit-llvm %s -o - | \
 // RUN:   FileCheck %s --check-prefix=IR
 
-// WARN: warning: argument unused during compilation: '-mfpu=64'
-// WARN: warning: argument unused during compilation: '-mabi=lp64d'
+// NOWARN-NOT: warning:
+// WARN: warning: ignoring '-mabi=lp64d' as it conflicts with that implied by '-msoft-float' (lp64s)
+// WARN: warning: ignoring '-mfpu=64' as it conflicts with that implied by '-msoft-float' (0)
 
 // CC1: "-target-feature" "-f"{{.*}} "-target-feature" "-d"
 // CC1: "-target-abi" "lp64s"
 
 // IR: attributes #[[#]] ={{.*}}"target-features"="{{(.*,)?}}-d,{{(.*,)?}}-f{{(,.*)?}}"
 
 int foo(void) {
   return 3;
 }