Page MenuHomePhabricator

[RISCV][LLD] Add RISCV zcmt optimise in linker relaxation
Needs ReviewPublic

Authored by VincentWu on Sep 25 2022, 4:27 AM.

Details

Summary

This patch implements optimizations for the zcmt extension in lld.

A new TableJumpSectio has been added.

Scans each R_RISCV_CALL/R_RISCV_CALL_PLT relocType in each section before the linker relaxation, recording the name of the symbol.

In finalizeContents the recorded symbol names are sorted in descending order by the number of jumps.

Optimise and insert a new cm.jt/cm.jalt during the relax process. in the process, we reused the`R_RISCV_JAL` relocType

co-author: @ScottEgerton

Diff Detail

Event Timeline

VincentWu created this revision.Sep 25 2022, 4:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 4:27 AM
VincentWu requested review of this revision.Sep 25 2022, 4:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2022, 4:27 AM
kito-cheng added a comment.EditedSep 26 2022, 4:14 AM

I think this should really write a psABI spec proposal first.

lld/ELF/Arch/RISCV.cpp
617

This relaxation type remove 6 byte, so this should has higher priority than R_RISCV_CALL/R_RISCV_CALL_PLT to jal?

lld/ELF/SyntheticSections.cpp
1165

Should be SHT_PROGBITS, SHT_RISCV_ATTRIBUTES is only used for .riscv.attribute.

1188–1192

This should be enough.

1273–1287

Hmm, how do you deal with more than one local symbol with same name?

e.g.
Following example has 2 different bar:

foo1.c

static int bar (){return 1;}

int foo1(){return bar();}

foo2.c

static int bar (){return 2;}

int foo2(){return bar();}

main.c

int foo1();
int foo2();
int main(){return foo1()+foo2();}
lld/ELF/SyntheticSections.h
379

addCMJTEntryCandidate/getaddCMJTEntryIndex, and same for addEntryZero/getEntryZero

402

entriesZero -> CMJTEntryCandidates
finalizedEntriesZero -> finalizedCMJTEntries
and same for entriesRa/finalizedEntriesRa.

We only need to record symbol name for finalEntries, so std::vector<std::string> is enough?

409

CMJTEntry/CMJALTEntry rather than Zero/Ra, e.g. maxCMJTEntrySize/maxCMJALTEntrySize, startCMJTEntryIdx/startCMJALTEntryIdx.

lld/ELF/Writer.cpp
457

newlib's implementation using __tbljalvec_base$? and maybe use STB_GLOBAL like __global_pointer$?

https://github.com/linsinan1995/riscv-glibc/commit/5d2e0376316f64acfc047dc3e29fd266a456fb8f

VincentWu updated this revision to Diff 462882.Sep 26 2022, 6:29 AM
VincentWu marked 6 inline comments as done.

address part of comments

lld/ELF/SyntheticSections.cpp
1237

We only need to record symbol name for finalEntries, so std::vector<std::string> is enough?

yep but we need sort it in descending order by the number of jumps.
it will be spand more linking time to copy again.

so I chose memory space instead of time.

did you mean it is acceptable if i spend more time to copy it again?

sinan added a subscriber: sinan.Sep 27 2022, 1:37 AM
sinan added inline comments.
lld/ELF/SyntheticSections.cpp
1165
craig.topper added inline comments.Sep 30 2022, 10:31 AM
lld/ELF/Options.td
321

RISCV-> RISC-V

MaskRay added inline comments.Oct 1 2022, 12:03 AM
lld/ELF/Options.td
320

FF: we require that new options must start with --, not -

lld/ELF/SyntheticSections.cpp
1164

The implementation should be moved to Arch/RISCV.cpp to avoid redundant virtual functions in TargetInfo.

lld/ELF/SyntheticSections.h
31

This is inefficient. If DenseMap<CachedStringRef, ...> works, prefer it.

We have the RISC-V attributes section, you should use that (and --(no-)relax) to determine whether or not to use Zcmt relaxation, not invent some new flag, especially since compiler drivers can't use it unless they're sure the linker supports that new flag (think Clang + binutils as the common case where it's hard to know for sure; at least with LLD the likelihood is it's not older than the Clang version, especially in the embedded space) or you require users to opt-in at link time to the relaxation.

jrtc27 added inline comments.Oct 1 2022, 11:46 AM
lld/ELF/Arch/RISCV.cpp
377

This seems wrong

813

This seems like poor design, you shouldn't be using R_RISCV_JAL once it's no longer a JAL instruction there

lld/ELF/SyntheticSections.cpp
1213

When moving this to RISCV.cpp please use extractBits

lld/ELF/Writer.cpp
1637

Why isn't this done when we read in the relocations the first time?

2859

Huh? This doesn't make any sense? You're just writing the jump table section on top of every SHF_ALLOC output section? Make it act like every single other synthetic section and end up in an output section.