Skip to content

Commit

Permalink
[ThinLTO] Refine reachability check to fix compile time increase
Browse files Browse the repository at this point in the history
Summary:
A recent fix to the ThinLTO whole program dead code elimination (D56117)
increased the thin link time on a large MSAN'ed binary by 2x.
It's likely that the time increased elsewhere, but was more noticeable
here since it was already large and ended up timing out.

That change made it so we would repeatedly scan all copies of linkonce
symbols for liveness every time they were encountered during the graph
traversal. This was needed since we only mark one copy of an aliasee as
live when we encounter a live alias. This patch fixes the issue in a
more efficient manner by simply proactively visiting the aliasee (thus
marking all copies live) when we encounter a live alias.

Two notes: One, this requires a hash table lookup (finding the aliasee
summary in the index based on aliasee GUID). However, the impact of this
seems to be small compared to the original pre-D56117 thin link time. It
could be addressed if we keep the aliasee ValueInfo in the alias summary
instead of the aliasee GUID, which I am exploring in a separate patch.

Second, we only populate the aliasee GUID field when reading summaries
from bitcode (whether we are reading individual summaries and merging on
the fly to form the compiled index, or reading in a serialized combined
index). Thankfully, that's currently the only way we can get to this
code as we don't yet support reading summaries from LLVM assembly
directly into a tool that performs the thin link (they must be converted
to bitcode first). I added a FIXME, however I have the fix under test
already. The easiest fix is to simply populate this field always, which
isn't hard, but more likely the change I am exploring to store the
ValueInfo instead as described above will subsume this. I don't want to
hold up the regression fix for this though.

Reviewers: trentxintong

Subscribers: mehdi_amini, inglorion, dexonsmith, llvm-commits

Differential Revision: https://reviews.llvm.org/D57203

llvm-svn: 352438
  • Loading branch information
teresajohnson committed Jan 28, 2019
1 parent a36a293 commit 5b2f6a1
Showing 1 changed file with 17 additions and 8 deletions.
25 changes: 17 additions & 8 deletions llvm/lib/Transforms/IPO/FunctionImport.cpp
Expand Up @@ -777,9 +777,7 @@ void llvm::computeDeadSymbols(
if (!VI)
return;

// We need to make sure all variants of the symbol are scanned, alias can
// make one (but not all) alive.
if (llvm::all_of(VI.getSummaryList(),
if (llvm::any_of(VI.getSummaryList(),
[](const std::unique_ptr<llvm::GlobalValueSummary> &S) {
return S->isLive();
}))
Expand Down Expand Up @@ -819,12 +817,23 @@ void llvm::computeDeadSymbols(
while (!Worklist.empty()) {
auto VI = Worklist.pop_back_val();
for (auto &Summary : VI.getSummaryList()) {
GlobalValueSummary *Base = Summary->getBaseObject();
// Set base value live in case it is an alias.
Base->setLive(true);
for (auto Ref : Base->refs())
if (auto *AS = dyn_cast<AliasSummary>(Summary.get())) {
// If this is an alias, visit the aliasee VI to ensure that all copies
// are marked live and it is added to the worklist for further
// processing of its references.
// FIXME: The aliasee GUID is only populated in the summary when we
// read them from bitcode, which is currently the only way we can
// get here (we don't yet support reading the summary index directly
// from LLVM assembly code in tools that can perform a thin link).
// If that ever changes, the below call to getAliaseGUID will assert.
visit(Index.getValueInfo(AS->getAliaseeGUID()));
continue;
}

Summary->setLive(true);
for (auto Ref : Summary->refs())
visit(Ref);
if (auto *FS = dyn_cast<FunctionSummary>(Base))
if (auto *FS = dyn_cast<FunctionSummary>(Summary.get()))
for (auto Call : FS->calls())
visit(Call.first);
}
Expand Down

0 comments on commit 5b2f6a1

Please sign in to comment.