[PATCH] D61629: [ELF] Try placing newly created ThunkSection at the end of the InputSection first

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

In D61629#1493114 <https://reviews.llvm.org/D61629#1493114>, @MaskRay wrote:

> In D61629#1493047 <https://reviews.llvm.org/D61629#1493047>, @peter.smith wrote:
>
> > I don't think it will do any harm, but it is probably not going to make a lot of difference. The thunk code makes the assumption that there is one branch range for the thunk section spacing, the vast majority will get placed in these sections. The no suitable ThunkSection case is for the small number of edge cases that don't fit the model, for example in Thumb there is one very rare branch that has a much shorter range so it may need to use this, but there aren't enough of them to make reuse opportunities that likely.
>
>
> `createInitialThunkSections()` looks to me a very good global heuristic. Without it, `Relocations.cpp:1669 getISDThunkSec` is making local decisions that may not be very good. I think it this way. There are three positions to create the thunk:
>
> - `IS->OutSecOff`: current status
> - `IS->OutSecOff + IS->getSize()`: this patch. It improves sharing slightly.
> - The last InputSection whose start address is before `IS->OutputSecOff + getThunkSectionSpacing()`. This is the best place to insert the thunk. (This is heuristically achieved by the proactive `createInitialThunkSections()`.
>
>   If you think the marginal benefit is not bad to have, I'll update the tests:)


I don't have any objections, just wanted to save you the work of updating the tests!

In an ideal world we would have something similar to IS->OutputSecOff + getThunkSectionSpacing() although it would need some modifications. For example the last case of  `IS->OutputSecOff + getThunkSectionSpacing()` in its current form would not work for Thumb2. The getThunkSectionSpacing() uses a singler per architecture value, which is 16 Megabytes for Thumb2 representing the branch range of the unconditional branches, the (much rarer) conditional branch instruction has a range of 1 Megabyte. We would need a more sophisticated implementation that took into account the specific branch range of the type of the relocation when calculating the ThunkSectionSpacing.


Repository:
  rLLD LLVM Linker

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

https://reviews.llvm.org/D61629