Page MenuHomePhabricator

[LLD][ELF] Add DefinedInThunk to SymbolBody to remove need for hash lookup
AbandonedPublic

Authored by peter.smith on Jul 14 2017, 6:06 AM.

Details

Reviewers
ruiu
rafael
Summary

When doing thunk creation, if a relocation target is defined in a thunk we need to find a way of getting to the Thunk object. This change removes the
DenseMap in ThunkCreator that stored the mapping in preference for storing a pointer to the Thunk in SymbolBody. This saves a hash lookup at the expense of adding a pointer to SymbolBody.

This was suggested in D34692

Diff Detail

Event Timeline

peter.smith created this revision.Jul 14 2017, 6:06 AM
ruiu added inline comments.Jul 14 2017, 2:07 PM
ELF/Symbols.h
93–97

Please add blank lines above and below this member variable.

DefinedInThunk sounds like a boolean variable, so I'd give a different name, but obviously Thunk doesn't work. Do you have any suggestion?

Thanks for the comments. I've updated the diff and renamed to LabeledThunk which I think makes sense as the Symbol labels the Thunk.

Rebased patch, no other changes.

The use of the hash table and the proposed new member is so that, for
example, two arm relocations to a thumb function use a single thunk,
correct?

If so the hash table is only needed during the one pass when we are
mutating relocations to point to the thunk.

If the above is correct, a hash table seems better IMHO since it is only
used in a very specific point in the code.

Cheers,
Rafael

Yes it is only used within createThunks(). This review was created in response to a review comment in one of the previous range-thunks patches (still in review) https://reviews.llvm.org/D34692#809065 expressing a preference for avoiding the hash table lookup. Personally I prefer the hash table lookup as well but I've not got a strong preference. My main concern is getting the range-thunks functionality in.

peter.smith abandoned this revision.Dec 15 2017, 3:19 AM

I don't think that there was consensus over whether this change was a good idea. I'm abandoning it for now, it can be resurrected if needed.