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

[Wei Wang via Phabricator via llvm-commits]

weiwang added inline comments.


================
Comment at: llvm/include/llvm/IR/ModuleSummaryIndex.h:157
+  /// invalidated when SummaryList or any associated summary is updated.
+  Optional<bool> IsDSOLocal;
 };
----------------
tejohnson wrote:
> 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. 
This is a really great suggestion, Thanks! 

Memory overhead was one concern I had. I'll see if I can do something similar as propagateAttributes. One question though, when you say "reset all IsDSOLocal flags if any are false. Then the isDSOLocal method only needs to check the first summary entry", do you mean if the flag in one of the summaries is false, then set all others to false as well?


================
Comment at: llvm/lib/LTO/LTO.cpp:894
+          // Invalidate cached result.
+          if (auto *I = ThinLTO.CombinedIndex.findSummaryInfo(GUID))
+            I->IsDSOLocal.reset();
----------------
tejohnson wrote:
> 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.
This is more of a bookkeeping to make sure the populated value is up-to-date. But with the propagation approach you suggested, I think this is no longer needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96398