Repository navigation
Add unstable -Zllvm-tpde flag to rustc_codegen_llvm - #164108
TechnoPorg wants to merge 15 commits into
Conversation
|
This PR changes how LLVM is built. Consider updating src/bootstrap/download-ci-llvm-stamp. This PR modifies If appropriate, please update This PR modifies If appropriate, please update
cc @bjorn3 The rustc-dev-guide subtree was changed. If your future PRs only touch the subtree, consider submitting them directly to rust-lang/rustc-dev-guide, which is where the document is primarily maintained (and has faster CI).
|
This comment has been minimized.
This comment has been minimized.
|
A few notes for the review:
|
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
Awesome. I'm currently traveling with limited data, but I'll be back Monday evening and have a look afterwards. |
|
Just a quick high-level pass, I'll look closer at the code Thursday/Friday.
I'd warn on higher levels than 1. It's likely to fail, and even if it succeeds, unlikely to provide a significant speedup. A warning should prevent test users from opening issues about that. That being said, I wouldn't error to not block experimenting, maybe someone might surprise us with positive results.
We can deal with that when it happens. Experimental nightly features are experimental, and using them incorrectly (=on unsupported features like asm) is expected to cause an ICE or similar. No need to delay or make the first PR bigger just to prevent that, especially if it needs upstream changes; those should go in follow-up PRs. :)
#goals/proposed > ✔ goals#791: Propose a project goal for a TPDE backend @ 💬 Can you also add a page under the rustc-dev-guide as part of this PR, mirroring the autodiff/offload side? Mainly listing briefly why we have it, how to enable (trivial), what to expect in the current state, and what features are known to not work? The 489 failures can be triaged later, but the 29 try failures are more interesting, and few enough that we should try to solve them first. Is it just because of some debug printing, or could it be a corner case where your new LLVMCodegenPM uses some undesired default values that we'd otherwise overwrite? |
There was a problem hiding this comment.
That's not nearly as invasive in our codegen backend as I was afraid it might be. :)
Still it would probably be good for @nikic to take a good look.
| if sess.opts.unstable_opts.tpde.is_some() { | ||
| sess.dcx().fatal("-Ztpde is LLVM-specific and not supported by Cranelift"); | ||
| } |
There was a problem hiding this comment.
So every out-of-tree backend has to remember to check for this? That does not scale.
There was a problem hiding this comment.
Isn't that what we're already doing, e.g. with instrument-coverage @RalfJung?
Imho that's just the small price out-of-tree backends pay? This one should also be a graceful failure since it would turn into a no-op, not a misscompilation.
We can do the frontend ad-hock name-check for llvm ourselves. It feels less maintainable to me, but should work. It's afaik just not what we usually do.
There was a problem hiding this comment.
Hm I wasn't involved in reviewing those other flags so I didn't know we already have such checks. I feel like we need a proper way for backends to indicate whether they support certain things that doesn't default to "just ignore the flag".
We could have an enum BackendFeature and then a backend_feature_suppoted method on the codegen backend trait?
There was a problem hiding this comment.
Yes, having a central method which based on the requested feature returns any of (supported|ignore|warn|error) sounds good. Backends could then overwrite the features they support, and fall through otherwise.
llvm-args seem to get remapped to cranelift/gcc flags.
instrument-coverage is ignored in gcc, errors in cranelift
-Clto is supported in gcc, error's in cranelift
Then there's the thin_lto_supported option in CodegenBackendInit as the one thing we handle explicitly.
Every other llvm specific flag seems to be unhandled and ignored in both cranelift and gcc.
Either way, I'd prefer not having such unrelated change as part of this PR. @TechnoPorg can you open an issue or directly a PR so that we can move the discussion out of this PR?
There was a problem hiding this comment.
Opened #164533 and removed the validation function for now
There was a problem hiding this comment.
This doesn't currently differentiate between opt levels. Should I make it error out on an opt level greater than 1?
I agree to warn (code might be slower due to more vector operations).
The logging is not very configurable right now: it's currently always on stdout, and the level "ON" or "OFF" is only able to be set at build time, so I will be sending some patches upstream to improve this.
Disable logging for now. We ultimately want to have a custom logging infrastructure in TPDE (essentially a function_ref), which TPDE-LLVM or the tpde-plugin.so configures to redirect errors to LLVMContext diagnostics.
58db8f6 to
6e64be1
Compare
This comment has been minimized.
This comment has been minimized.
-Ztpde flag to rustc_codegen_llvm-Zllvm-tpde flag to rustc_codegen_llvm
|
@rustbot author (still working on CMake build stuff, hopefully done by the end of my UTC-4 day) |
This comment has been minimized.
This comment has been minimized.
34d9105 to
0a028c2
Compare
This comment has been minimized.
This comment has been minimized.
|
I've been running experiments with crater-at-home to gather data on which common instructions are unsupported by TPDE yet (for which I have opened tpde2/tpde#49 upstream). This gives some interesting numbers on how well TPDE supports building1 "normal" Rust code. I saw no outright build failures due to TPDE (there were some unrelated issues with nightly features). -Copt-level=0Of the top 1000 most downloaded crates, 419 fall back to LLVM. -Copt-level=1NOTE: I'm pretty sure I botched the data collection here so I will update this once I've re-run. Of the top 1000 most downloaded crates, 172 fall back to LLVM. -Copt-level=3NOTE: I'm pretty sure I botched the data collection here so I will update this once I've re-run. Although this isn't a recommended mode of operation, I was curious about numbers. Of the top 300 most downloaded crates, 19 fall back to LLVM. Footnotes
|
| std::ptr::null() | ||
| }; | ||
|
|
||
| let tpde = TpdeWrapper::get_instance(); |
There was a problem hiding this comment.
get_instance goes through a mutex, which is then dropped at the end of this function, right?
write_output_file should be called by multiple workers, so you'd serialize this section, even if we don't want to use tpde. The Mutex wasn't much of an issue in the original Enzyme version since Enzyme requires fat-lto (and thus a single core) anyway, but I don't think we need it here?
There was a problem hiding this comment.
Once resolved we should also do a perf run, to make sure that we don't affect the llvm codegen backend. I've seen a few surprising regressions in the past.
There was a problem hiding this comment.
This should be ready for a perf run now
|
Some changes occurred in src/tools/compiletest cc @jieyouxu |
This comment has been minimized.
This comment has been minimized.
|
CI failure is the same as #164921 (comment) and will hopefully be fixed by #164929. |
Something something why I became a Rust developer
4168143 to
f6f50dc
Compare
|
This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed. Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers. |
View all comments
Part of rust-lang/goals#802
This adds an unstable
-Z llvm-tpdeflag to rustc to opt into using the TPDE-LLVM codegen backend instead of LLVM itself. It takes several possible values:rustc_codegen_llvmoperates as before.-Zllvm-tpdeor-Zllvm-tpde=try, it attempts to compile modules with TPDE, but falls back to LLVM if TPDE fails-Zllvm-tpde=only, it attempts to compile modules with TPDE, and bails if TPDE fails (this is intended for development purposes)This PR does not attempt to optimize TPDE's performance in any way. Previous experiments have run basic optimization passes even on -O0, which helped with wall time, or maintained a list of incompatible IR elements which cause the TPDE path to be skipped. I plan on adding those later.
Stats
-Zllvm-tpde=only-Zllvm-tpde=tryMost of the failing UI tests are to do with assembly, sanitizers, SIMD, and other things we already know TPDE doesn't support. I haven't triaged all the failures yet (not sure if this would be a good use of time at this point).
When compiling ripgrep with
-Zllvm-tpde=try, I get a 3-4% wall time improvement, but a 17% icount improvement. I'm less confident in my measurement of the latter metric. 12 out of 1113 CGUs have to fall back to LLVM, with the rest using TPDE. All ripgrep unit tests pass.Blocked on #163731
@rustbot blocked
r? ZuseZ4
An LLM was consulted on how to use
perfto measure instruction counts.