Skip to content

Commit 5d87ebf

Browse files
MaskRayMaskRay
committed
[MC] Refactor fixup evaluation and relocation generation
Follow-up to commits 5710759 and 634f9a9 - Integrate `evaluateFixup` into `recordRelocation` and inline code within `MCAssembler::layout`, removing `handleFixup`. - Update `fixupNeedsRelaxation` to bypass `shouldForceRelocation` when calling `evaluateFixup`, eliminating the `WasForced` workaround for RISC-V linker relaxation (https://reviews.llvm.org/D46350 ).
1 parent af7a7ba commit 5d87ebf

File tree

12 files changed

+80
-120
lines changed

12 files changed

+80
-120
lines changed

‎llvm/include/llvm/MC/MCAsmBackend.h‎

Lines changed: 5 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -109,11 +109,10 @@ class MCAsmBackend {
109109
return false;
110110
}
111111

112-
virtual bool evaluateTargetFixup(const MCAssembler &Asm,
113-
const MCFixup &Fixup, const MCFragment *DF,
114-
const MCValue &Target,
115-
const MCSubtargetInfo *STI, uint64_t &Value,
116-
bool &WasForced) {
112+
virtual bool evaluateTargetFixup(const MCAssembler &Asm, const MCFixup &Fixup,
113+
const MCFragment *DF, const MCValue &Target,
114+
const MCSubtargetInfo *STI,
115+
uint64_t &Value) {
117116
llvm_unreachable("Need to implement hook if target has custom fixups");
118117
}
119118

@@ -156,8 +155,7 @@ class MCAsmBackend {
156155
virtual bool fixupNeedsRelaxationAdvanced(const MCAssembler &,
157156
const MCRelaxableFragment &,
158157
const MCFixup &, const MCValue &,
159-
uint64_t, bool Resolved,
160-
bool WasForced) const;
158+
uint64_t, bool Resolved) const;
161159

162160
/// Simple predicate for targets where !Resolved implies requiring relaxation
163161
virtual bool fixupNeedsRelaxation(const MCFixup &Fixup,

‎llvm/include/llvm/MC/MCAssembler.h‎

Lines changed: 2 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -95,14 +95,11 @@ class MCAssembler {
9595
/// evaluates to.
9696
/// \param Value [out] On return, the value of the fixup as currently laid
9797
/// out.
98-
/// \param WasForced [out] On return, the value in the fixup is set to the
99-
/// correct value if WasForced is true, even if evaluateFixup returns false.
100-
/// \return Whether the fixup value was fully resolved. This is true if the
101-
/// \p Value result is fixed, otherwise the value may change due to
98+
/// \param RecordReloc Record relocation if needed.
10299
/// relocation.
103100
bool evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
104101
MCValue &Target, const MCSubtargetInfo *STI,
105-
uint64_t &Value, bool &WasForced) const;
102+
uint64_t &Value, bool RecordReloc) const;
106103

107104
/// Check whether a fixup can be satisfied, or whether it needs to be relaxed
108105
/// (increased in size, in order to hold its value correctly).
@@ -127,9 +124,6 @@ class MCAssembler {
127124
bool relaxCVDefRange(MCCVDefRangeFragment &DF);
128125
bool relaxPseudoProbeAddr(MCPseudoProbeAddrFragment &DF);
129126

130-
std::tuple<MCValue, uint64_t, bool>
131-
handleFixup(MCFragment &F, const MCFixup &Fixup, const MCSubtargetInfo *STI);
132-
133127
public:
134128
/// Construct a new assembler instance.
135129
//

‎llvm/lib/MC/MCAsmBackend.cpp‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -115,9 +115,11 @@ bool MCAsmBackend::shouldForceRelocation(const MCAssembler &, const MCFixup &,
115115
return Target.getSpecifier();
116116
}
117117

118-
bool MCAsmBackend::fixupNeedsRelaxationAdvanced(
119-
const MCAssembler &, const MCRelaxableFragment &, const MCFixup &Fixup,
120-
const MCValue &, uint64_t Value, bool Resolved, bool WasForced) const {
118+
bool MCAsmBackend::fixupNeedsRelaxationAdvanced(const MCAssembler &,
119+
const MCRelaxableFragment &,
120+
const MCFixup &Fixup,
121+
const MCValue &, uint64_t Value,
122+
bool Resolved) const {
121123
if (!Resolved)
122124
return true;
123125
return fixupNeedsRelaxation(Fixup, Value);

‎llvm/lib/MC/MCAssembler.cpp‎

Lines changed: 46 additions & 62 deletions
Original file line numberDiff line numberDiff line change
@@ -138,7 +138,7 @@ bool MCAssembler::isThumbFunc(const MCSymbol *Symbol) const {
138138

139139
bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
140140
MCValue &Target, const MCSubtargetInfo *STI,
141-
uint64_t &Value, bool &WasForced) const {
141+
uint64_t &Value, bool RecordReloc) const {
142142
++stats::evaluateFixup;
143143

144144
// FIXME: This code has some duplication with recordRelocation. We should
@@ -150,47 +150,51 @@ bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
150150
const MCExpr *Expr = Fixup.getValue();
151151
MCContext &Ctx = getContext();
152152
Value = 0;
153-
WasForced = false;
154153
if (!Expr->evaluateAsRelocatable(Target, this)) {
155154
Ctx.reportError(Fixup.getLoc(), "expected relocatable expression");
156155
return true;
157156
}
158157

159-
unsigned FixupFlags = getBackend().getFixupKindInfo(Fixup.getKind()).Flags;
160-
if (FixupFlags & MCFixupKindInfo::FKF_IsTarget)
161-
return getBackend().evaluateTargetFixup(*this, Fixup, DF, Target, STI,
162-
Value, WasForced);
163-
164-
const MCSymbol *Add = Target.getAddSym();
165-
const MCSymbol *Sub = Target.getSubSym();
166-
bool IsPCRel = FixupFlags & MCFixupKindInfo::FKF_IsPCRel;
167158
bool IsResolved = false;
168-
if (!IsPCRel) {
169-
IsResolved = Target.isAbsolute();
170-
} else if (Add && !Sub && !Add->isUndefined() && !Add->isAbsolute()) {
171-
IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||
172-
getWriter().isSymbolRefDifferenceFullyResolvedImpl(
173-
*this, *Add, *DF, false, true);
159+
unsigned FixupFlags = getBackend().getFixupKindInfo(Fixup.getKind()).Flags;
160+
if (FixupFlags & MCFixupKindInfo::FKF_IsTarget) {
161+
IsResolved =
162+
getBackend().evaluateTargetFixup(*this, Fixup, DF, Target, STI, Value);
163+
} else {
164+
const MCSymbol *Add = Target.getAddSym();
165+
const MCSymbol *Sub = Target.getSubSym();
166+
Value = Target.getConstant();
167+
if (Add && Add->isDefined())
168+
Value += getSymbolOffset(*Add);
169+
if (Sub && Sub->isDefined())
170+
Value -= getSymbolOffset(*Sub);
171+
172+
bool IsPCRel = FixupFlags & MCFixupKindInfo::FKF_IsPCRel;
173+
bool ShouldAlignPC =
174+
FixupFlags & MCFixupKindInfo::FKF_IsAlignedDownTo32Bits;
175+
if (IsPCRel) {
176+
uint64_t Offset = getFragmentOffset(*DF) + Fixup.getOffset();
177+
178+
// A number of ARM fixups in Thumb mode require that the effective PC
179+
// address be determined as the 32-bit aligned version of the actual
180+
// offset.
181+
if (ShouldAlignPC)
182+
Offset &= ~0x3;
183+
Value -= Offset;
184+
185+
if (Add && !Sub && !Add->isUndefined() && !Add->isAbsolute()) {
186+
IsResolved = (FixupFlags & MCFixupKindInfo::FKF_Constant) ||
187+
getWriter().isSymbolRefDifferenceFullyResolvedImpl(
188+
*this, *Add, *DF, false, true);
189+
}
190+
} else {
191+
IsResolved = Target.isAbsolute();
192+
assert(!ShouldAlignPC && "FKF_IsAlignedDownTo32Bits must be PC-relative");
193+
}
174194
}
175195

176-
Value = Target.getConstant();
177-
if (Add && Add->isDefined())
178-
Value += getSymbolOffset(*Add);
179-
if (Sub && Sub->isDefined())
180-
Value -= getSymbolOffset(*Sub);
181-
182-
bool ShouldAlignPC = FixupFlags & MCFixupKindInfo::FKF_IsAlignedDownTo32Bits;
183-
assert((ShouldAlignPC ? IsPCRel : true) &&
184-
"FKF_IsAlignedDownTo32Bits is only allowed on PC-relative fixups!");
185-
186-
if (IsPCRel) {
187-
uint64_t Offset = getFragmentOffset(*DF) + Fixup.getOffset();
188-
189-
// A number of ARM fixups in Thumb mode require that the effective PC
190-
// address be determined as the 32-bit aligned version of the actual offset.
191-
if (ShouldAlignPC) Offset &= ~0x3;
192-
Value -= Offset;
193-
}
196+
if (!RecordReloc)
197+
return IsResolved;
194198

195199
// .reloc directive and the backend might force the relocation.
196200
// Backends that customize shouldForceRelocation generally just need the fixup
@@ -200,12 +204,12 @@ bool MCAssembler::evaluateFixup(const MCFixup &Fixup, const MCFragment *DF,
200204
auto TargetVal = Target;
201205
TargetVal.Cst = Value;
202206
if (Fixup.getKind() >= FirstLiteralRelocationKind ||
203-
getBackend().shouldForceRelocation(*this, Fixup, TargetVal, STI)) {
207+
getBackend().shouldForceRelocation(*this, Fixup, TargetVal, STI))
204208
IsResolved = false;
205-
WasForced = true;
206-
}
207209
}
208-
210+
if (!IsResolved)
211+
getWriter().recordRelocation(const_cast<MCAssembler &>(*this), DF, Fixup,
212+
Target, Value);
209213
return IsResolved;
210214
}
211215

@@ -844,24 +848,6 @@ void MCAssembler::writeSectionData(raw_ostream &OS,
844848
OS.tell() - Start == getSectionAddressSize(*Sec));
845849
}
846850

847-
std::tuple<MCValue, uint64_t, bool>
848-
MCAssembler::handleFixup(MCFragment &F, const MCFixup &Fixup,
849-
const MCSubtargetInfo *STI) {
850-
// Evaluate the fixup.
851-
MCValue Target;
852-
uint64_t FixedValue;
853-
bool WasForced;
854-
bool IsResolved =
855-
evaluateFixup(Fixup, &F, Target, STI, FixedValue, WasForced);
856-
if (!IsResolved) {
857-
// The fixup was unresolved, we need a relocation. Inform the object
858-
// writer of the relocation, and give it an opportunity to adjust the
859-
// fixup value if need be.
860-
getWriter().recordRelocation(*this, &F, Fixup, Target, FixedValue);
861-
}
862-
return std::make_tuple(Target, FixedValue, IsResolved);
863-
}
864-
865851
void MCAssembler::layout() {
866852
assert(getBackendPtr() && "Expected assembler backend");
867853
DEBUG_WITH_TYPE("mc-dump", {
@@ -987,10 +973,9 @@ void MCAssembler::layout() {
987973
}
988974
for (const MCFixup &Fixup : Fixups) {
989975
uint64_t FixedValue;
990-
bool IsResolved;
991976
MCValue Target;
992-
std::tie(Target, FixedValue, IsResolved) =
993-
handleFixup(Frag, Fixup, STI);
977+
bool IsResolved = evaluateFixup(Fixup, &Frag, Target, STI, FixedValue,
978+
/*RecordReloc=*/true);
994979
getBackend().applyFixup(*this, Fixup, Target, Contents, FixedValue,
995980
IsResolved, STI);
996981
}
@@ -1012,11 +997,10 @@ bool MCAssembler::fixupNeedsRelaxation(const MCFixup &Fixup,
1012997
assert(getBackendPtr() && "Expected assembler backend");
1013998
MCValue Target;
1014999
uint64_t Value;
1015-
bool WasForced;
10161000
bool Resolved = evaluateFixup(Fixup, DF, Target, DF->getSubtargetInfo(),
1017-
Value, WasForced);
1001+
Value, /*RecordReloc=*/false);
10181002
return getBackend().fixupNeedsRelaxationAdvanced(*this, *DF, Fixup, Target,
1019-
Value, Resolved, WasForced);
1003+
Value, Resolved);
10201004
}
10211005

10221006
bool MCAssembler::fragmentNeedsRelaxation(const MCRelaxableFragment *F) const {

‎llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -356,7 +356,7 @@ static bool needsInterworking(const MCAssembler &Asm, const MCSymbol *Sym,
356356

357357
bool ARMAsmBackend::fixupNeedsRelaxationAdvanced(
358358
const MCAssembler &Asm, const MCRelaxableFragment &, const MCFixup &Fixup,
359-
const MCValue &Target, uint64_t Value, bool Resolved, bool) const {
359+
const MCValue &Target, uint64_t Value, bool Resolved) const {
360360
const MCSymbol *Sym = Target.getAddSym();
361361
if (needsInterworking(Asm, Sym, Fixup.getTargetKind()))
362362
return true;

‎llvm/lib/Target/ARM/MCTargetDesc/ARMAsmBackend.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -57,7 +57,7 @@ class ARMAsmBackend : public MCAsmBackend {
5757
bool fixupNeedsRelaxationAdvanced(const MCAssembler &,
5858
const MCRelaxableFragment &,
5959
const MCFixup &, const MCValue &, uint64_t,
60-
bool, bool) const override;
60+
bool) const override;
6161

6262
void relaxInstruction(MCInst &Inst,
6363
const MCSubtargetInfo &STI) const override;

‎llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.cpp‎

Lines changed: 5 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -171,17 +171,11 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
171171
}
172172
}
173173

174-
bool CSKYAsmBackend::fixupNeedsRelaxationAdvanced(const MCAssembler &,
175-
const MCRelaxableFragment &,
176-
const MCFixup &Fixup,
177-
const MCValue &,
178-
uint64_t Value, bool Resolved,
179-
const bool WasForced) const {
180-
// Return true if the symbol is actually unresolved.
181-
// Resolved could be always false when shouldForceRelocation return true.
182-
// We use !WasForced to indicate that the symbol is unresolved and not forced
183-
// by shouldForceRelocation.
184-
if (!Resolved && !WasForced)
174+
bool CSKYAsmBackend::fixupNeedsRelaxationAdvanced(
175+
const MCAssembler &, const MCRelaxableFragment &, const MCFixup &Fixup,
176+
const MCValue &, uint64_t Value, bool Resolved) const {
177+
// Return true if the symbol is unresolved.
178+
if (!Resolved)
185179
return true;
186180

187181
int64_t Offset = int64_t(Value);

‎llvm/lib/Target/CSKY/MCTargetDesc/CSKYAsmBackend.h‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,7 +42,7 @@ class CSKYAsmBackend : public MCAsmBackend {
4242
bool fixupNeedsRelaxationAdvanced(const MCAssembler &,
4343
const MCRelaxableFragment &,
4444
const MCFixup &, const MCValue &, uint64_t,
45-
bool, bool) const override;
45+
bool) const override;
4646

4747
bool writeNopData(raw_ostream &OS, uint64_t Count,
4848
const MCSubtargetInfo *STI) const override;

‎llvm/lib/Target/Hexagon/MCTargetDesc/HexagonAsmBackend.cpp‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -568,8 +568,8 @@ class HexagonAsmBackend : public MCAsmBackend {
568568
bool fixupNeedsRelaxationAdvanced(const MCAssembler &Asm,
569569
const MCRelaxableFragment &DF,
570570
const MCFixup &Fixup, const MCValue &,
571-
uint64_t Value, bool Resolved,
572-
bool) const override {
571+
uint64_t Value,
572+
bool Resolved) const override {
573573
MCInst const &MCB = DF.getInst();
574574
assert(HexagonMCInstrInfo::isBundle(MCB));
575575

‎llvm/lib/Target/RISCV/MCTargetDesc/RISCVAsmBackend.cpp‎

Lines changed: 7 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -143,19 +143,15 @@ bool RISCVAsmBackend::shouldForceRelocation(const MCAssembler &Asm,
143143

144144
bool RISCVAsmBackend::fixupNeedsRelaxationAdvanced(
145145
const MCAssembler &, const MCRelaxableFragment &, const MCFixup &Fixup,
146-
const MCValue &, uint64_t Value, bool Resolved,
147-
const bool WasForced) const {
146+
const MCValue &, uint64_t Value, bool Resolved) const {
148147
if (!RelaxBranches)
149148
return false;
150149

151150
int64_t Offset = int64_t(Value);
152151
unsigned Kind = Fixup.getTargetKind();
153152

154-
// Return true if the symbol is actually unresolved.
155-
// Resolved could be always false when shouldForceRelocation return true.
156-
// We use !WasForced to indicate that the symbol is unresolved and not forced
157-
// by shouldForceRelocation.
158-
if (!Resolved && !WasForced)
153+
// Return true if the symbol is unresolved.
154+
if (!Resolved)
159155
return true;
160156

161157
switch (Kind) {
@@ -595,12 +591,9 @@ static uint64_t adjustFixupValue(const MCFixup &Fixup, uint64_t Value,
595591
}
596592
}
597593

598-
bool RISCVAsmBackend::evaluateTargetFixup(const MCAssembler &Asm,
599-
const MCFixup &Fixup,
600-
const MCFragment *DF,
601-
const MCValue &Target,
602-
const MCSubtargetInfo *STI,
603-
uint64_t &Value, bool &WasForced) {
594+
bool RISCVAsmBackend::evaluateTargetFixup(
595+
const MCAssembler &Asm, const MCFixup &Fixup, const MCFragment *DF,
596+
const MCValue &Target, const MCSubtargetInfo *STI, uint64_t &Value) {
604597
const MCFixup *AUIPCFixup;
605598
const MCFragment *AUIPCDF;
606599
MCValue AUIPCTarget;
@@ -647,12 +640,7 @@ bool RISCVAsmBackend::evaluateTargetFixup(const MCAssembler &Asm,
647640
Value = Asm.getSymbolOffset(SA) + AUIPCTarget.getConstant();
648641
Value -= Asm.getFragmentOffset(*AUIPCDF) + AUIPCFixup->getOffset();
649642

650-
if (shouldForceRelocation(Asm, *AUIPCFixup, AUIPCTarget, STI)) {
651-
WasForced = true;
652-
return false;
653-
}
654-
655-
return true;
643+
return !shouldForceRelocation(Asm, *AUIPCFixup, AUIPCTarget, STI);
656644
}
657645

658646
bool RISCVAsmBackend::handleAddSubRelocations(const MCAssembler &Asm,

0 commit comments

Comments
 (0)