This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Emit files for distributed builds for all modules
ClosedPublic

Authored by tejohnson on Aug 18 2016, 11:01 AM.

Details

Reviewers
mehdi_amini
Summary

With the new LTO API in r278338, we stopped emitting the individual
index files and imports files for some modules in the distributed backend
case (thinlto-index-only plugin option).

Specifically, this is when the linker decides not to include a module in the
link, because it was in an archive library and did not have a strong
reference to it. Not creating the expected output files makes the
distributed build system implementation more difficult, in terms of
checking for the expected outputs of the thin link, and scheduling the
backend jobs. To address this, the gold-plugin will write dummy empty
.thinlto.bc and .imports files for modules not included in the link
(which LTO never sees).

Augmented a gold v1.12+ test, since that version of gold has the handling
for notifying on modules not being included in the link.

Diff Detail

Event Timeline

tejohnson updated this revision to Diff 68572.Aug 18 2016, 11:01 AM
tejohnson retitled this revision from to [ThinLTO] Emit files for distributed builds for all modules.
tejohnson updated this object.
tejohnson added a reviewer: mehdi_amini.
tejohnson added subscribers: pcc, llvm-commits.
mehdi_amini added inline comments.Aug 18 2016, 11:11 AM
tools/llvm-lto2/llvm-lto2.cpp
147 ↗(On Diff #68572)

Looks like we stepped on each other feet (D23681)

mehdi_amini edited edge metadata.Aug 18 2016, 11:29 AM

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?).

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.

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.

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?

tejohnson updated this revision to Diff 71952.Sep 20 2016, 10:22 AM
tejohnson edited edge metadata.
Address feedback from earlier review:
1) Emit empty summaries for files without any GVs that can be imported
when in ThinLTO mode. This ensures that LTO treats them as ThinLTO
inputs and schedules ThinLTO backends for them. It required a few
fixes to handle the empty summaries.
2) Per offline discussion with Mehdi, for the case where the linker
is not going to include some of the modules in the final link, in
the gold-plugin client itself write the dummy empty files expected by the
distributed build system for all modules.
mehdi_amini added inline comments.Sep 20 2016, 10:35 AM
lib/Bitcode/Reader/BitcodeReader.cpp
6111

Can you commit this as a separate patch? This is mostly unrelated to the distributed build.

tejohnson added inline comments.Sep 20 2016, 10:37 AM
lib/Bitcode/Reader/BitcodeReader.cpp
6111

It is related to the change to always emit a summary when compiling for ThinLTO. Maybe I should split that portion (along with the corresponding fixes like this one) out into a separate patch. Is that what you meant? It fixes part of the distributed build issue.

pcc added inline comments.Sep 20 2016, 10:42 AM
tools/gold/gold-plugin.cpp
755

I am not sure if this is needed, but even if it is, can't this be done inside lib/LTO?

tejohnson added inline comments.Sep 20 2016, 10:45 AM
tools/gold/gold-plugin.cpp
755

This is needed because the distributed build system (in this case Bazel) wants to check that its expected outputs were generated. It doesn't know what the linker decided to include in the final link or not. This was a regression from the earlier behavior before we moved to the new LTO API.

My earlier version of the patch had this fix in lib/LTO, but Mehdi expressed a preference (in an offline discussion) to move this into the client.

mehdi_amini added inline comments.Sep 20 2016, 10:50 AM
lib/Bitcode/Reader/BitcodeReader.cpp
6111

It also change the behavior outside of distributed build right?
It will make files that we didn't include in ThinLTO before, part of the ThinLTO partitions now.

mehdi_amini added inline comments.Sep 20 2016, 10:51 AM
tools/gold/gold-plugin.cpp
755

Right: it seems "client specific" as it is a constraint that comes from Bazel.

tejohnson added inline comments.Sep 20 2016, 10:53 AM
lib/Bitcode/Reader/BitcodeReader.cpp
6111

Yes. Ok, will split that whole part out into a separate patch. This patch will end up just being the changes to gold-plugin.cc, test/tools/gold/X86/v1.12/thinlto_emit_linked_objects.ll, and the refactoring of getThinLTOOutputFile in LTO.

tools/gold/gold-plugin.cpp
755

Although note that Bazel is independent of gold. It just happens that we build with using the gold linker and the Bazel distributed build system.

pcc added inline comments.Sep 20 2016, 11:04 AM
tools/gold/gold-plugin.cpp
755

When you say "It doesn't know what the linker decided to include" are you referring to which archive members the linker chose?

So the regression is that the distributed build is missing non-ThinLTO objects from archives, and the fix is to create empty .thinlto.bc files to cause the build system to include the non-ThinLTO objects in the final link?

Or am I misunderstanding?

tejohnson added inline comments.Sep 20 2016, 11:13 AM
tools/gold/gold-plugin.cpp
755

Correct, it is to deal with the fact that with archives (--start-lib/--end-lib in our case), gold will not include some of the files in the final link. Those are not passed to the backend and no ThinLTO outputs are created for them.

We do emit the list of files to include in the final link (into the thinlto_linked_objects_file, which is an optional file provided to the thinlto-index-only= option), and that ends up informing the final link done by Bazel (essentially passed in via the @ linker option).

However, the issue here is with the way Bazel does sanity checking of expected outputs - all it knows when it kicks off the ThinLink action is that it is passing a set of object (IR) files to the Thin Link action, and each action needs to be told what output files it should bring back. The lower level job handling checks that these expected outputs are created and fetches them back. With the current LTO code, that contract is being broken, because not all expected outputs are being created.

mehdi_amini added inline comments.Sep 20 2016, 11:13 AM
tools/gold/gold-plugin.cpp
755

Although note that Bazel is independent of gold. It just happens that we build with using the gold linker and the Bazel distributed build system.

Right, I'm not convinced it belongs here entirely :)

My understanding is that it is very targeted: this operates under the assumptions that:

  1. The build system has a list of all the input to the linker (including archive members)
  2. The build system expects the linker to produces 1-1 output for each input.

So it is about both archive members not chosen by the linker, and input files that are not ThinLTO.
I don't think the latter case is handled though, it seems to be relying on every input being built with -flto=thin (or I missed a piece somewhere, how would an object file be handled?)

tejohnson added inline comments.Sep 20 2016, 11:17 AM
tools/gold/gold-plugin.cpp
755

So it is about both archive members not chosen by the linker, and input files that are not ThinLTO.
I don't think the latter case is handled though, it seems to be relying on every input being built with -flto=thin
(or I missed a piece somewhere, how would an object file be handled?)

We know which compile actions are ThinLTO actions and set up the expected inputs and outputs that way. In cases where there is e.g. a native object, there isn't a ThinLTO output scheduled to be returned from the ThinLink. So the part of this patch that always generates a summary when compiling -flto=thin fixes the issue where we were not treating these as ThinLTO inputs in the ThinLink.

For the archive members not included in the final link. the issue is that when we set up the ThinLink action, we know which inputs are ThinLTO inputs, and need to specify which outputs should be expected and brought back. That has no knowledge of the linker behavior and which inputs it will ultimately select.

mehdi_amini added inline comments.Sep 20 2016, 11:30 AM
tools/gold/gold-plugin.cpp
755

So if I have a project that depends on a vendor supplied library, it is up to the person configuring the project to instruct Bazel about the ThinLTO member inside this archive?

pcc added inline comments.Sep 20 2016, 11:31 AM
tools/gold/gold-plugin.cpp
755

I see. So my next question is, if Bazel knows the names of the expected output files, can it (or some other component of the system such as a wrapper script) not be taught to touch those files by itself before running the initial linker pass?

I can appreciate if the answer is "not easily". I'm just trying to get a sense of the right tradeoff here.

tejohnson added inline comments.Sep 20 2016, 11:52 AM
tools/gold/gold-plugin.cpp
755

So if I have a project that depends on a vendor supplied library, it is up to the person configuring the project to instruct Bazel about the ThinLTO member inside this archive?

There are a few limitations that would currently prevent this from working well with the build system right now. That use case would require some more work beyond this, at least to get the backend job distributed properly. We currently expect to compile all ThinLTO code from source.

tejohnson added inline comments.Sep 20 2016, 11:56 AM
tools/gold/gold-plugin.cpp
755

I can appreciate if the answer is "not easily". I'm just trying to get a sense of the right tradeoff here.

I don't think this is easy given the way things are architected. But it also prevents error checking that we did in fact run the thin link and get outputs.

tejohnson updated this revision to Diff 71973.Sep 20 2016, 1:10 PM

Moved the part of this patch that handles always emitting a summary
section for files compiled -flto=thin into a separate patch (D24779).

This part deals with ensuring the distributed build system gets back
expected outputs for the files it compiled in ThinLTO mode.

pcc, are you ok with keeping this here rather than in LTO? The advantage is that LTO doesn't need to know about files the linker decided to exclude, just those to be built with LTO.

mehdi_amini accepted this revision.Sep 21 2016, 10:07 AM
mehdi_amini edited edge metadata.

I'm fine with the patch as it is now.
Let's just wait for Peter to confirm.

This revision is now accepted and ready to land.Sep 21 2016, 10:07 AM
pcc added inline comments.Sep 21 2016, 12:11 PM
tools/gold/gold-plugin.cpp
755

But it also prevents error checking that we did in fact run the thin link and get outputs.

You could do that by reading the LinkedObjectsFile, no?

But okay, I suppose this is fine since it doesn't preclude a better implementation on the bazel side later.

tejohnson added inline comments.Sep 21 2016, 12:17 PM
tools/gold/gold-plugin.cpp
755

You could do that by reading the LinkedObjectsFile, no?

That's difficult since where the outputs are checked is low level generic job management functionality, and not cognizant of the fact that this is ThinLTO (I looked at this initially).

But okay, I suppose this is fine since it doesn't preclude a better implementation on the bazel side later.

Ok thanks.

tejohnson updated this object.Sep 21 2016, 12:20 PM
tejohnson edited edge metadata.