Skip to content

[RISCV] Add -march=unset to cancel and ignore a previous -march. #148321

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Jul 12, 2025

-mcpu is used to determine the ISA string if an explicit -march is not present on the command line. If there is a -march present it always has priority over -mcpu regardless of where it appears in the command line.

This can cause issues if -march appears in a Makefile and a user wants to override it with an -mcpu on the command line. The user would need to provide a potentially long ISA string to -march that matches the -mcpu in order to override the MakeFile.

This issue can also be seen on Compiler Explorer where the rv64gc toolchain is passed -march=rv64gc before any user command line options are added. If you pass -mcpu=spacemit-x60, vectors will not be enabled due to the hidden -march.

This patch adds a new option for -march, "unset" that makes the -march ignored for purposes of prioritizing over -mcpu. Now a user can write -march=unset -mcpu=<cpu_name>. This is also implemented by gcc for ARM32.

An alternative would be to allow -march to take a cpu name, but that requires "-march=<cpu_name> -mcpu=<cpu_name>" or "-march=<cpu_name> -mtune=<cpu_name>" to ensure the tune cpu also gets updated. IMHO, needing to repeat the CPU name twice isn't friendly and invites mistakes.

More discussion here riscv-non-isa/riscv-toolchain-conventions#88 I won't merge this until we reach consensus there.

-mcpu is used to determine the ISA string if an explicit -march
is not present on the command line. If there is a -march present
it always has priority over -mcpu regardless of where it appears
in the command line.

This can cause issues if -march appears in a Makefile and a user
wants to override it with an -mcpu on the command line. The user
would need to provide a potentially long ISA string to -march that
matches the -mcpu in order to override the MakeFile.

This issue also shows up on Compiler Explorer where the rv64gc toolchain
is passed -march=rv64gc before any user command line options are
added.

This patch adds a new option for -march, "unset" that makes the
-march ignored for purposes of prioritizing over -mcpu. Now
a user can write -march=unset -mcpu=<cpu_name>. This is also
implemented by gcc for ARM32.

An alternative would be to allow -march to take a cpu name, but
that requires "-march=<cpu_name> -mcpu=<cpu_name>" or
"-march=<cpu_name> -mcpu=<cpu_name>" to ensure the tune cpu also
gets updated. IMHO, needing to repeat the CPU name twice isn't
friendly and invites mistakes.

More discussion here riscv-non-isa/riscv-toolchain-conventions#88
I won't merge this until we reach consensus there.
@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' labels Jul 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Jul 12, 2025

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-clang-driver

Author: Craig Topper (topperc)

Changes

-mcpu is used to determine the ISA string if an explicit -march is not present on the command line. If there is a -march present it always has priority over -mcpu regardless of where it appears in the command line.

This can cause issues if -march appears in a Makefile and a user wants to override it with an -mcpu on the command line. The user would need to provide a potentially long ISA string to -march that matches the -mcpu in order to override the MakeFile.

This issue can also be seen on Compiler Explorer where the rv64gc toolchain is passed -march=rv64gc before any user command line options are added. If you pass -mcpu=spacemit-x60, vectors will not be enabled due to the hidden -march.

This patch adds a new option for -march, "unset" that makes the -march ignored for purposes of prioritizing over -mcpu. Now a user can write -march=unset -mcpu=<cpu_name>. This is also implemented by gcc for ARM32.

An alternative would be to allow -march to take a cpu name, but that requires "-march=<cpu_name> -mcpu=<cpu_name>" or "-march=<cpu_name> -mcpu=<cpu_name>" to ensure the tune cpu also gets updated. IMHO, needing to repeat the CPU name twice isn't friendly and invites mistakes.

More discussion here riscv-non-isa/riscv-toolchain-conventions#88 I won't merge this until we reach consensus there.


Full diff: https://github.com/llvm/llvm-project/pull/148321.diff

2 Files Affected:

  • (modified) clang/lib/Driver/ToolChains/Arch/RISCV.cpp (+7-4)
  • (modified) clang/test/Driver/riscv-cpus.c (+5)
diff --git a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
index baa2c8c0bcfb2..76dde0da8e849 100644
--- a/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
+++ b/clang/lib/Driver/ToolChains/Arch/RISCV.cpp
@@ -273,9 +273,12 @@ std::string riscv::getRISCVArch(const llvm::opt::ArgList &Args,
   // Clang does not yet support MULTILIB_REUSE, so we use `rv{XLEN}imafdc`
   // instead of `rv{XLEN}gc` though they are (currently) equivalent.
 
-  // 1. If `-march=` is specified, use it.
-  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ))
-    return A->getValue();
+  // 1. If `-march=` is specified, use it unless the value is "unset".
+  if (const Arg *A = Args.getLastArg(options::OPT_march_EQ)) {
+    StringRef MArch = A->getValue();
+    if (MArch != "unset")
+      return MArch.str();
+  }
 
   // 2. Get march (isa string) based on `-mcpu=`
   if (const Arg *A = Args.getLastArg(options::OPT_mcpu_EQ)) {
@@ -300,7 +303,7 @@ std::string riscv::getRISCVArch(const llvm::opt::ArgList &Args,
 
     StringRef MArch = llvm::RISCV::getMArchFromMcpu(CPU);
     // Bypass if target cpu's default march is empty.
-    if (MArch != "")
+    if (!MArch.empty())
       return MArch.str();
   }
 
diff --git a/clang/test/Driver/riscv-cpus.c b/clang/test/Driver/riscv-cpus.c
index d32d1c1a8183f..661bacce5d22f 100644
--- a/clang/test/Driver/riscv-cpus.c
+++ b/clang/test/Driver/riscv-cpus.c
@@ -403,6 +403,11 @@
 // MCPU-MARCH: "-nostdsysteminc" "-target-cpu" "sifive-e31" "-target-feature" "+m" "-target-feature" "+c"
 // MCPU-MARCH: "-target-abi" "ilp32"
 
+// march=unset erases previous march
+// RUN: %clang --target=riscv32 -### -c %s 2>&1 -march=rv32imc -march=unset -mcpu=sifive-e31 | FileCheck -check-prefix=MARCH-UNSET %s
+// MARCH-UNSET: "-nostdsysteminc" "-target-cpu" "sifive-e31" "-target-feature" "+m" "-target-feature" "+a" "-target-feature" "+c"
+// MARCH-UNSET: "-target-abi" "ilp32"
+
 // Check interaction between mcpu and mtune, mtune won't affect arch related
 // target feature, but mcpu will.
 //

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for implementing -mcpu=unset. This looks good. I'm glad to see -mcpu=unset gaining support, especially after feeling like the lone voice in the GCC thread: https://gcc.gnu.org/pipermail/gcc-patches/2025-June/685537.html

// march=unset erases previous march
// RUN: %clang --target=riscv32 -### -c %s 2>&1 -march=rv32imc -march=unset -mcpu=sifive-e31 | FileCheck -check-prefix=MARCH-UNSET %s
// MARCH-UNSET: "-nostdsysteminc" "-target-cpu" "sifive-e31" "-target-feature" "+m" "-target-feature" "+a" "-target-feature" "+c"
// MARCH-UNSET: "-target-abi" "ilp32"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add -SAME if applicable.

@@ -403,6 +403,11 @@
// MCPU-MARCH: "-nostdsysteminc" "-target-cpu" "sifive-e31" "-target-feature" "+m" "-target-feature" "+c"
// MCPU-MARCH: "-target-abi" "ilp32"

// march=unset erases previous march
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/// -march=unset erases previous -march.

Always include - when referring to a - option.

Copy link
Member

@MaskRay MaskRay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! The reactions include a thumbs-up and a love. Setting my approval.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM and I totally support this!

Copy link
Member

@lenary lenary left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't recall running into this issue, but I'm happy with this solution over accepting cpu names in -march, it feels much clearer.

@topperc
Copy link
Collaborator Author

topperc commented Jul 14, 2025

I forgot about tomorrow's LLVM 21 branch so maybe I should merge this based on the support I've received so far?

@lenary
Copy link
Member

lenary commented Jul 15, 2025

@topperc (thanks autocorrect) what's the status of this in GCC? If it's in there I think we should land/backport this, otherwise after the branch is probably ok.

@MaskRay
Copy link
Member

MaskRay commented Jul 15, 2025

I forgot about tomorrow's LLVM 21 branch so maybe I should merge this based on the support I've received so far?

Looks good!

https://github.com/toppers what's the status of this in GCC? If it's in there I think we should land/backport this, otherwise after the branch is probably ok.

https://gcc.gnu.org/releases.html GCC has a X.1 release every year. There should be plenty of time. @kito-cheng

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants