[PATCH] D79977: [ELF][PPC64] Synthesize _savegpr[01]_{14..31} and _restgpr[01]_{14..31}

[Fangrui Song via Phabricator via llvm-commits]

MaskRay marked 5 inline comments as done.
MaskRay added inline comments.


================
Comment at: lld/ELF/Arch/PPC64.cpp:133
+void elf::addPPC64SaveRestore() {
+  static uint32_t savegpr0[20], restgpr0[21], savegpr1[19], restgpr1[19];
+  constexpr uint32_t blr = 0x4e800020, mtlr_0 = 0x7c0803a6;
----------------
sfertile wrote:
> Since we are always writing the entire routine into the buffer anyway why not just hard code the array contents so we don't have to compute it at runtime?
We need to define symbols `_savegpr*` and `_restgpr*`, so a loop is needed. There is little saving if we move instructions alone to a constant array.


================
Comment at: lld/ELF/Arch/PPC64.cpp:143
+  auto addSection = [&](uint32_t *begin, uint32_t *end) {
+    if (defined.empty())
+      return;
----------------
sfertile wrote:
> real minor nit: I think the code would be a bit clearer if this check cake outside the lamda: eg
> 
> ```
> if (!defined.empty())
>   addSection(...)
> ```
Mostly there are 4 callers. Placing it here saves some code.


================
Comment at: lld/ELF/Arch/PPC64.cpp:152
+      sym->section = sec;
+      sym->value -= 4 * (first - begin);
+    }
----------------
sfertile wrote:
> Does it clutter the code too much to keep track of the correct value in the loop that calls `addOptional' and not have to adjust this later?
Yes, the alternative approach will clutter the code. It will require 2 loops, with one finding the start point and the second placing instructions and definitions. This post adjustment approach is the simplest I can find.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79977