[PATCH] D146776: [llvm] Preliminary fat-lto-objects support

Thu Apr 27 12:54:39 PDT 2023

nikic added a comment.

In D146776#4302238 <https://reviews.llvm.org/D146776#4302238>, @tejohnson wrote:

> In D146776#4301320 <https://reviews.llvm.org/D146776#4301320>, @nikic wrote:
>
>> - The (currently disabled) MemProfiler pass. This looks like a plain bug to me: What is an instrumentation pass doing in the simplification pipeline? I'm pretty sure this should be in the optimization pipeline instead. Even if not, we could easily explicitly run this one, because it's at the very end of the simplification pipeline.
>
> I added this. As with other profile instrumentation passes it is off by default. I think I added it here because this is also where we add the PGO instrumentation passes, although I suppose there are specific reasons for doing those here (i.e. inlining is also invoked here). I can move it out so this becomes a non-issue.

Right. I'd expect the pipeline position here to be based on whether the instrumentation needs to be pre-inline or post-inline. The pre-inline (non-CS) PGO instrumentation runs during simplification, the post-inline CS PGO instrumentation is part of optimization.

>> - A single GlobalDCE pass at the end of the pipeline. I'm pretty sure this is another bug, because the comment says "Remove dead code, except in the ThinLTO pre-link pipeline where we may want to keep available_externally functions", even though available_externally functions only become relevant post-link, not pre-link.
>
> ThinLTO importing is not the only producer of available_externally functions. There can be available_externally functions coming out of clang, and we want them to survive until after importing and the post LTO link inlining is done.

Good point. The justification still seems somewhat dubious to me, because GlobalDCE will only remove available_externally functions if they aren't referenced. Referenced available_externally functions are only dropped by EliminateAvailableExternally, which we don't run pre-link. It's not obvious to me why we would want to keep around unreferenced available_externally functions pre-link even in a ThinLTO scenario.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D146776