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

Fri Feb 12 10:46:48 PST 2021

weiwang added inline comments.

================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:295
   setWithAttributePropagation();
+  setWithDSOLocalPropagation();
   if (llvm::AreStatisticsEnabled())
----------------
tejohnson wrote:
> weiwang wrote:
> > tejohnson wrote:
> > > tejohnson wrote:
> > > > 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.
> > > Actually, probably best to leave the flags decoupled. In case we eventually want to be able to disable them independently for debugging.
> > Sure, I'll leave them decoupled. Side question: I see that withAttributePropagation flag is preserved across bitcode file read/write. If the flag is set when index is materialized, does it mean the propagation was done previously on the index? If that's the case, can we skip it?
> Oh that's a good point, I forgot that gets serialized. Hmm, that is actually going to be important for distributed ThinLTO backends, where we serialize out the combined index, read it back in for each backend, and invoke the function importing from there. So please add that for the new flag as well. Looks like this test should fail without an update to the expected flags: llvm/test/Assembler/summary-flags.ll.
> 
> I don't think it is worth adding a check to this code as to whether it has been set already though. I don't see any way we will ever invoke this propagation on a combined index that was serialized out after invoking it already.
Thanks. I've updated the patch to include serialization and de-serialization of the flag. Since this is non-trivial, I'd like to get approval again.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96398