[PATCH] D128382: [LLD] Two tweaks to symbol ordering scheme

[YongKang Zhu via Phabricator via llvm-commits]

yozhu added a comment.

In D128382#3608798 <https://reviews.llvm.org/D128382#3608798>, @MaskRay wrote:

> In D128382#3606454 <https://reviews.llvm.org/D128382#3606454>, @yozhu wrote:
>
>>>>> In D128382#3603513 <https://reviews.llvm.org/D128382#3603513>, @MaskRay wrote:
>>>>>
>>>>>> How does it save one page?
>>>>>
>>>>> Hot code will always be placed together, so where it starts impact how many pages it will occupy.  Moving it towards the beginning of the output section increases the possibility that one less page will be taken.
>>>>
>>>> I am not sure this is true.
>>>> For -z separate-code layout, PT_LOAD program header has an aligned start address. I agree that placing hot code at the start may potentially remove one hot page.
>>>> For -z noseparate-code layout, I think we can construct a case that placing hot code at the start may use one more page.
>>>
>>> This is to follow what ordering implies.  For CISC machine target the linker won't do anything but just strictly follow the specified order.
>>
>> For the `-z noseparate-code` case, it would be the same probability for the impact on number of pages used by hot contributions. We don't know where in a page .text section will start, so it will be purely random or with equal probability that hot contribution may take one more page, one less page, or take same number of pages, between placing hot contributions at section start (like what the linker does for CISC machines) and shuffling some amount of cold contributions before hot ones.
>
> `-z noseparate-code is the default case. Since it is purely random, this patch does not justify its usefulness.
>
>> The current logic is to avoid generating branch thunks only.  So if we know branch thunk is no needed, why bother doing the extra shuffling working?
>
> The patch introduced some non-trivial complexity and therefore it needs to justify it. For non-code sections I agree that not shuffling would look better but that only suggests that `osec->flags & SHF_EXECINSTR` (osec needs to passed from the caller of `sortISDBySectionOrder`) is fine, not `executableInputSections > 1`.
>
>> And for the `-z separarte-code` case, it is guaranteed to be better (or being same in rare cases).
>
> Yes. With no measurement we can't accept an arbitrary change which claims to be better.

Curious how the previous change (D44969 <https://reviews.llvm.org/D44969>) got accepted without any measurement for the impact on the "no need of branch thunk but still do shuffling" scenario.  It makes sense when it implies higher probability of reducing number of required branch thunks, but sounds against what symbol ordering implies to user if no branch thunk is needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128382