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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay 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
----------------
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;
    }
}
```


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54720