[RISCV] Force relocations if initial MCSubtargetInfo contains FeatureRelax

[MaskRay]

Regarding

.option norelax
j label
.option relax
// relaxable instructions
// For assembly input, RISCVAsmParser::ParseInstruction will set ForceRelocs (https://reviews.llvm.org/D46423).
// For direct object emission, ForceRelocs is not set after https://github.com/llvm/llvm-project/pull/73721
label:

The J instruction needs a relocation to ensure the target is correct
after linker relaxation. This is related a limitation in the assembler:
RISCVAsmBackend::shouldForceRelocation decides upfront whether a
relocation is needed, instead of checking more information (whether
there are relaxable fragments in between).

Despite the limitation, j label produces a relocation in direct object
emission mode, but was broken by #73721 due to the shouldForceRelocation
limitation.

Add a workaround to RISCVTargetELFStreamer to emulate the previous
behavior.

Link: ClangBuiltLinux/linux#1965

[llvmbot]

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

Author: Fangrui Song (MaskRay)

Changes

Regarding

.option norelax
j label
.option relax
// relaxable instructions
label:

The J instruction needs a relocation to ensure the target is correct
after linker relaxation. This is related a limitation in the assembler:
RISCVAsmBackend::shouldForceRelocation decides upfront whether a
relocation is needed, instead of checking more information (whether
there are relaxable fragments in between).

Despite the limitation, j label produces a relocation in direct object
emission mode, but was broken by #73721 due to the shouldForceRelocation
limitation.

Add a workaround to RISCVTargetELFStreamer to emulate the previous
behavior.

Link: ClangBuiltLinux/linux#1965

---

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

2 Files Affected:

• (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp (+7)
• (added) llvm/test/CodeGen/RISCV/option-relax-relocation.ll (+31)

diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 9db5148208b3ec..961b8f0afe2254 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -37,6 +37,13 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
   auto &MAB = static_cast<RISCVAsmBackend &>(MCA.getBackend());
   setTargetABI(RISCVABI::computeTargetABI(STI.getTargetTriple(), Features,
                                           MAB.getTargetOptions().getABIName()));
+  // `j label` in `.option norelax; j label; .option relax; ...; label:` needs a
+  // relocation to ensure the jump target is correct after linking. This is due
+  // to a limitation that shouldForceRelocation has to make the decision upfront
+  // without knowing a possibly future .option relax. When RISCVAsmParser is used,
+  // its ParseInstruction may call setForceRelocs as well.
+  if (STI.hasFeature(RISCV::FeatureRelax))
+    static_cast<RISCVAsmBackend &>(MAB).setForceRelocs();
 }
 
 RISCVELFStreamer &RISCVTargetELFStreamer::getStreamer() {
diff --git a/llvm/test/CodeGen/RISCV/option-relax-relocation.ll b/llvm/test/CodeGen/RISCV/option-relax-relocation.ll
new file mode 100644
index 00000000000000..f9335f196e10ae
--- /dev/null
+++ b/llvm/test/CodeGen/RISCV/option-relax-relocation.ll
@@ -0,0 +1,31 @@
+;; With +relax, J below needs a relocation to ensure the target is correct
+;; after linker relaxation.
+
+; RUN: llc -mtriple=riscv64 -mattr=-relax -filetype=obj < %s \
+; RUN:     | llvm-objdump -d -r - | FileCheck %s
+; RUN: llc -mtriple=riscv64 -mattr=+relax -filetype=obj < %s \
+; RUN:     | llvm-objdump -d -r - | FileCheck %s --check-prefixes=CHECK,RELAX
+
+; CHECK:        j       {{.*}}
+; RELAX-NEXT:           R_RISCV_JAL  {{.*}}
+; CHECK-NEXT:   auipc   ra, 0x0
+; CHECK-NEXT:           R_RISCV_CALL_PLT     f
+; RELAX-NEXT:           R_RISCV_RELAX        *ABS*
+; CHECK-NEXT:   jalr    ra
+
+define dso_local noundef signext i32 @main() local_unnamed_addr #0 {
+entry:
+  callbr void asm sideeffect ".option push\0A.option norvc\0A.option norelax\0Aj $0\0A.option pop\0A", "!i"() #2
+          to label %asm.fallthrough [label %label]
+
+asm.fallthrough:                                  ; preds = %entry
+  tail call void @f()
+  br label %label
+
+label:                                            ; preds = %asm.fallthrough, %entry
+  ret i32 0
+}
+
+declare void @f()
+
+attributes #0 = { nounwind "target-features"="-c,+relax" }

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff daecc303bb719ed63566fcb343afec169826f82c c9726b0eadd8a9d7a23ad67bc2dabac80f152883 -- llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp

View the diff from clang-format here.

diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
index 961b8f0afe..880f469f03 100644
--- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
+++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVELFStreamer.cpp
@@ -40,8 +40,8 @@ RISCVTargetELFStreamer::RISCVTargetELFStreamer(MCStreamer &S,
   // `j label` in `.option norelax; j label; .option relax; ...; label:` needs a
   // relocation to ensure the jump target is correct after linking. This is due
   // to a limitation that shouldForceRelocation has to make the decision upfront
-  // without knowing a possibly future .option relax. When RISCVAsmParser is used,
-  // its ParseInstruction may call setForceRelocs as well.
+  // without knowing a possibly future .option relax. When RISCVAsmParser is
+  // used, its ParseInstruction may call setForceRelocs as well.
   if (STI.hasFeature(RISCV::FeatureRelax))
     static_cast<RISCVAsmBackend &>(MAB).setForceRelocs();
 }

[topperc]

LGTM

[MaskRay]

This addresses the issue when the initial MCSubtargetInfo contains FeatureRelax, there is a relaxable instruction in a norelax region, and we want to ensure that the relaxation instruction has an associated relocation (even if no R_RISCV_RELAX).

There is another interesting case (probably invalid) when the initial MCSubtargetInfo doesn't contain FeatureRelax and we have .option relax later. Perhaps we can call it invalid as EF_RISCV_RELAX is not set. Nevertheless, this isn't a case handled by our previous behavior. If we really want to handle the case, we probably should use a per-function approach like RISCVAsmPrinter::emitDirectiveOptionArch, but we don't have access to MCAsmBackend there.