[PATCH] D54720: [PPC64] toc-indirect to toc-relative relaxation.

[Sean Fertile via Phabricator via llvm-commits]

sfertile added inline comments.


================
Comment at: ELF/Arch/PPC64.cpp:126
+
+  // The relocations are sorted by offset, and while the most common case is
+  // that every .toc entry will have a relocation, it is not guaranteed. For
----------------
MaskRay wrote:
> MaskRay wrote:
> > sfertile wrote:
> > > MaskRay wrote:
> > > > > The relocations are sorted by offset
> > > > 
> > > > IIRC this is not guaranteed but I think it is fine to lose toc optimization if the input does not have this property.
> > > > 
> > > > The linear search below `for (auto R : llvm::reverse(Relas.slice(0, Index))) {` concerns me. 
> > > > 
> > > > Is it possible to add a `LastRelaIndex` variable in `InputSectionBase::relocateAlloc`:
> > > > 
> > > >       if (!tryRelaxTocPPC64(Type, Rel, Expr, BufLoc, LastRelaIndex))
> > > > 
> > > > and keep it monotonically increasing in `getSymAndAddend`? If the input doesn't guarantee increasing `r_offset`, it may lose TOC optimizations for some relocations, but as I said it should be fine.
> > > >> The relocations are sorted by offset
> > > > IIRC this is not guaranteed
> > > I believe its guaranteed for binutils tools. We also sort the relocations for the .toc section in `scanRelocs` in case there are other tools which don't produce the relocations in sorted order.
> > > 
> > > > The linear search below for (auto R : llvm::reverse(Relas.slice(0, Index))) { concerns me
> > > I believe missing toc relocations to be an uncommon thing. As far as I am aware llvm will not place constants into the .toc section. gcc will for a few very specific cases but it is still not very common. xlc is the outlier here as I heard it will do this some what commonly despite there being no need to with the V2 abi. 
> > > 
> > > For the objects I found that were missing relocations when compiled with gcc , we either found the relocation we were looking for, or broke out of the search in a few comparisons. If a linear search does become a problem for some pathological inputs we could switch to a binary search but I think doing that initially we would end up pessimaling the common case for something that might not ever naturally occur.
> > I was not suggesting a binary search.
> > 
> > I suggested to change:
> > 
> > ```
> > for (const Relocation &Rel : Relocations) { // the loop is in relocateAlloc
> >   // Offset is increasing
> >   for (Index = computed(Rel); J >= 0; J--) // the loop is in PPC64.cpp getSymAndAddend
> >     if (Offset == Relas[Index].r_offset) {
> >       /* found */
> >       break;
> >     }
> > }
> > ```
> > 
> > to
> > 
> > ```
> > Index = 0
> > for (const Relocation &Rel : Relocations) { // the loop is in relocateAlloc
> >   // Offset is increasing
> >   for (; Index < Relas.size(); ++Index)
> >     // Index is increasing
> >     if (Offset == Relas[Index].r_offset) {
> >       /* found */
> >       break;
> >     }
> > }
> > ```
> Sorry, see below:
> 
> ```
> for (const Relocation &Rel : Relocations) { // the loop is in relocateAlloc
>   // Offset is increasing
>   for (Index = computed(Rel); Index >= 0; Index--) // the loop is in PPC64.cpp getSymAndAddend
>     if (Offset == Relas[Index].r_offset) {
>       /* found */
>       break;
>     }
> }
> ```
> 
> to
> 
> ```
> Index = 0
> for (const Relocation &Rel : Relocations) { // the loop is in relocateAlloc
>   // Offset is increasing
>   for (; Index < Relas.size(); ++Index)
>     // Index is increasing
>     if (Offset == Relas[Index].r_offset) {
>       /* found */
>       break;
>     } else if (Offset < Relas[Index].r_offset)
>       break;
> }
> ```
> 
I think I may have misunderstood which relocations you were referring too with the first comment about sort order. Only the relocations that operate on the .toc section are sorted. (Otherwise I'm misunderstanding this proposed change, perhaps we could talk on IRC to clairfy?)

Here is my understanding of the proposed look-up:

```
Index = 0
for (const Relocation &Rel : Relocations) { // the loop is in relocateAlloc
  // Offset is not (guranteed) increasing, Addend is not (typically) increasing.
  Rel.Sym    // This must be .toc if its an indirect.
  Rel.Addend // The addends will not be in any particular order. 
                         // When looking up the symbol we indirect too, we want to find the
                        //  relocation that relocates the toc section at offset `Rel.Addend`.
  Rel.Offset // The offset in the section being relocated. Unrelated to the lookup performed in the .toc section.

  // Inside the PPC64 target.
  // Loop over the relocations for the .toc section. These **are** sorted based on offset.

  for (; Index < Relas.size(); ++Index)
    // Index is increasing
    if (Offset == Relas[Index].r_offset) {
      /* found */
      break;
    } else if (Offset < Relas[Index].r_offset)
      break;
}
```

1) Any indirect that refers to an offset smaller then the first //missing// relocation would still be optimized as its array mapping is still correct.
2) Any offset strictly between the first //missing// relocation and the largest toc offset seen  so far will not be optimized as `Relas[Index].r_offset > Offset`
3) Any toc offset greater then or equal too the largest toc offset seen so far will be optimized (as long as it has a relocation). If the new offset is greater then the previous maximum offset, it increases the size of the window described in #2.



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54720