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

[Fangrui Song via Phabricator via llvm-commits]

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


================
Comment at: ELF/Arch/PPC64.cpp:122
+  uint64_t Index = Offset / 8;
+  if (Index >= Relas.size())
+    return {};
----------------
sfertile wrote:
> 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.
My bad. The test `ppc64-toc-relax-constants.s` had demonstrated the failure. Fixed.


================
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
----------------
sfertile wrote:
> 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 ...
> ````
Reworded.


================
Comment at: ELF/InputSection.cpp:73
+  // Spurious false for InputSection::Discarded as Config isn't available.
+  AreRelocsRela = Config && Config->IsRela;
+
----------------
sfertile wrote:
> Is this related or is it from a different patch?
It is related to an assertion failure when `relas()` is called with zero relocations. 
```
  template <class ELFT> ArrayRef<typename ELFT::Rela> relas() const {
    assert(AreRelocsRela); // If relas is empty(), this is false
    return llvm::makeArrayRef(
        static_cast<const typename ELFT::Rela *>(FirstRelocation),
        NumRelocations);
  }
```

I have switched to early return if `TocReloc->NumRelocations == 0`. The assertion will not fire.



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D60958