[PATCH] D134600: [RISCV][LLD] Add RISCV zcmt optimise in linker relaxation

[Kito Cheng via Phabricator via llvm-commits]

kito-cheng added a comment.

I think this should really write a psABI spec first.



================
Comment at: lld/ELF/Arch/RISCV.cpp:617
+              0xa002 | (tblEntryIndex << 2)); // cm.jt or cm.jalt
+        remove = 6;
+      }
----------------
This relaxation type remove 6 byte, so this should has higher priority than `R_RISCV_CALL`/`R_RISCV_CALL_PLT` to `jal`?


================
Comment at: lld/ELF/SyntheticSections.cpp:1165
+TableJumpSection::TableJumpSection()
+    : SyntheticSection(SHF_ALLOC | SHF_WRITE, SHT_RISCV_ATTRIBUTES,
+                       config->wordsize, ".tbljalentries") {}
----------------
Should be SHT_PROGBITS, SHT_RISCV_ATTRIBUTES is only used for `.riscv.attribute`.


================
Comment at: lld/ELF/SyntheticSections.cpp:1188-1192
+  if (entriesList.count(symbol.getName().str()) == 0) {
+    entriesList[symbol.getName().str()] = 1;
+  } else {
+    entriesList[symbol.getName().str()] += 1;
+  }
----------------
This should be enough. 


================
Comment at: lld/ELF/SyntheticSections.cpp:1273-1287
+    uint8_t *buf, std::vector<std::pair<std::string, int>> &entriesList) {
+  for (const auto &symbolName : entriesList) {
+    // Use the symbol from in.symTab to ensure we have the final adjusted
+    // symbol.
+    for (const auto &symbol : in.symTab->getSymbols()) {
+      if (symbol.sym->getName() != symbolName.first)
+        continue;
----------------
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();}
```


================
Comment at: lld/ELF/SyntheticSections.h:379
+  void addEntryRa(const Symbol &symbol);
+  int getEntryRa(const Symbol &symbol);
+  void scanTableJumpEntrys(const InputSection &sec) const;
----------------
`addCMJTEntryCandidate`/`getaddCMJTEntryIndex`, and same for addEntryZero/getEntryZero


================
Comment at: lld/ELF/SyntheticSections.h:402
+  std::map<std::string, int> entriesRa;
+  std::vector<std::pair<std::string, int>> finalizedEntriesRa;
+
----------------
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?


================
Comment at: lld/ELF/SyntheticSections.h:409
+  const size_t startZero = 0;
+  const size_t startRa = 64;
+};
----------------
CMJTEntry/CMJALTEntry rather than Zero/Ra, e.g. maxCMJTEntrySize/maxCMJALTEntrySize, startCMJTEntryIdx/startCMJALTEntryIdx.


================
Comment at: lld/ELF/Writer.cpp:457
+    symtab->addSymbol(Defined{
+        /*file=*/nullptr, ".tbljalentries", STB_WEAK, STT_NOTYPE, STT_NOTYPE,
+        /*value=*/0, /*size=*/0, in.riscvTableJumpSection.get()});
----------------
newlib's implementation using `__tbljalvec_base$`? and maybe use STB_GLOBAL like `__global_pointer$`?

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


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D134600/new/

https://reviews.llvm.org/D134600