[RISCV] Always emit relocations for resolved symbols and relax #73793
Conversation
If relaxation is not itended, it can be disabled in the linker. Also, we cannot trust Subtarget features here, because it may be empty in case of LTO codegen, preventing relaxations. Also forward --no-relax option to linker.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-backend-risc-v Author: Andreu Carminati (andcarminati) ChangesIf relaxation is not itended, it can be disabled in the linker. Also, we cannot trust Subtarget features here, because it may be empty in case of LTO codegen, preventing relaxations. Also forward --no-relax option to linker. Full diff: https://github.com/llvm/llvm-project/pull/73793.diff 5 Files Affected:
diff --git a/clang/lib/Driver/ToolChains/BareMetal.cpp b/clang/lib/Driver/ToolChains/BareMetal.cpp
index 42c8336e626c7b5..fc955d79780e5a0 100644
--- a/clang/lib/Driver/ToolChains/BareMetal.cpp
+++ b/clang/lib/Driver/ToolChains/BareMetal.cpp
@@ -443,6 +443,9 @@ void baremetal::Linker::ConstructJob(Compilation &C, const JobAction &JA,
CmdArgs.push_back("-Bstatic");
+ if (Args.hasArg(options::OPT_mno_relax))
+ CmdArgs.push_back("--no-relax");
+
if (Triple.isARM() || Triple.isThumb()) {
bool IsBigEndian = arm::isARMBigEndian(Triple, Args);
if (IsBigEndian)
diff --git a/clang/lib/Driver/ToolChains/RISCVToolchain.cpp b/clang/lib/Driver/ToolChains/RISCVToolchain.cpp
index 7e6abd144428783..0be7d1a88994957 100644
--- a/clang/lib/Driver/ToolChains/RISCVToolchain.cpp
+++ b/clang/lib/Driver/ToolChains/RISCVToolchain.cpp
@@ -156,6 +156,9 @@ void RISCV::Linker::ConstructJob(Compilation &C, const JobAction &JA,
if (!D.SysRoot.empty())
CmdArgs.push_back(Args.MakeArgString("--sysroot=" + D.SysRoot));
+ if (Args.hasArg(options::OPT_mno_relax))
+ CmdArgs.push_back("--no-relax");
+
bool IsRV64 = ToolChain.getArch() == llvm::Triple::riscv64;
CmdArgs.push_back("-m");
if (IsRV64) {
diff --git a/clang/test/Driver/baremetal.cpp b/clang/test/Driver/baremetal.cpp
index c04f4506a0994db..134bf427e3dc160 100644
--- a/clang/test/Driver/baremetal.cpp
+++ b/clang/test/Driver/baremetal.cpp
@@ -460,3 +460,13 @@
// RUN: | FileCheck --check-prefix=CHECK-CLANGRT-ARCH %s
// CHECK-CLANGRT-ARCH: "-lclang_rt.builtins-armv6m"
// CHECK-CLANGRT-ARCH-NOT: "-lclang_rt.builtins"
+
+// RUN: %clang %s -### 2>&1 --target=riscv64-unknown-elf -nostdinc -mno-relax \
+// RUN: --sysroot=%S/Inputs/basic_riscv64_tree/riscv64-unknown-elf \
+// RUN: | FileCheck --check-prefix=CHECK-RV64-NORELAX %s
+// CHECK-RV64-NORELAX: "--no-relax"
+
+// RUN: %clang %s -### 2>&1 --target=riscv64-unknown-elf -nostdinc \
+// RUN: --sysroot=%S/Inputs/basic_riscv64_tree/riscv64-unknown-elf \
+// RUN: | FileCheck --check-prefix=CHECK-RV64-RELAX %s
+// CHECK-RV64-RELAX-NOT: "--no-relax"
\ No newline at end of file
diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
index dfc3c9e9908d888..d4efaaf2666e426 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp
@@ -103,9 +103,9 @@ RISCVAsmBackend::getFixupKindInfo(MCFixupKind Kind) const {
return Infos[Kind - FirstTargetFixupKind];
}
-// If linker relaxation is enabled, or the relax option had previously been
-// enabled, always emit relocations even if the fixup can be resolved. This is
-// necessary for correctness as offsets may change during relaxation.
+// Always emit relocations for relative addresses, even if the fixup can be
+// resolved. This is necessary for correctness as offsets may change during
+// relaxation.
bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
const MCFixup &Fixup,
const MCValue &Target) {
@@ -122,13 +122,9 @@ bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
if (Target.isAbsolute())
return false;
break;
- case RISCV::fixup_riscv_got_hi20:
- case RISCV::fixup_riscv_tls_got_hi20:
- case RISCV::fixup_riscv_tls_gd_hi20:
- return true;
}
- return STI.hasFeature(RISCV::FeatureRelax) || ForceRelocs;
+ return true;
}
bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(const MCFixup &Fixup,
diff --git a/llvm/test/CodeGen/RISCV/compress.ll b/llvm/test/CodeGen/RISCV/compress.ll
index 479b7e524cd347c..fd7c4e9cc9934e9 100644
--- a/llvm/test/CodeGen/RISCV/compress.ll
+++ b/llvm/test/CodeGen/RISCV/compress.ll
@@ -1,3 +1,4 @@
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py UTC_ARGS: --version 3
; This test is designed to run twice, once with function attributes and once
; with target attributes added on the command line.
;
@@ -50,35 +51,45 @@ define i32 @simple_arith(i32 %a, i32 %b) #0 {
define i32 @select(i32 %a, ptr %b) #0 {
; RV32IC-LABEL: <select>:
; RV32IC: c.lw a2, 0(a1)
-; RV32IC-NEXT: c.beqz a2, 0x18
+; RV32IC-NEXT: c.beqz a2, 0x14 <select+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_2>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: c.bnez a2, 0x1e
+; RV32IC-NEXT: c.bnez a2, 0x1a <.LBB1_2+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_4>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: bltu a2, a0, 0x26
+; RV32IC-NEXT: bltu a2, a0, 0x20 <.LBB1_4+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_6>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: bgeu a0, a2, 0x2e
+; RV32IC-NEXT: bgeu a0, a2, 0x28 <.LBB1_6+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_8>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: bltu a0, a2, 0x36
+; RV32IC-NEXT: bltu a0, a2, 0x30 <.LBB1_8+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_10>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: bgeu a2, a0, 0x3e
+; RV32IC-NEXT: bgeu a2, a0, 0x38 <.LBB1_10+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_12>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: blt a2, a0, 0x46
+; RV32IC-NEXT: blt a2, a0, 0x40 <.LBB1_12+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_14>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: bge a0, a2, 0x4e
+; RV32IC-NEXT: bge a0, a2, 0x48 <.LBB1_14+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_16>:
; RV32IC-NEXT: c.lw a2, 0(a1)
-; RV32IC-NEXT: blt a0, a2, 0x56
+; RV32IC-NEXT: blt a0, a2, 0x50 <.LBB1_16+0x2>
; RV32IC-NEXT: c.mv a0, a2
+; RV32IC-LABEL: <.LBB1_18>:
; RV32IC-NEXT: c.lw a1, 0(a1)
-; RV32IC-NEXT: bge a1, a0, 0x5e
+; RV32IC-NEXT: bge a1, a0, 0x58 <.LBB1_18+0x2>
; RV32IC-NEXT: c.mv a0, a1
+; RV32IC-LABEL: <.LBB1_20>:
; RV32IC-NEXT: c.jr ra
%val1 = load volatile i32, ptr %b
%tst1 = icmp eq i32 0, %val1
|
|
Hi @topperc , just another idea to solve the problem, mostly to discuss! Regards. |
andcarminati
changed the title
|
As far as I can tell this is pointless. If you want relaxation you need R_RISCV_RELAX and R_RISC_ALIGN relocations to be emitted. If you don't want relaxation you don't need these. Therefore it seems like all this does is emit a whole bunch of useless relocations for the case when you're not enabling relaxation at compile time and thus cannot possibly enable it at link time? |
And that's the problem. It's vital that we have the information. Anything else is just a hack that papers over the fundamental issue. |
Hi @jrtc27, thank you for your comment, understood your point. For relaxation, I think we need also the branch relocation/anything relative, as we are removing some lui instructions. My original idea was based on the case that relaxation is a default ON feature for RISCV, but I honestly don't know the use cases to disable it. Just to follow the discussion, I can consider the following use case: clang [...] -c -o myobject.o (just compile) In this case, myobject.elf will be relaxed, the -mno-relax will be silently ignored. GCC handles this case as expected. Maybe we have two different things to handle. Regards. |
|
For the driver BareMetal.cpp change, claiming |
In the case of -mno-relax option. Otherwise, we cannot prevent relaxation if we split compilation and linking.
Thank you for the update. I updated the PR reverting the backend part and addressing this issue just for RISC-V. |
|
The current patch doesn't do what the title implies ("Always emit relocations for resolved "). |
Sure, I will close this PR because I created another one targeting just this case: #76432 Regards. |
If relaxation is not itended, it can be disabled in the linker. Also, we cannot trust Subtarget features here, because it may be empty in case of LTO codegen, preventing relaxations.
Also forward --no-relax option to linker.