[PATCH] D51089: [LLD] [COFF] Add support for creating range extension thunks for ARM

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added a comment.

In https://reviews.llvm.org/D51089#1231980, @ruiu wrote:

> Sorry for the belated response. I was thinking of this patch for a while.
>
> Every time I saw the code of thunk range extension, I wonder if we really need this multi-pass algorithm which add thunks iterative on each pass. I believe in almost all cases, the algorithm finishes on the first iteration, if we allow a very small margin when determining "reachability". As long as a margin is small, size increase by allowing a margin should be negligible.


I actually thought about that before, but either didn't think it through properly or didn't feel it was necessary - but by adding that in this current design I can make it succeed after the first pass already (previously it required two passes adding thunks on my testcase).

> For pathetic executables for which we need to generate tons of thunks (which enlarges distance between callers and callees and thus need multiple passes with the current algorithm), we can simply discard everything that we made in the previous iteration instead of keeping them, double the margin, and then try again from scratch. In practice, I believe that fallback doesn't happen too frequently.
> 
> What do you think of the algorithm? If it works, I prefer that algorithm because discarding everything and redo with a larger margin is simpler than keeping thunks created in previous passes.

The approach you describe feels a bit fragile. Unless you're really sure the margin tradeoff is right and it will be done on the first pass in real-world cases, it'll degrade pretty badly.

I was going to try to give this an honest and objective try, but I don't feel it will be much simpler. For your suggestion, we would need to have two kinds of loops over the chunks - one loop which checks whether thunks are needed with margin, adding them as necessary, and a second loop which checks if all relocations now are in range (not trying to add any thunks, but just aborting the loop, then resetting everything back to the original state and starting over. While the current code (which also is very similar to the corresponding ELF thunk code) does verification at the same time as runs a new pass trying to add more thunks if needed. If no more were needed, the algorithm was done.



================
Comment at: COFF/Chunks.cpp:658
+// A Thumb2, PIC, non-interworking range extension thunk.
+const uint8_t RangeExtensionThunkARMData[] = {
+    0x40, 0xf2, 0x00, 0x0c, // P:  movw ip,:lower16:S - (P + (L1-P) + 4)
----------------
ruiu wrote:
> This variable name seems a bit too long to my taste; I'd name `ArmThunk` or something like that, and that should be fine as long as this is a file-scope variable.
Sure, I'll shorten it.


================
Comment at: COFF/Chunks.cpp:824-831
   for (SectionChunk *C : Sections) {
     if (!C->isLive())
       continue;
     size_t Off = Builder.getOffset(toStringRef(C->getContents()));
     C->setOutputSection(Out);
     C->setRVA(RVA + Off);
     C->OutputSectionOff = OutputSectionOff + Off;
----------------
ruiu wrote:
> This part is not guarded by `Finalized` -- is that intended?
Yes - this is called from assignAddresses on each relayout, to propagate the current location in the layout.


================
Comment at: COFF/PDB.cpp:775
          "debug sections should not be in output sections");
+  DebugChunk->readRelocTargets();
   DebugChunk->writeTo(Buffer);
----------------
ruiu wrote:
> If you change this line to
> 
>   cast<SectionChunk>(DebugChunk)->readRelocTargets();
> 
> then can you make `readRelocTargets` a non-virtual member function that belongs to `SectionChunk`?
That also requires changes like `if (SectionChunk *SC = dyn_cast_or_null<SectionChunk>(C))` in readRelocTargets() in Writer.cpp. Or we move calling that to somewhere else, but then it's probably not quite as easy to parallelize.


https://reviews.llvm.org/D51089