[PATCH] D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS.
Fangrui Song via Phabricator via llvm-commits
llvm-commits at lists.llvm.org
Wed Jan 15 18:15:39 PST 2020
MaskRay added inline comments.
================
Comment at: lld/ELF/Relocations.cpp:1775
+ // in size.
+ if (isdBase + target->getThunkSectionSpacing() < os->size &&
+ isdLimit - isdBase > 4096) {
----------------
MaskRay wrote:
> Can we simplify `isdBase + target->getThunkSectionSpacing() < os->size` to `off + target->getThunkSectionSpacing() < os->size`
We can't.
`off + target->getThunkSectionSpacing() < os->size` can cause https://bugs.llvm.org//show_bug.cgi?id=44071#c5 to fail with `ld.lld: error: thunk creation not converged`.
I suggested `off` because I thought `off` corresponds to `prevIsecLimit` in the code below:
```lang=cpp
if (isecLimit > thunkUpperBound) {
addThunkSection(os, isd, prevIsecLimit);
thunkUpperBound = prevIsecLimit + thunkSectionSpacing;
}
```
Still puzzled why we use `isdBase + target->getThunkSectionSpacing() < os->size` here.
================
Comment at: lld/ELF/Relocations.cpp:1781
+ // rounding the size up.
+ ts->alignment = 4;
+ ts->roundUpSizeForErrata = true;
----------------
I wonder whether we should use 4 unconditionally https://reviews.llvm.org/D72819 .
Most thunks are 16 bytes. I don't know why we selected 8, instead of 4 or 16 in D29129.
(If 8 were for locality/cache line considerations, 16 would be a better choice.)
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72344/new/
https://reviews.llvm.org/D72344
More information about the llvm-commits
mailing list