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

[Sean Fertile via Phabricator via llvm-commits]

sfertile 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;
----------------
MaskRay wrote:
> 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.
The savings mostly come from making the 4 loops we have now (and the 2 that need to be added later for the fpr save/restore routines) nearly identical. We can then write a helper function or lambda and call that 6 time instead of having 6 loops once we add support for  `_savefpr*` and `_restfpr*` as well. With a minor tweak I believe  the helper would also work for the vector save restore routines as well.

Something like:

```
Helper(uint32_t *Buffer, uint32_t BufferSize, StringRef Prefix) {
  int r = findFirstOf(prefix);  // returns the register number corresponding to the first symbol we need to define.
  // Any referenced symbols are defined, nothing to do.
  if (r == 32)
  return;

  int Value = 0;
  const int Offset = 4 * (r - 14);
  for (r < 32; ++r) {
   Name = FormatName(prefix, r)
    addOptional(Name, Value, defined);
    Value += 4;
  }
  // No need to check if defined is empty becuase we would have returned already if no symbols need to be defiend.
  addSection(Buffer + Offset, Buffer + Size, defined);
}


```

Its longer and more verbose then one of the for loops, but I think its much easier to follow the logic, and then  it only takes 6 lines like:

```
Helper(savegpr0, ArraySize(savegpr0), "__savegpr0_");
```

Its more lines of code total, since defining the arrays adds more lines then we save with the replacing the for loops with the helper but I think its much more approachable for a reader.


================
Comment at: lld/ELF/Arch/PPC64.cpp:152
+      sym->section = sec;
+      sym->value -= 4 * (first - begin);
+    }
----------------
MaskRay wrote:
> 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.
I think this approach is simpler in terms of less lines of code, but not necessarily simpler to read or understand. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79977