llvmbot wrote:
<!--LLVM PR SUMMARY COMMENT--> @llvm/pr-subscribers-backend-risc-v Author: Sam Elliott (lenary) <details> <summary>Changes</summary> Span-dependent instructions on RISC-V interact in a complex manner with linker relaxation. The span-dependent assembler algorithm implemented in LLVM has to start with the smallest version of an instruction and then only make it larger, so we compress instructions before emitting them to the streamer. When the instruction is streamed, the information that the instruction (or rather, the fixup on the instruction) is linker relaxable must be accurate, even though the assembler relaxation process may transform a not-linker-relaxable instruction/fixup into one that that is linker relaxable, for instance `c.jal` becoming `qc.e.jal`, or `bne` getting turned into `beq; jal` (the `jal` is linker relaxable). In order for this to work, the following things have to happen: - Any instruction/fixup which might be relaxed to a linker-relaxable instruction/fixup, gets marked as `RelaxCandidate = true` in RISCVMCCodeEmitter. - In RISCVAsmBackend, when emitting the `R_RISCV_RELAX` relocation, we have to check that the relocation/fixup kind is one that may need a relax relocation, as well as that it is marked as linker relaxable (the latter will not be set if relaxation is disabled). - Linker Relaxable instructions streamed to a Relaxable fragment need to mark the fragment and its section as linker relaxable. I also added more debug output for Sections/Fixups which are marked Linker Relaxable. This results in more relocations, when these PC-relative fixups cross an instruction with a fixup that is resolved as not linker-relaxable but caused the fragment to be marked linker relaxable at streaming time (i.e. `c.j`). (cherry picked from commit 9e8f7acd2b3a71dad473565a6a6f3ba51a3e6bca) This cherry-pick differs from the commit on `main`, because the MC layer there has been significantly refactored. This version is more conservative when emitting relocations (more relocations are emitted, even when not needed). Fixes: #<!-- -->150071 --- Full diff: https://github.com/llvm/llvm-project/pull/153670.diff 8 Files Affected: - (modified) llvm/lib/MC/MCExpr.cpp (+4) - (modified) llvm/lib/MC/MCFragment.cpp (+4) - (modified) llvm/lib/MC/MCObjectStreamer.cpp (+7) - (modified) llvm/lib/MC/MCSection.cpp (+2) - (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp (+33-9) - (modified) llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp (+25-4) - (modified) llvm/test/MC/RISCV/Relocations/mc-dump.s (+2-2) - (added) llvm/test/MC/RISCV/xqcibi-linker-relaxation.s (+84) ``````````diff diff --git a/llvm/lib/MC/MCExpr.cpp b/llvm/lib/MC/MCExpr.cpp index a196591744099..243704c68000a 100644 --- a/llvm/lib/MC/MCExpr.cpp +++ b/llvm/lib/MC/MCExpr.cpp @@ -361,6 +361,10 @@ static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm, if (BBeforeRelax && AAfterRelax) return; } + const auto *RF = dyn_cast<MCRelaxableFragment>(F); + if (RF && RF->isLinkerRelaxable()) { + return; + } if (&*F == FA) { // If FA and FB belong to the same subsection, the loop will find FA and // we can resolve the difference. diff --git a/llvm/lib/MC/MCFragment.cpp b/llvm/lib/MC/MCFragment.cpp index c59fabe5df1b9..28cddb9396467 100644 --- a/llvm/lib/MC/MCFragment.cpp +++ b/llvm/lib/MC/MCFragment.cpp @@ -70,6 +70,8 @@ LLVM_DUMP_METHOD void MCFragment::dump() const { OS << "\n Fixup @" << F.getOffset() << " Value:"; F.getValue()->print(OS, nullptr); OS << " Kind:" << F.getKind(); + if (F.isLinkerRelaxable()) + OS << " LinkerRelaxable"; } }; @@ -113,6 +115,8 @@ LLVM_DUMP_METHOD void MCFragment::dump() const { } case MCFragment::FT_Relaxable: { const auto *F = cast<MCRelaxableFragment>(this); + if (F->isLinkerRelaxable()) + OS << " LinkerRelaxable"; OS << " Size:" << F->getContents().size() << ' '; F->getInst().dump_pretty(OS); printFixups(F->getFixups()); diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp index 44a82f75576b6..d9c39bbedf37c 100644 --- a/llvm/lib/MC/MCObjectStreamer.cpp +++ b/llvm/lib/MC/MCObjectStreamer.cpp @@ -408,6 +408,13 @@ void MCObjectStreamer::emitInstToFragment(const MCInst &Inst, Inst, IF->getContentsForAppending(), Fixups, STI); IF->doneAppending(); IF->appendFixups(Fixups); + + for (auto &Fixup : Fixups) { + if (Fixup.isLinkerRelaxable()) { + IF->setLinkerRelaxable(); + getCurrentSectionOnly()->setLinkerRelaxable(); + } + } } #ifndef NDEBUG diff --git a/llvm/lib/MC/MCSection.cpp b/llvm/lib/MC/MCSection.cpp index beb472b7c7deb..a7330692571de 100644 --- a/llvm/lib/MC/MCSection.cpp +++ b/llvm/lib/MC/MCSection.cpp @@ -63,6 +63,8 @@ LLVM_DUMP_METHOD void MCSection::dump( raw_ostream &OS = errs(); OS << "MCSection Name:" << getName(); + if (isLinkerRelaxable()) + OS << " LinkerRelaxable"; for (auto &F : *this) { OS << '\n'; F.dump(); diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp index e42d6c539a349..df3028e051399 100644 --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp +++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp @@ -792,6 +792,23 @@ void RISCVAsmBackend::maybeAddVendorReloc(const MCFragment &F, Asm->getWriter().recordRelocation(F, VendorFixup, VendorTarget, VendorValue); } +static bool relaxableFixupNeedsRelocation(const MCFixupKind Kind) { + // Some Fixups are marked as LinkerRelaxable by + // `RISCVMCCodeEmitter::getImmOpValue` only because they may be + // (assembly-)relaxed into a linker-relaxable instruction. This function + // should return `false` for those fixups so they do not get a `R_RISCV_RELAX` + // relocation emitted in addition to the relocation. + switch (Kind) { + default: + break; + case RISCV::fixup_riscv_rvc_jump: + case RISCV::fixup_riscv_rvc_branch: + case RISCV::fixup_riscv_jal: + return false; + } + return true; +} + bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup, const MCValue &Target, uint64_t &FixedValue, bool IsResolved) { @@ -834,25 +851,32 @@ bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup, return false; } - // If linker relaxation is enabled and supported by the current relocation, - // generate a relocation and then append a RELAX. - if (Fixup.isLinkerRelaxable()) + // If linker relaxation is enabled and supported by the current fixup, then we + // always want to generate a relocation. + bool NeedsRelax = Fixup.isLinkerRelaxable() && + relaxableFixupNeedsRelocation(Fixup.getKind()); + if (NeedsRelax) IsResolved = false; + if (IsResolved && Fixup.isPCRel()) IsResolved = isPCRelFixupResolved(Target.getAddSym(), F); if (!IsResolved) { - // Some Fixups require a vendor relocation, record it (directly) before we + // Some Fixups require a VENDOR relocation, record it (directly) before we // add the relocation. maybeAddVendorReloc(F, Fixup); Asm->getWriter().recordRelocation(F, Fixup, Target, FixedValue); - } - if (Fixup.isLinkerRelaxable()) { - auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_RELAX); - Asm->getWriter().recordRelocation(F, FA, MCValue::get(nullptr), - FixedValueA); + if (NeedsRelax) { + // Some Fixups get a RELAX relocation, record it (directly) after we add + // the relocation. + MCFixup RelaxFixup = + MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_RISCV_RELAX); + MCValue RelaxTarget = MCValue::get(nullptr); + uint64_t RelaxValue; + Asm->getWriter().recordRelocation(F, RelaxFixup, RelaxTarget, RelaxValue); + } } return false; diff --git a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp index cbeabdddb9371..717fba68b48ed 100644 --- a/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp +++ b/llvm/lib/Target/RISCV/MCTargetDesc/RISCVMCCodeEmitter.cpp @@ -576,8 +576,21 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo, "getImmOpValue expects only expressions or immediates"); const MCExpr *Expr = MO.getExpr(); MCExpr::ExprKind Kind = Expr->getKind(); - unsigned FixupKind = RISCV::fixup_riscv_invalid; + + // `RelaxCandidate` must be set to `true` in two cases: + // - The fixup's relocation gets a R_RISCV_RELAX relocation + // - The underlying instruction may be relaxed to an instruction that gets a + // `R_RISCV_RELAX` relocation. + // + // The actual emission of `R_RISCV_RELAX` will be handled in + // `RISCVAsmBackend::applyFixup`. bool RelaxCandidate = false; + auto AsmRelaxToLinkerRelaxableWithFeature = [&](unsigned Feature) -> void { + if (!STI.hasFeature(RISCV::FeatureExactAssembly) && STI.hasFeature(Feature)) + RelaxCandidate = true; + }; + + unsigned FixupKind = RISCV::fixup_riscv_invalid; if (Kind == MCExpr::Specifier) { const auto *RVExpr = cast<MCSpecifierExpr>(Expr); FixupKind = RVExpr->getSpecifier(); @@ -644,18 +657,26 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo, // FIXME: Sub kind binary exprs have chance of underflow. if (MIFrm == RISCVII::InstFormatJ) { FixupKind = RISCV::fixup_riscv_jal; + AsmRelaxToLinkerRelaxableWithFeature(RISCV::FeatureVendorXqcilb); } else if (MIFrm == RISCVII::InstFormatB) { FixupKind = RISCV::fixup_riscv_branch; + // This might be assembler relaxed to `b<cc>; jal` but we cannot relax + // the `jal` again in the assembler. } else if (MIFrm == RISCVII::InstFormatCJ) { FixupKind = RISCV::fixup_riscv_rvc_jump; + AsmRelaxToLinkerRelaxableWithFeature(RISCV::FeatureVendorXqcilb); } else if (MIFrm == RISCVII::InstFormatCB) { FixupKind = RISCV::fixup_riscv_rvc_branch; + // This might be assembler relaxed to `b<cc>; jal` but we cannot relax + // the `jal` again in the assembler. } else if (MIFrm == RISCVII::InstFormatCI) { FixupKind = RISCV::fixup_riscv_rvc_imm; } else if (MIFrm == RISCVII::InstFormatI) { FixupKind = RISCV::fixup_riscv_12_i; } else if (MIFrm == RISCVII::InstFormatQC_EB) { FixupKind = RISCV::fixup_riscv_qc_e_branch; + // This might be assembler relaxed to `qc.e.b<cc>; jal` but we cannot + // relax the `jal` again in the assembler. } else if (MIFrm == RISCVII::InstFormatQC_EAI) { FixupKind = RISCV::fixup_riscv_qc_e_32; RelaxCandidate = true; @@ -670,9 +691,9 @@ uint64_t RISCVMCCodeEmitter::getImmOpValue(const MCInst &MI, unsigned OpNo, assert(FixupKind != RISCV::fixup_riscv_invalid && "Unhandled expression!"); addFixup(Fixups, 0, Expr, FixupKind); - // If linker relaxation is enabled and supported by this relocation, set - // a bit so that if fixup is unresolved, a R_RISCV_RELAX relocation will be - // appended. + // If linker relaxation is enabled and supported by this relocation, set a bit + // so that the assembler knows the size of the instruction is not fixed/known, + // and the relocation will need a R_RISCV_RELAX relocation. if (EnableRelax && RelaxCandidate) Fixups.back().setLinkerRelaxable(); ++MCNumFixups; diff --git a/llvm/test/MC/RISCV/Relocations/mc-dump.s b/llvm/test/MC/RISCV/Relocations/mc-dump.s index 24f3e67ebbdda..c646e6f54b261 100644 --- a/llvm/test/MC/RISCV/Relocations/mc-dump.s +++ b/llvm/test/MC/RISCV/Relocations/mc-dump.s @@ -2,12 +2,12 @@ # RUN: llvm-mc -filetype=obj --triple=riscv64 --mattr=+relax %s -debug-only=mc-dump -o /dev/null 2>&1 | FileCheck %s # CHECK:Sections:[ -# CHECK-NEXT:MCSection Name:.text +# CHECK-NEXT:MCSection Name:.text LinkerRelaxable # CHECK-NEXT:0 Data Size:0 [] # CHECK-NEXT: Symbol @0 .text # CHECK-NEXT:0 Align Align:4 Fill:0 FillLen:1 MaxBytesToEmit:4 Nops # CHECK-NEXT:0 Data LinkerRelaxable Size:8 [97,00,00,00,e7,80,00,00] -# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4023 +# CHECK-NEXT: Fixup @0 Value:specifier(19,ext) Kind:4023 LinkerRelaxable # CHECK-NEXT: Symbol @0 $x # CHECK-NEXT:8 Align Align:8 Fill:0 FillLen:1 MaxBytesToEmit:8 Nops # CHECK-NEXT:12 Data Size:4 [13,05,30,00] diff --git a/llvm/test/MC/RISCV/xqcibi-linker-relaxation.s b/llvm/test/MC/RISCV/xqcibi-linker-relaxation.s new file mode 100644 index 0000000000000..b12585265bf1d --- /dev/null +++ b/llvm/test/MC/RISCV/xqcibi-linker-relaxation.s @@ -0,0 +1,84 @@ +# RUN: llvm-mc --triple=riscv32 -mattr=+relax,+experimental-xqcilb,+experimental-xqcibi \ +# RUN: %s -filetype=obj -o - -riscv-add-build-attributes \ +# RUN: | llvm-objdump -dr -M no-aliases - \ +# RUN: | FileCheck %s + +## This tests that we correctly emit relocations for linker relaxation when +## relaxing `JAL` to `QC.E.JAL`. + +## PR150071 + +.global foo + +# CHECK-LABEL: <branch_over_relaxable>: +branch_over_relaxable: + jal x1, foo +# CHECK: qc.e.jal 0x0 <branch_over_relaxable> +# CHECK-NEXT: R_RISCV_VENDOR QUALCOMM +# CHECK-NEXT: R_RISCV_CUSTOM195 foo +# CHECK-NEXT: R_RISCV_RELAX *ABS* + bne a0, a1, branch_over_relaxable +# CHECK-NEXT: bne a0, a1, 0x6 <branch_over_relaxable+0x6> +# CHECK-NEXT: R_RISCV_BRANCH branch_over_relaxable +# CHECK-NOT: R_RISCV_RELAX + qc.e.bnei a0, 0x21, branch_over_relaxable +# CHECK-NEXT: qc.e.bnei a0, 0x21, 0xa <branch_over_relaxable+0xa> +# CHECK-NEXT: R_RISCV_VENDOR QUALCOMM +# CHECK-NEXT: R_RISCV_CUSTOM193 branch_over_relaxable +# CHECK-NOT: R_RISCV_RELAX + ret +# CHECK-NEXT: c.jr ra + +# CHECK-LABEL: <short_jump_over_fixed>: +short_jump_over_fixed: + nop +# CHECK: c.nop + j short_jump_over_fixed +# CHECK-NEXT: c.j 0x14 <short_jump_over_fixed+0x2> +# CHECK-NEXT: R_RISCV_RVC_JUMP short_jump_over_fixed +# CHECK-NOT: R_RISCV_RELAX + ret +# CHECK-NEXT: c.jr ra + +# CHECK-LABEL: <short_jump_over_relaxable>: +short_jump_over_relaxable: + call foo +# CHECK: auipc ra, 0x0 +# CHECK-NEXT: R_RISCV_CALL_PLT foo +# CHECK-NEXT: R_RISCV_RELAX *ABS* +# CHECK-NEXT: jalr ra, 0x0(ra) <short_jump_over_relaxable> + j short_jump_over_relaxable +# CHECK-NEXT: c.j 0x20 <short_jump_over_relaxable+0x8> +# CHECK-NEXT: R_RISCV_RVC_JUMP short_jump_over_relaxable +# CHECK-NOT: R_RISCV_RELAX + ret +# CHECK-NEXT: c.jr ra + +# CHECK-LABEL: <mid_jump_over_fixed>: +mid_jump_over_fixed: + nop +# CHECK: c.nop + .space 0x1000 +# CHECK-NEXT: ... + j mid_jump_over_fixed +# CHECK-NEXT: jal zero, 0x1026 <mid_jump_over_fixed+0x1002> +# CHECK-NEXT: R_RISCV_JAL mid_jump_over_fixed +# CHECK-NOT: R_RISCV_RELAX + ret +# CHECK-NEXT: c.jr ra + +# CHECK-LABEL: <mid_jump_over_relaxable>: +mid_jump_over_relaxable: + call foo +# CHECK: auipc ra, 0x0 +# CHECK-NEXT: R_RISCV_CALL_PLT foo +# CHECK-NEXT: R_RISCV_RELAX *ABS* +# CHECK-NEXT: jalr ra, 0x0(ra) <mid_jump_over_relaxable> + .space 0x1000 +# CHECK-NEXT: ... + j mid_jump_over_relaxable +# CHECK-NEXT: jal zero, 0x2034 <mid_jump_over_relaxable+0x1008> +# CHECK-NEXT: R_RISCV_JAL mid_jump_over_relaxable +# CHECK-NOT: R_RISCV_RELAX + ret +# CHECK-NEXT: c.jr ra `````````` </details> https://github.com/llvm/llvm-project/pull/153670 _______________________________________________ llvm-branch-commits mailing list llvm-branch-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-branch-commits
