[RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V #88538
Conversation
-O0 implies -mrelax-all as an assembler compile time optimization. -mrelax-all allows the assembler to complete layout in 2 passes instead of doing iterative branch relaxation. Jump offsets larger than +/-1MiB require an indirect jump on RISC-V. This can't be done by the assembler, so we use a branch relaxation MIR pass and use register scavenging to find a free register. The conditional branch offsets for RISC-V are also somewhat small so we support MC layer branch relaxation to make life easier for assembly programmers. This may also cover up bugs in our function size estimation in MachineIR. Enabling -mrelax-all causes the MC layer relaxation to agressively relax branches. This increases code size and can create cases where we need an indirect jump, but we can't create one. This leads to linker failures. The easiest way to avoid this is to not default to -mrelax-all for -O0 and sacrifice the compile time optimization. That's what this patch does. Fixes llvm#87127
llvmbot
added
clang
|
@llvm/pr-subscribers-clang @llvm/pr-subscribers-clang-driver Author: Craig Topper (topperc) Changes-O0 implies -mrelax-all as an assembler compile time optimization. -mrelax-all allows the assembler to complete layout in 2 passes instead of doing iterative branch relaxation. Jump offsets larger than +/-1MiB require an indirect jump on RISC-V. This can't be done by the assembler, so we use a branch relaxation MIR pass and use register scavenging to find a free register. The conditional branch offsets for RISC-V are also somewhat small so we support MC layer branch relaxation to make life easier for assembly programmers. This may also cover up bugs in our function size estimation in MachineIR. Enabling -mrelax-all causes the MC layer relaxation to agressively relax branches. This increases code size and can create cases where we need an indirect jump, but we can't create one. This leads to linker failures. The easiest way to avoid this is to not default to -mrelax-all for -O0 and sacrifice the compile time optimization. That's what this patch does. Fixes #87127 Full diff: https://github.com/llvm/llvm-project/pull/88538.diff 2 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..856a88bdc3aad4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -844,6 +844,16 @@ static bool UseRelaxAll(Compilation &C, const ArgList &Args) {
if (Arg *A = Args.getLastArg(options::OPT_O_Group))
RelaxDefault = A->getOption().matches(options::OPT_O0);
+ // RISC-V requires an indirect jump for offsets larger than 1MiB. This cannot
+ // be done by assembler branch relaxation as it needs a free temporary
+ // register. Because of this, branch relaxation is handled by a MachineIR
+ // pass before the assembler. Forcing assembler branch relaxation for -O0
+ // makes the MachineIR branch relaxation inaccurate and it will miss cases
+ // where an indirect branch is necessary. To avoid this issue we are
+ // sacrificing the compile time improvement of using -mrelax-all for -O0.
+ if (C.getDefaultToolChain().getTriple().isRISCV())
+ RelaxDefault = false;
+
if (RelaxDefault) {
RelaxDefault = false;
for (const auto &Act : C.getActions()) {
diff --git a/clang/test/Driver/integrated-as.c b/clang/test/Driver/integrated-as.c
index d7658fdfd63374..5b79714a2c547e 100644
--- a/clang/test/Driver/integrated-as.c
+++ b/clang/test/Driver/integrated-as.c
@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}
// RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
|
There was a problem hiding this comment.
I left a comment at
#87127 (comment)
This is more about a weird -O0 -mrelax-all behavior that RISC-V now opts out, which is good on its own, even if the basis looks like a self-inflicted problem...
clang/test/Driver/integrated-as.c
Outdated
| @@ -1,4 +1,4 @@ | |||
| // XFAIL: target={{.*}}-aix{{.*}} | |||
| // XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}} | |||
There was a problem hiding this comment.
This causes different behaviors with the default target triple, which is probably not perfect.
I think we can change the -mrelax-all check with an explicit target triple and then add a --target=riscv64.
There was a problem hiding this comment.
What triple should i use for the -mrelax-all test?
There was a problem hiding this comment.
Perhaps the commonplace --target=x86_64
-O0 implies -mrelax-all as an assembler compile time optimization. -mrelax-all allows the assembler to complete layout in 2 passes instead of doing iterative branch relaxation.
Jump offsets larger than +/-1MiB require an indirect jump on RISC-V. This can't be done by the assembler, so we use a branch relaxation MIR pass and use register scavenging to find a free register.
The conditional branch offsets for RISC-V are also somewhat small so we support MC layer branch relaxation to make life easier for assembly programmers. This may also cover up bugs in our function size estimation in MachineIR.
Enabling -mrelax-all causes the MC layer relaxation to agressively relax branches. This increases code size and can create cases where we need an indirect jump, but we can't create one. This leads to linker failures.
The easiest way to avoid this is to not default to -mrelax-all for -O0 and sacrifice the compile time optimization. That's what this patch does.
Fixes #87127