[PATCH] D96398: [LTO] Pre-populate IsDSOLocal result in combined index

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added a comment.

In D96398#2554344 <https://reviews.llvm.org/D96398#2554344>, @weiwang wrote:

> The overhead of repeatedly scanning all summaries could be quite high in the current ValueInfo::isDSOLocal implementation, especially in large projects. This change helped reduce overall thinLTO link time of a major internal workload by 20 mins.

Great! I have some suggestions to improve the approach further.



================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:157
+  /// invalidated when SummaryList or any associated summary is updated.
+  Optional<bool> IsDSOLocal;
 };
----------------
Instead of adding the memory overhead of an Optional per GUID in the index, can you just have the populate method turn into a propagation method, and just reset all IsDSOLocal flags if any are false? Then the isDSOLocal method only needs to check the first summary entry. You can add a global flag to the index that indicates that the propagation has been done, and assert that it has been set when isDSOLocal() is called. See for example the global set when we do dead code analysis and attribute propagation (WithGlobalValueDeadStripping and WithAttributePropagation).

Actually, for the most efficiency, I wonder if you can just merge this into propagateAttributes (and use a single global flag set on the index when it has been propagated). Like propagateAttributes, you presumably don't need to do this on anything that is dead, and it wouldn't need to be invoked if importing is disabled. 


================
Comment at: llvm/lib/LTO/LTO.cpp:894
+          // Invalidate cached result.
+          if (auto *I = ThinLTO.CombinedIndex.findSummaryInfo(GUID))
+            I->IsDSOLocal.reset();
----------------
This adds a second hash table lookup for GUID. Better to do the new call to findSummaryInfo(GUID) first, then there is another findSummaryInModule that takes a ValueInfo that can be used instead of the GUID based one used a few lines above.

But I'm not sure I understand why this is needed in the first place (ditto for the reset invoked when adding a global value summary) - since the populate method is not called until we have the full combined index.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D96398/new/

https://reviews.llvm.org/D96398