This is an archive of the discontinued LLVM Phabricator instance.

[ELF] Refactor uses of getInputSections to improve efficiency NFC
ClosedPublic

Authored by andrewng on Jan 20 2020, 7:47 AM.

Details

Summary

Add new method getFirstInputSection and use instead of getInputSections
where appropriate to avoid creation of an unneeded vector of input
sections.

Diff Detail

Event Timeline

andrewng created this revision.Jan 20 2020, 7:47 AM

Noticed this again whilst working on D72775. Appears to have a minor performance benefit of ~2% for the same two links with stack sizes mentioned in D72775.

Not got any objections to the refactoring, I'm not sure we need to share the loop traversal between getInputSections and getFirstInputSections (see comment inline) but I'm willing to go with the consensus on that.

lld/ELF/OutputSections.cpp
478

It would definitely be worth adding a comment, explaining that f must return true to continue and false to early terminate. I didn't find it obvious until reading the loop body and the two examples.

I thought about trying a different name (there is an identically named helper in Relocations.cpp that does something similar) but I couldn't come up with anything succinct. The best I could come up with was forEachInputSectionDescriptionUntilFalse().

485

Given that we only have 2 clients of forEachInputSectionDescription and no nested loops , it might be worth having two simpler functions that have their own loop

InputSection *getFirstInputSection(const OutputSection *os) {
  for (BaseCommand *base : os->sectionCommands)
    if (auto *isd = dyn_cast<InputSectionDescription>(base))
      if (!isd->sections.empty())
        return isd->sections.front();
  return nullptr;
}

getInputSections can remain the same as before.

grimar added inline comments.Jan 21 2020, 2:07 AM
lld/ELF/OutputSections.cpp
478

StringRef has take_while and take_until
It could be forEachInputSectionDescriptionDoWhile probably.
But see below.

485

I agree. No need to introduce forEachInputSectionDescription probably.

andrewng updated this revision to Diff 239263.Jan 21 2020, 2:49 AM

Updated patch to address review comments.

grimar accepted this revision.Jan 21 2020, 3:15 AM

LGTM

This revision is now accepted and ready to land.Jan 21 2020, 3:15 AM

LGTM too. Thanks for the update.

This revision was automatically updated to reflect the committed changes.