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



Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D54720