[PATCH] D57203: [ThinLTO] Refine reachability check to fix compile time increase

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson created this revision.
tejohnson added a reviewer: trentxintong.
Herald added subscribers: dexonsmith, inglorion, mehdi_amini.

A recent fix to the ThinLTO whole program dead code elimination (D56117 <https://reviews.llvm.org/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.


Repository:
  rL LLVM

https://reviews.llvm.org/D57203

Files:
  lib/Transforms/IPO/FunctionImport.cpp


Index: lib/Transforms/IPO/FunctionImport.cpp
===================================================================
--- lib/Transforms/IPO/FunctionImport.cpp
+++ lib/Transforms/IPO/FunctionImport.cpp
@@ -777,13 +777,9 @@
     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(),
-                     [](const std::unique_ptr<llvm::GlobalValueSummary> &S) {
-                       return S->isLive();
-                     }))
-      return;
+    for (auto &S : VI.getSummaryList())
+      if (S->isLive())
+        return;
 
     // We only keep live symbols that are known to be non-prevailing if any are
     // available_externally, linkonceodr, weakodr. Those symbols are discarded
@@ -819,12 +815,23 @@
   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);
     }


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D57203.183430.patch
Type: text/x-patch
Size: 2156 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20190125/7acbf64c/attachment.bin>