[PATCH] D52156: [LLD] [COFF] Alternative ARM range thunk algorithm

[Martin Storsjö via Phabricator via llvm-commits]

mstorsjo added inline comments.


================
Comment at: COFF/Writer.cpp:385
+static std::pair<Defined *, bool>
+getThunk(DenseMap<uint64_t, Defined *> &LastThunks, Defined *Target, uint64_t P,
+         uint16_t Type, int Margin) {
----------------
ruiu wrote:
> mstorsjo wrote:
> > ruiu wrote:
> > > The relationship between section and its thunk is 1:N. I believe that's why you are using a map to manage thunk -> symbol mapping.
> > > 
> > > However, I think you visit sections in the increasing address order, so if the last thunk is not reachable, no other thunks are reachable. Can you use the fact?
> > > The relationship between section and its thunk is 1:N. I believe that's why you are using a map to manage thunk -> symbol mapping.
> > 
> > For every target Symbol, there can be N thunks that point to it, yes.
> > 
> > > However, I think you visit sections in the increasing address order, so if the last thunk is not reachable, no other thunks are reachable. Can you use the fact?
> > 
> > Yes - I'm using exactly this fact. This map allows me to look up the last thunk pointing to each specific RVA. (In the previous version, I had something similar to `DenseMap<uint64_t, vector<Defined *>>`, to allow finding all thunks for a specific target. But as you say, as we progress linearly we only need to keep track of the last thunk for each target.)
> I wonder if you can eliminate map lookups (which is slow). Since the relocation is 1:N, you can add a `Defined *` to a symbol to represent the relationship, no?
Oh, yes, that should also work, I think. And it seems like sizeof(Defined) is significantly less than sizeof(SymbolUnion), so it shouldn't grow the total memory consumption.

But in that case, on restarts, we need to iterate over all symbols in order to reset it. That's probably fine performance-wise since that's not expected to happen in real life - but is that even possible? SymbolTable only contains global symbols right?

If that's not possible, we'd need to store `Defined *` and the pass number it was added in.


https://reviews.llvm.org/D52156