[PATCH] D96398: [LTO] Perform DSOLocal propagation in combined index

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

lgtm, thanks for the efficiency improvement! Some minor comments below.



================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:238
 // See internalizeGVsAfterImport.
 void ModuleSummaryIndex::propagateAttributes(
     const DenseSet<GlobalValue::GUID> &GUIDPreservedSymbols) {
----------------
I don't know that it is worth changing the name, but please add a note to the comment here and in the header that it is also propagating the IsDSOLocal flag.


================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:284
+      if (IsDSOLocal && !S->isDSOLocal())
+        IsDSOLocal = false;
     }
----------------
Super nit: would it be more efficient to just do:
   IsDSOLocal &= S->isDSOLocal();
i.e. without the if condition? Either way is probably ok though.


================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:295
   setWithAttributePropagation();
+  setWithDSOLocalPropagation();
   if (llvm::AreStatisticsEnabled())
----------------
We could presumably get by with one merged flag. Not sure it is worth it, in case we decide to decouple these at some point...I don't have a strong feeling so up to you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96398