[PATCH] D33922: Apply summary-based dead stripping to regular LTO modules with summaries.

[Teresa Johnson via Phabricator via llvm-commits]

tejohnson added inline comments.


================
Comment at: llvm/lib/Bitcode/Reader/BitcodeReader.cpp:5509
+  ModuleSummaryIndexBitcodeReader R(std::move(Stream), Strtab, CombinedIndex);
+  R.Mod = CombinedIndex.addModule(ModulePath, ModuleId);
   return R.parseModule();
----------------
pcc wrote:
> tejohnson wrote:
> > It wasn't clear to me when I first looked at this part of the patch why the ModuleIdentifier on the BitcodeModule wouldn't be used, as it is in getSummary below (or was here before). I think it would be clearer to pass in a flag indicating that this is a regularLTO summary, and then here either use the ModuleIdentifier or "", depending on the flag.
> I did it this way because I wanted LTO to handle regular LTO module summaries on its own (well, as much as possible I suppose). With that suggestion, we would move more of the handling into the bitcode reader, which I'm not entirely opposed to, but let me know what you think of that rationale.
Ok, but please document the fact that we are ignoring the ModuleIdentifier because of client-dependent behavior around the desired module path in the combined index.


================
Comment at: llvm/lib/LTO/LTO.cpp:509
+  } else {
+    return linkRegularLTO(std::move(*ModOrErr), /*LivenessFromIndex=*/false);
+  }
----------------
pcc wrote:
> tejohnson wrote:
> > Why not just link all of the regular LTO objects during runRegularLTO? I guess you still need to keep track of which had summaries though. How much of addRegularLTO could then be deferred until runRegularLTO (i.e. keeping track of the resolution info, like we do for ThinLTO, but delaying building the Module, computing Keep, etc)? It seems conceptually simpler to do all the linking in one place.
> I think the first thing I tried was something along those lines, but the code turned out to be less clean in my opinion, precisely because there was more state to keep track of. I guess it's just a matter of opinion, but this seems cleaner to me, as there turned out to be a relatively clean boundary between preparing to link a module and linking it.
Ok. But is this affected by the change being discussed in D34156, where my understanding is that all regular LTO modules will have summaries?


https://reviews.llvm.org/D33922