[PATCH] D23680: [ThinLTO] Emit files for distributed builds for all modules

[Teresa Johnson via llvm-commits]

tejohnson added a comment.

In https://reviews.llvm.org/D23680#519874, @mehdi_amini wrote:

> > They were no longer being emitted for files that didn't contain a module index (e.g. when they had inline assembly, since we can't currently safely import), because the new API only added them to the ThinLTO backend based on the presence of a module index. They were instead added as regular LTO files, and were all expected to be merged into a single LTO module. This is problematic because the distributed build system will not know how to create backend actions for those files.
>
>
> I do not necessarily see this as a "bug" right now: we need a coherent story for the intended behavior here. If a file is not meant to go to to the ThinLTO backend, then it shouldn't go there. It seems to me that the real issue you're tackling is that we don't emit any summary in the bitcode for files that have inline asm, and thus they are not detected as "ThinLTO" files and not sent to the right backend.
>  I believe the "right" fix for this is to change the way we detect if a file was produced with -flto=thin to detect this file (the "easy" way may be to emit an empty summary?).


Possible, but:

> It does not totally solve the question about how to do with inputs that were not build with -flto=thin but -flto=full. It does not seem right to me to send these to the ThinLTO backend. You can start by consider this as "unsupported" by your current distributed build system integration, and then built the mechanism that would allow the build system to deal with this if needed.


For now we can handle this unsupported case by treating all the IR modules (regardless of how they were produced) through the ThinLTO compilation model. I am happy to put this under a different option rather than keying off of thinlto-index-only if you'd prefer, would that be better?

> Note also that the `class LTO` is doing too much right now: if the `RegularLTOState` was moved to a separate entity in way that it could be injected like the ThinLTOBackend, all of this could be handled/overridden by the client (I mentioned this lack of layering in early reviews). The "ForceThinLTO" flag in the config seems to me to be a workaround for this.


That may be a better longer term change. In the meantime this is a simple way to reverse the regression (before the new API, the gold-plugin keyed off of the "thinlto" plugin option, not the presence of a module index section). I could put a FIXME there if that would help. Does that sound ok?


https://reviews.llvm.org/D23680