[PATCH] D72344: [LLD][ELF][ARM][AArch64] Only round up ThunkSection Size when large OS.

[Fangrui Song via Phabricator via llvm-commits]

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