Skip to content

Commit bb03cdc

Browse files
authored
RISCV: Remove shouldForceRelocation and unneeded relocations
Follow-up to #140494 `shouldForceRelocation` is conservative and produces redundant relocations. For example, RISCVAsmBackend::ForceRelocs (introduced to support mixed relax/norelax code) leads to redundant relocations in the following example adapted from #77436 ``` .option norelax j label // For assembly input, RISCVAsmParser::ParseInstruction sets ForceRelocs (https://reviews.llvm.org/D46423). // For direct object emission, RISCVELFStreamer sets ForceRelocs (#77436) .option relax call foo // linker-relaxable .option norelax j label // redundant relocation due to ForceRelocs .option relax label: ``` Root problem: The `isSymbolRefDifferenceFullyResolvedImpl` condition in MCAssembler::evaluateFixup does not check whether two locations are separated by a fragment whose size can be indeterminate due to linker instruction (e.g. MCDataFragment with relaxation, or MCAlignFragment due to indeterminate start offst). This patch * Updates the fragment walk code in `attemptToFoldSymbolOffsetDifference` to treat MCRelaxableFragment (for --riscv-asm-relax-branches) as fixed size after finishLayout. * Adds a condition in `addReloc` to complement `isSymbolRefDifferenceFullyResolvedImpl`. * Removes the no longer needed `shouldForceRelocation`. This fragment walk code path handles nicely handles mixed relax/norelax case from https://discourse.llvm.org/t/possible-problem-related-to-subtarget-usage/75283 and allows us to remove `MCSubtargetInfo` argument (#73721) as a follow-up. This fragment walk code should be avoided in the absence of linker-relaxable fragments within the current section. Adjust two bolt/test/RISCV tests (#141310) Pull Request: #140692
1 parent d5802c3 commit bb03cdc

22 files changed

+86
-112
lines changed

bolt/test/RISCV/reloc-label-diff.s

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -20,4 +20,6 @@ _test_end:
2020
.data
2121
// CHECK: Hex dump of section '.data':
2222
// CHECK: 0x{{.*}} 04000000
23+
.reloc ., R_RISCV_ADD32, _test_end
24+
.reloc ., R_RISCV_SUB32, _start
2325
.word _test_end - _start

bolt/test/RISCV/reorder-blocks-reverse.s

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@
77
.p2align 1
88
_start:
99
nop
10+
.reloc ., R_RISCV_BRANCH, 1f
1011
beq t0, t1, 1f
1112
nop
1213
beq t0, t2, 2f

llvm/include/llvm/MC/MCAssembler.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -64,6 +64,7 @@ class MCAssembler {
6464
std::unique_ptr<MCObjectWriter> Writer;
6565

6666
bool HasLayout = false;
67+
bool HasFinalLayout = false;
6768
bool RelaxAll = false;
6869

6970
SectionListType Sections;
@@ -197,6 +198,7 @@ class MCAssembler {
197198
void layout();
198199

199200
bool hasLayout() const { return HasLayout; }
201+
bool hasFinalLayout() const { return HasFinalLayout; }
200202
bool getRelaxAll() const { return RelaxAll; }
201203
void setRelaxAll(bool Value) { RelaxAll = Value; }
202204

llvm/include/llvm/MC/MCFixup.h

Lines changed: 5 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -73,9 +73,9 @@ class MCFixup {
7373
/// determine how the operand value should be encoded into the instruction.
7474
MCFixupKind Kind = FK_NONE;
7575

76-
/// Used by RISC-V style linker relaxation. If the fixup is unresolved,
77-
/// whether a RELAX relocation should follow.
78-
bool NeedsRelax = false;
76+
/// Used by RISC-V style linker relaxation. Whether the fixup is
77+
/// linker-relaxable.
78+
bool LinkerRelaxable = false;
7979

8080
/// Consider bit fields if we need more flags.
8181

@@ -105,8 +105,8 @@ class MCFixup {
105105

106106
const MCExpr *getValue() const { return Value; }
107107

108-
bool needsRelax() const { return NeedsRelax; }
109-
void setNeedsRelax() { NeedsRelax = true; }
108+
bool isLinkerRelaxable() const { return LinkerRelaxable; }
109+
void setLinkerRelaxable() { LinkerRelaxable = true; }
110110

111111
/// Return the generic fixup kind for a value with the given size. It
112112
/// is an error to pass an unsupported size.

llvm/include/llvm/MC/MCSection.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,10 @@ class MCSection {
107107

108108
bool IsVirtual : 1;
109109

110+
/// Whether the section contains linker-relaxable fragments. If true, the
111+
/// offset between two locations may not be fully resolved.
112+
bool LinkerRelaxable : 1;
113+
110114
MCDummyFragment DummyFragment;
111115

112116
// Mapping from subsection number to fragment list. At layout time, the
@@ -175,6 +179,9 @@ class MCSection {
175179
bool isRegistered() const { return IsRegistered; }
176180
void setIsRegistered(bool Value) { IsRegistered = Value; }
177181

182+
bool isLinkerRelaxable() const { return LinkerRelaxable; }
183+
void setLinkerRelaxable() { LinkerRelaxable = true; }
184+
178185
const MCDummyFragment &getDummyFragment() const { return DummyFragment; }
179186
MCDummyFragment &getDummyFragment() { return DummyFragment; }
180187

llvm/lib/MC/MCAssembler.cpp

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -897,6 +897,10 @@ void MCAssembler::layout() {
897897
// example, to set the index fields in the symbol data).
898898
getWriter().executePostLayoutBinding(*this);
899899

900+
// Fragment sizes are finalized. For RISC-V linker relaxation, this flag
901+
// helps check whether a PC-relative fixup is fully resolved.
902+
this->HasFinalLayout = true;
903+
900904
// Evaluate and apply the fixups, generating relocation entries as necessary.
901905
for (MCSection &Sec : *this) {
902906
for (MCFragment &Frag : Sec) {

llvm/lib/MC/MCELFStreamer.cpp

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -450,8 +450,10 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
450450
auto Fixups = MutableArrayRef(DF->getFixups()).slice(FixupStartIndex);
451451
for (auto &Fixup : Fixups) {
452452
Fixup.setOffset(Fixup.getOffset() + CodeOffset);
453-
if (Fixup.needsRelax())
453+
if (Fixup.isLinkerRelaxable()) {
454454
DF->setLinkerRelaxable();
455+
getCurrentSectionOnly()->setLinkerRelaxable();
456+
}
455457
}
456458

457459
DF->setHasInstructions(STI);

llvm/lib/MC/MCExpr.cpp

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -390,6 +390,12 @@ static void attemptToFoldSymbolOffsetDifference(const MCAssembler *Asm,
390390
unsigned Count;
391391
if (DF) {
392392
Displacement += DF->getContents().size();
393+
} else if (auto *RF = dyn_cast<MCRelaxableFragment>(FI);
394+
RF && Asm->hasFinalLayout()) {
395+
// Before finishLayout, a relaxable fragment's size is indeterminate.
396+
// After layout, during relocation generation, it can be treated as a
397+
// data fragment.
398+
Displacement += RF->getContents().size();
393399
} else if (auto *AF = dyn_cast<MCAlignFragment>(FI);
394400
AF && Layout && AF->hasEmitNops() &&
395401
!Asm->getBackend().shouldInsertExtraNopBytesForCodeAlign(

llvm/lib/MC/MCSection.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ MCSection::MCSection(SectionVariant V, StringRef Name, bool IsText,
2323
bool IsVirtual, MCSymbol *Begin)
2424
: Begin(Begin), BundleGroupBeforeFirstInst(false), HasInstructions(false),
2525
HasLayout(false), IsRegistered(false), IsText(IsText),
26-
IsVirtual(IsVirtual), Name(Name), Variant(V) {
26+
IsVirtual(IsVirtual), LinkerRelaxable(false), Name(Name), Variant(V) {
2727
DummyFragment.setParent(this);
2828
// The initial subsection number is 0. Create a fragment list.
2929
CurFragList = &Subsections.emplace_back(0u, FragList{}).second;

llvm/lib/Target/LoongArch/MCTargetDesc/LoongArchAsmBackend.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -504,7 +504,7 @@ bool LoongArchAsmBackend::addReloc(MCAssembler &Asm, const MCFragment &F,
504504
IsResolved = Fallback();
505505
// If linker relaxation is enabled and supported by the current relocation,
506506
// append a RELAX relocation.
507-
if (Fixup.needsRelax()) {
507+
if (Fixup.isLinkerRelaxable()) {
508508
auto FA = MCFixup::create(Fixup.getOffset(), nullptr, ELF::R_LARCH_RELAX);
509509
Asm.getWriter().recordRelocation(Asm, &F, FA, MCValue::get(nullptr),
510510
FixedValueA);

0 commit comments

Comments
 (0)