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

[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



================
Comment at: llvm/lib/LTO/LTO.cpp:509
+  } else {
+    return linkRegularLTO(std::move(*ModOrErr), /*LivenessFromIndex=*/false);
+  }
----------------
pcc wrote:
> tejohnson wrote:
> > 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?
> After that change lands all LTO modules produced by clang will have summaries, so we will normally be following the new code path that defers the link step. But we will still need to be able to read modules without summaries for backwards compatibility reasons and in order to handle bitcode created by non-clang producers.
I suspect we may eventually want to reorganize this so that regularLTO operates more like ThinLTO (read the summary and symbols here only, read the module later during runRegularLTO). But let's go forward with this approach for now, since it is a smaller delta compared to the current approach.


https://reviews.llvm.org/D33922