This is an archive of the discontinued LLVM Phabricator instance.

Create a SymbolBody for locals
ClosedPublic

Authored by rafael on Mar 10 2016, 6:25 AM.

Details

Reviewers
ruiu

Diff Detail

Event Timeline

rafael updated this revision to Diff 50271.Mar 10 2016, 6:25 AM
rafael retitled this revision from to Create a SymbolBody for locals.
rafael updated this object.
rafael added a reviewer: ruiu.
rafael added a subscriber: llvm-commits.
ruiu edited edge metadata.Mar 10 2016, 1:02 PM

So a natural question to ask is whether LocalSymbol is an appropriate class or not. Conceptually, being local is orthogonal to the type. There can be an undefined local symbol (and I think some ABI like PPC actually has that for some special use case.) What do you think?

I is probably orthogonal, but having it in place is very handy to
change one thing at a time.

Note that each patch in the series makes the codebase smaller.
Unifying LocalSymbol is what I want to try in patch 4, which will
hopefully delete even more lines :-)

Cheers,
Rafael

ruiu added inline comments.Mar 10 2016, 3:26 PM
ELF/InputFiles.h
112–115

For consistency with getSymbols, I'd name getLocalSymbols and getNonLocalSymbols.

ruiu added a comment.Mar 10 2016, 7:12 PM

LGTM

I build three more programs with and without the series of patches for local symbols (and in addition to that with GNU gold) and observed that the patch series improved performance in all three cases.

Time is in milliseconds.

Clang (RelWithDebugInfo)

w/o patch  13730
w/patch    13224
gold       15795

Clang (RelWithDebugInfo without -f{function,data}-sections)

w/o patch  13300
w/patch    12790
gold       15185

gold in binutils 2.26

w/o patch  542
w/patch    501
gold       726

(The gap between our linker and gold is getting narrower, but that's not a topic of this patch.)

It cannot describe why there is a case that this patch had a negative impact on performance, but I confirmed that the your test result is correct using the archive file you sent to me. So there must be some hidden condition that this approach is not very good at it. But broadly speaking this seems a good one. This makes things much simpler and seems to improve performance in general.

I hoped you did not change the signature of getSymbol in this patch because that change made this patch a lot larger. You could have done that in a different patch. However, since I have already reviewed all the changes, this overall looks good to me.

ruiu accepted this revision.Mar 10 2016, 7:13 PM
ruiu edited edge metadata.
This revision is now accepted and ready to land.Mar 10 2016, 7:13 PM
Eugene.Zelenko added a subscriber: Eugene.Zelenko.

Committed in r263222.