Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Properly test cross-language LTO #57438

Open
michaelwoerister opened this issue Jan 8, 2019 · 9 comments
Open

Properly test cross-language LTO #57438

michaelwoerister opened this issue Jan 8, 2019 · 9 comments
Labels
A-linkage Area: linking into static, shared libraries and binaries A-testsuite Area: The testsuite used to check the correctness of rustc E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.

Comments

@michaelwoerister
Copy link
Member

I'd like us to stabilize cross-language LTO as soon as possible but before we can do that we need more robust tests. Some background:

  • Cross-language LTO works by emitting LLVM bitcode instead of machine code for RLIBs/staticlibs and then lets the linker do the final ThinLTO pass. Because code coming from other languages (most notably C/C++) are also compiled to LLVM bitcode, the linker can perform inter-procedural optimizations across language boundaries.
  • For this to work properly, the LLVM versions used by rustc and the foreign language compilers must roughly be the same (at least the same major version).
  • The current implementation of cross-language LTO is available via -Zcross-lang-lto in rustc. It has some tests that make sure libraries indeed contain LLVM bitcode instead of object files. This is the bare-minimum requirement for cross-language LTO to work.

However, there is a problem with this approach:

  • Having LLVM bitcode available might not be sufficient for cross-language optimizations to actually happen. For example, LLVM will not inline one function into another if their target-features don't match. For this reason we unconditionally emit the target-cpu attribute for LLVM functions. For now this seems to be sufficient but for future versions of LLVM additional requirements might emerge and break cross-language LTO in a silent way (i.e. everything still compiles but the expected optimizations aren't happening).

So, to test the feature in a more robust way, we have to have a test that actually compiles some mixed Rust/C++ code and then verifies that function inlining across language boundaries was successful.

The good news is that trivial functions (e.g. ones that just return a constant integer) are reliably inlined, so we have a good way of checking whether inlining happens at all.

The bad news is that, since Rust's LLVM is ahead of stable Clang releases, we need to build our own Clang in order to get a compatible version. We already have Clang in tree because we need it for building Rust-enabled LLDB, but this is not the default.

So my current proposal for implementing these tests is:

  • Add a script (available to run-make-fulldeps tests) that checks if a compatible Clang is available.
  • Add an option to compiletest that allows to specify whether tests relying on Clang are optional or required.
  • If Clang-based tests are required, fail them if Clang is not available, otherwise just ignore them (so that local testing can be done without also building Clang).
  • Force building Rust LLDB on major platform CI jobs, thus making Clang available.
  • For those same jobs, set the compiletest option that requires Clang to be available.

This way cross-language LTO will be tested by CI and hopefully sccache will make building Clang+LLDB cheap enough. But we also rely on not forgetting to set the appropriate flags in CI images, which might cause things to silently go untested.

@rust-lang/infra & @rust-lang/compiler, do you have any thoughts? Or a better, less complicated approach? It would be great if we could require Clang unconditionally but I'm afraid that that would be too much of a burden for people running tests locally.

@michaelwoerister michaelwoerister added A-linkage Area: linking into static, shared libraries and binaries A-build T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jan 8, 2019
@alexcrichton
Copy link
Member

I think we'll definitely want to only run these tests on an opt-in basis rather than by default, and then we'll just want to carefully make sure that CI is running these tests. We can probably just pick a builder that's already finishing pretty quickly and build clang on it from source (which LLDB uses as well), and rely on sccache to make things fast.

@michaelwoerister
Copy link
Member Author

Thanks, @alexcrichton! I should have a PR soon.

bors added a commit that referenced this issue Jan 24, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 24, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 25, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 28, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 28, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 30, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
bors added a commit that referenced this issue Jan 31, 2019
compiletest: Support opt-in Clang-based run-make tests and use them for testing xLTO.

Some cross-language run-make tests need a Clang compiler that matches the LLVM version of `rustc`. Since such a compiler usually isn't available these tests (marked with the `needs-matching-clang`
directive) are ignored by default.

For some CI jobs we do need these tests to run unconditionally though. In order to support this a `--force-clang-based-tests` flag is added to compiletest. If this flag is specified, `compiletest` will fail if it can't detect an appropriate version of Clang.

@rust-lang/infra The PR doesn't yet enable the tests yet. Do you have any recommendation for which jobs to enable them?

cc #57438

r? @alexcrichton
@michaelwoerister
Copy link
Member Author

Implemented in #57514.

@Mark-Simulacrum
Copy link
Member

@michaelwoerister Should we perhaps reopen this to track re-enabling a few of the tests on Windows? I believe you mentioned it wasn't super important to have that testing... but that might've been in reference to short term rather than long term.

@michaelwoerister
Copy link
Member Author

It's important to test it on some platform. It is a little less important to test it on all platforms. But yes, let's re-open so we don't forget.

@jonas-schievink jonas-schievink added T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) and removed T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) A-build labels Apr 21, 2019
@michaelwoerister
Copy link
Member Author

@rust-lang/infra Now that we've switched to a new CI vendor, do we have the resources to test xLTO on all tier 1 platforms? That would be really nice!

@pietroalbini
Copy link
Member

We don't have 4 cores yet on Azure Pipelines, so we have even less capacity than before (a single build is now around 3h30m).

@michaelwoerister
Copy link
Member Author

OK, let's revisit at some later point in time then.

@jonas-schievink jonas-schievink added the A-testsuite Area: The testsuite used to check the correctness of rustc label Jan 13, 2020
@jyn514
Copy link
Member

jyn514 commented Feb 3, 2023

I think we have more spare capacity on Windows builders now than we have in the past; would it still be useful to test this on Windows?

If it hasn't failed since 2019 that maybe points to no ... but I think we should no longer need a fork of Clang, hopefully, so maybe we can test this without blowing up CI times too much?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-linkage Area: linking into static, shared libraries and binaries A-testsuite Area: The testsuite used to check the correctness of rustc E-help-wanted Call for participation: Help is requested to fix this issue. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants