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

Thu Feb 11 17:49:40 PST 2021

tejohnson added inline comments.

================
Comment at: llvm/lib/IR/ModuleSummaryIndex.cpp:295
   setWithAttributePropagation();
+  setWithDSOLocalPropagation();
   if (llvm::AreStatisticsEnabled())
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96398