Page MenuHomePhabricator

Please use GitHub pull requests for new patches. Phabricator shutdown timeline

[BOLT][RISCV] Add support for linker relaxation
ClosedPublic

Authored by jobnoorman on Tue, Aug 29, 6:17 AM.

Details

Summary

Calls on RISC-V are typically compiled to auipc/jalr pairs to allow
a maximum target range (32-bit pc-relative). In order to optimize calls
to near targets, linker relaxation may replace those pairs with, for
example, single jal instructions.

To allow BOLT to freely reassign function addresses in relaxed binaries,
this patch proposes the following approach:

  • Expand all relaxed calls back to auipc/jalr;
  • Rely on JITLink to relax those back to shorter forms where possible.

This is implemented by detecting all possible call instructions and
replacing them with PseudoCALL (or PseudoTAIL) instructions. The
RISC-V backend then expands those and adds the necessary relocations for
relaxation.

Since BOLT generally ignores pseudo instruction, this patch makes
MCPlusBuilder::isPseudo virtual so that RISCVMCPlusBuilder can
override it to exclude PseudoCALL and PseudoTAIL.

To ensure JITLink knows about the correct section addresses while
relaxing, reassignment of addresses has been moved to a post-allocation
pass. Note that this is probably the time it had to be done in the
first place since in notifyResolved (where it was done before), all
symbols are supposed to be resolved already.

Depends on D159082

Diff Detail

Event Timeline

jobnoorman created this revision.Tue, Aug 29, 6:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 29, 6:17 AM
jobnoorman requested review of this revision.Tue, Aug 29, 6:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Aug 29, 6:17 AM

Note that since this relies on a fix in lld (D159082), this patch will probably cause CI problems again (since the system's clang is used there).

Amir added a comment.Wed, Sep 13, 12:30 PM

Note that since this relies on a fix in lld (D159082), this patch will probably cause CI problems again (since the system's clang is used there).

Pre-merge checks here and in GitHub PRs use trunk lld [1], so if you replace clang with lld in test then it should pass.

[1]: https://buildkite.com/llvm-project/phabricator-run-tests/builds/173788#018a4173-32e6-4d74-9871-6c0bff371c9c/117-120

bolt/test/RISCV/relax.s
2

Use llvm-mc+ld.lld instead of clang.

jobnoorman marked an inline comment as done.Thu, Sep 14, 12:20 AM

Note that since this relies on a fix in lld (D159082), this patch will probably cause CI problems again (since the system's clang is used there).

Pre-merge checks here and in GitHub PRs use trunk lld [1], so if you replace clang with lld in test then it should pass.

Thanks @Amir, I completely missed that.

Thanks for the patch, Job! Do you know what's the effect of changing the reassignSectionAddress() invocation on other platforms? Everything else LGTM.

Thanks for the patch, Job! Do you know what's the effect of changing the reassignSectionAddress() invocation on other platforms? Everything else LGTM.

It shouldn't have any effect on other platforms. The change will call reassignSectionAddress() a bit earlier in the JITLink pipeline (notifyResolved is called right after running the PostAllocationPasses, see this) but this shouldn't matter from BOLT's perspective.

Just to be sure, I'll run llvm-bolt-wrapper in the morning to verify (I'm pretty sure I already did this but it has been a while).

Awesome. Let me also check this change internally just in case.

maksfb accepted this revision.Fri, Sep 15, 1:33 AM
This revision is now accepted and ready to land.Fri, Sep 15, 1:33 AM
This revision was automatically updated to reflect the committed changes.