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

[Wenlei He via Phabricator via llvm-commits]

wenlei 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:
> weiwang wrote:
> > 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?
> Yes - I suggested that since isDSOLocal() currently returns true only if all IsDSOLocal flags are true, and false otherwise. So it seems like you would just propagate the flags across all summaries in the list such that if any are false, all become false (or leave them all true if all are true) - then you get the same behavior by simply querying the first summary in the list's IsDSOLocal flag.
Thanks for the suggestion, @tejohnson. That'd be a smart and clean way of doing it. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96398