[PATCH] D60958: [PPC64] toc-indirect to toc-relative relaxation

[Sean Fertile via Phabricator via llvm-commits]

sfertile added a comment.

Patch looks really good. I think you managed to explain the toc-indirection without getting to verbose which is something I've struggled with. I have a couple minor suggestions and  I need to go over the test a bit more indepth but overall this looks good to me.



================
Comment at: ELF/Arch/PPC64.cpp:122
+  uint64_t Index = Offset / 8;
+  if (Index >= Relas.size())
+    return {};
----------------
I think if Relas.size() is not equal to zero, then we want to set `Index` to index the last element of the array, and perform the linear probing. For example trying to relax an access to last element of a .toc section that contains a constant as the first entry and a symbol as the second: Index will be 1, but Relas[0] is the relocation we want to find to perform the relaxation.


================
Comment at: ELF/Arch/PPC64.cpp:139
+//
+//   addis 3, 2, foo at toc@ha  # R_PPC64_TOC16_HA
+//   ld    3, foo at toc@l(3)   # R_PPC64_TOC16_LO_DS, load the address from a .toc entry
----------------
I think we need to make a small change to this comment. The compiler generates the indirection by adding a private label at the address of the toc entry for the symbol, and the `@toc` symbol modifier indicates to the assembler to create a toc-relative relocation for that private label. If we relax then  we rewrite the instructions to access the symbol toc-relative:


```
// .toc
// ...
// .LC0:
//  .tc foo               # will allocate 8 bytes storage and a R_PPC64_ADDR64 relocation for 'foo'


//  addis 3, 2, LC0 at toc@ha    # R_PPC64_TOC16_HA
//  ld 3, LC0 at toc@l(3)        # R_PPC64_TOC16_LO_DS, load the address of 'foo' from the .toc entry
//  ld/lwa  3, 0(3)           # load the value of foo


// If foo is defined, non-preemptable and addressable with a 32-bit signed
// offset from the toc base, the address can be computed by adding an offset to
// the toc base, saving a load.

//  addis 3, 2, foo at toc@ha ...
````


================
Comment at: ELF/Arch/PPC64.cpp:355
+  case R_PPC64_TOC16_HA:
+    // Convert "addis reg, 2, foo at toc@h" to "addis reg, 2, toc_relative_ha" or
+    // "nop".
----------------
Need to make similar adjustments to this comment and the next.


================
Comment at: ELF/InputSection.cpp:73
+  // Spurious false for InputSection::Discarded as Config isn't available.
+  AreRelocsRela = Config && Config->IsRela;
+
----------------
Is this related or is it from a different patch?


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D60958