-
Notifications
You must be signed in to change notification settings - Fork 14.7k
[RISCV] Track Linker Relaxable through Assembly Relaxation #152602
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 7 commits
20f900d
fcc304e
439939e
a7f2d1e
f99df26
f27ddf3
f66017f
7f28a2f
751cb18
a489820
2a7d435
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -819,6 +819,41 @@ void RISCVAsmBackend::maybeAddVendorReloc(const MCFragment &F, | |
Asm->getWriter().recordRelocation(F, VendorFixup, VendorTarget, VendorValue); | ||
} | ||
|
||
static bool fixupGetsRelaxRelocation(const MCFixupKind Kind) { | ||
switch (Kind) { | ||
default: | ||
break; | ||
case RISCV::fixup_riscv_lo12_i: | ||
case RISCV::fixup_riscv_lo12_s: | ||
case RISCV::fixup_riscv_hi20: | ||
case RISCV::fixup_riscv_pcrel_lo12_i: | ||
case RISCV::fixup_riscv_pcrel_lo12_s: | ||
case RISCV::fixup_riscv_pcrel_hi20: | ||
case RISCV::fixup_riscv_call_plt: | ||
case RISCV::fixup_riscv_qc_abs20_u: | ||
case RISCV::fixup_riscv_qc_e_32: | ||
case RISCV::fixup_riscv_qc_e_call_plt: | ||
case ELF::R_RISCV_TPREL_LO12_I: | ||
case ELF::R_RISCV_TPREL_LO12_S: | ||
case ELF::R_RISCV_TPREL_HI20: | ||
case ELF::R_RISCV_TPREL_ADD: | ||
return true; | ||
} | ||
return false; | ||
} | ||
|
||
void RISCVAsmBackend::maybeAddRelaxReloc(const MCFragment &F, | ||
const MCFixup &Fixup) { | ||
if (!Fixup.isLinkerRelaxable() || !fixupGetsRelaxRelocation(Fixup.getKind())) | ||
return; | ||
|
||
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); | ||
} | ||
|
||
bool RISCVAsmBackend::addReloc(const MCFragment &F, const MCFixup &Fixup, | ||
const MCValue &Target, uint64_t &FixedValue, | ||
bool IsResolved) { | ||
|
@@ -861,25 +896,24 @@ 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. | ||
if (Fixup.isLinkerRelaxable() && fixupGetsRelaxRelocation(Fixup.getKind())) | ||
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); | ||
// Some Fixups may get a RELAX relocation, record it (directly) after we | ||
// add the relocation. | ||
maybeAddRelaxReloc(F, Fixup); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The outlined function seems unnecessary. The previous open coding seems clearer. // If linker relaxation is enabled and supported by the current fixup, then we
// always want to generate a relocation.
bool needsRelax = Fixup.isLinkerRelaxable() && fixupGetsRelaxRelocation(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
// add the relocation.
maybeAddVendorReloc(F, Fixup);
Asm->getWriter().recordRelocation(F, Fixup, Target, FixedValue);
if (needsRelax) {
...
}
} There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
} | ||
|
||
return false; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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`. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. RISCVAsmBackend::applyFixup (make it clearer) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
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; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,79 @@ | ||
# RUN: llvm-mc --triple=riscv32 -mattr=+relax,+experimental-xqcilb,+experimental-xqcibi \ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Suggest that we don't use pr... in the test filename. Use linker-relaxation-xqcibi.s or xqcibi-linker-relaxation.s We can specify "## PR150071" as a comment. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
# RUN: %s -filetype=obj -o - -riscv-add-build-attributes \ | ||
# RUN: | llvm-objdump -dr -M no-aliases - \ | ||
# RUN: | FileCheck %s | ||
|
||
.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 0x12 <short_jump_over_fixed> | ||
# CHECK-NOT: R_RISCV_RVC_JUMP | ||
# 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, 0x24 <mid_jump_over_fixed> | ||
# CHECK-NOT: R_RISCV_JAL | ||
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does any fixup need special treatment? check-llvm-mc still passes if fixupGetsRelaxRelocation returns false.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if we don't have this, we'll end up with R_RISCV_RELAX on compressed instructions which are in-range but require a relocation (i.e. reference something close but the other side of something relaxable).I'll make a testcase for this tomorrow.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added more to the linker-relaxation-pr150071.s test which will fail if this either always returns
true
orfalse
.The problem is about the over-approximation of
RelaxCandidate = true
in the MCCodeEmitter. Now, many more relocations are marked as linkerrelaxable, which is good, but those might be pc-relative resolvable, like a jump over non-relaxable data. In these cases we want to allow the fixup itself to resolve if possible (these will only be left if they are in range, when xqcilb is enabled).And then we want to make sure, if a fixup is not resolved, that only fixups expecting R_RISCV_RELAX get that relocation, even if the fixup is marked linker relaxable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Encoding the complement set is clearer.
We are essentially listing short-range fixups that suppress RELAX relocations. Perhaps a function name like
fixupSuppressRelax
is clearer.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done. Renamed to
relaxableFixupNeedsRelocation
which was clearer to me thanfixupSuppressRelax
.