Improve Session/CodegenBackend construction - #161432
nnethercote wants to merge 3 commits into
Conversation
|
cc @rust-lang/miri
cc @bjorn3
|
|
This is an opinionated change, see what you all think. LLM disclosure: some of the ideas came from an analysis done by an LLM. I wrote all the code and text myself. |
| fn fmt(&self, formatter: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { | ||
| self.info.lock().expect("lock").fmt(formatter) | ||
| } | ||
| #[derive(Clone)] |
There was a problem hiding this comment.
Do the cg_gcc changes need to be done in this PR?
I would be more confortable landing this directly in the cg_gcc repo so that the whole test suite can run (some cg_gcc tests do not run here in the Rust repo).
There was a problem hiding this comment.
I think they do, because both commits change the signature of CodegenBackend::init. Doing a local test run in cg_gcc is probably the way forward, if/when there's agreement that this PR is worth merging.
This comment has been minimized.
This comment has been minimized.
There was a problem hiding this comment.
FWIW there is some more codegen-backend-related session initialization happening inside add_configuration. And especially the handing of target features is a complete mess (not as bad as it used to be, but still bad). We're calling llvm_util::global_llvm_features like half a dozen times because we need it in various places and we don't have a tcx yet so it can't be a query...
Anyway, not really something for this PR. I just wondered what this PR does with the messy part of codegen backend initialization that I regularly run into, and the answer is "nothing". Which is fine, the cleanup here seems reasonable on its own. Maybe inspiration for a future cleanup PR. :)
There was a problem hiding this comment.
Interesting. I looked into add_configuration and found a bug; #161718 fixes it and takes a step towards cleaning things up more. Once that PR merges I will do more in this PR to fix the remaining ordering problems.
I also looked at global_llvm_features. There is a query for it, global_backend_features, but it's not actually necessary. It should be possible to get the features once and store them in the session, which should make things simpler. Not sure yet if I will do that in this PR or a follow-up.
There was a problem hiding this comment.
I have successfully removed the global_backend_features query. #161903 needs to merge first.
Currently, `parse_cfg` calls `build_configuration`, which calls `default_configuration`, which calls `sess.target.singlethread(&sess.internal_target_features)`. But `sess.internal_target_features` hasn't been set at this point and is empty! This commit moves the setting of `sess.internal_target_features` before the `parse_cfg` call to fix this ordering bug. This results in the `cfg(target_has_threads)` being correctly set on `wasm32-unknown-unknown` when `-Ctarget-feature=+atomics` is specified. Note: I have plans to make this kind of ordering bug difficult/impossible in a follow-up (e.g. rust-lang#161432).
`llvm::target_config` creates `target_machine` by calling `create_informational_target_machine`, which calls `target_machine_factory`, which uses `internal_target_features`. But this is just before `internal_target_features` is initialized! So we should move `internal_target_features` initialization before `target_machine`, right? But `internal_target_features` initialization involves a closure that inspects `target_machine`. There is a cyclic dependency. There is enough function nesting here that it's hard to spot. In practice this cycle doesn't cause problems because the closure doesn't inspect the parts of `target_machine` that depend on `internal_target_features`. But it demonstrates how startup initialization is all tangled up, and it's blocking some cleanups I am doing in rust-lang#161432 relating to the dangerous uses of `Session` before it's fully initialized. Therefore, this commit changes the first part: instead of creating an `OwnedTargetMachine` we create an `OwnedMCSubtargetInfo`. This is a smaller type that has the feature information we need but doesn't depend on `internal_target_features`. Under the covers we are now using LLVM's `Target::createMCSubtargetInfo` instead of `TargetMachine::getMCSubtargetInfo` so that we avoid having to create a `TargetMachine` at this early stage. This eliminates the cycle. (`TargetMachine` can still be created later on, once we're past this fraught initialization.) There are some slight differences between these two approaches, and the preceding commits fixed up some issues there. Some details about this commit: - The new `OwnedMCSubtargetInfo` is similar to the existing `OwnedTargetMachine`. - `create_informational_target_machine` no longer needs a `for_cfg` parameter, because the one site where `for_cfg` was true has been removed. - `LLVMRustCreateMCSubtargetInfo` mostly replicates part of `LLVMRustCreateTargetMachine` - `LLVMRustMCSubtargetInfoHasFeature` partly replicates `LLVMRustHasFeature`. - `LLVMRustHasFeature` is no longer needed. - The error message for `custom-target-invalid-llvm-target.rs` changed.
`llvm::target_config` creates `target_machine` by calling `create_informational_target_machine`, which calls `target_machine_factory`, which uses `internal_target_features`. But this is just before `internal_target_features` is initialized! So we should move `internal_target_features` initialization before `target_machine`, right? But `internal_target_features` initialization involves a closure that inspects `target_machine`. There is a cyclic dependency. There is enough function nesting here that it's hard to spot. In practice this cycle doesn't cause problems because the closure doesn't inspect the parts of `target_machine` that depend on `internal_target_features`. But it demonstrates how startup initialization is all tangled up, and it's blocking some cleanups I am doing in rust-lang#161432 relating to the dangerous uses of `Session` before it's fully initialized. Therefore, this commit changes the first part: instead of creating and `OwnedTargetMachine` we create an `OwnedMCSubtargetInfo`. This is a smaller type that has the feature information we need but doesn't depend on `internal_target_features`. Under the covers we are now using LLVM's `Target::createMCSubtargetInfo` instead of `TargetMachine::getMCSubtargetInfo` so that we avoid having to create a `TargetMachine` at this early stage. This eliminates the cycle. (`TargetMachine` can still be created later on, once we're past this fraught initialization.) There are some slight differences between these two approaches, and the preceding commits fixed up some issues there. Some details about this commit: - The new `OwnedMCSubtargetInfo` is similar to the existing `OwnedTargetMachine`. - `create_informational_target_machine` no longer needs a `for_cfg` parameter, because the one site where `for_cfg` was true has been removed. - `LLVMRustCreateMCSubtargetInfo` mostly replicates part of `LLVMRustCreateTargetMachine` - `LLVMRustMCSubtargetInfoHasFeature` partly replicates `LLVMRustHasFeature`. - `LLVMRustHasFeature` is no longer needed. - The error message for `custom-target-invalid-llvm-target.rs` changed.
1225a52 to
dacfcaa
Compare
|
cc @rust-lang/clippy These commits modify compiler targets. |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`llvm::target_config` creates `target_machine` by calling `create_informational_target_machine`, which calls `target_machine_factory`, which uses `internal_target_features`. But this is just before `internal_target_features` is initialized! So we should move `internal_target_features` initialization before `target_machine`, right? But `internal_target_features` initialization involves a closure that inspects `target_machine`. There is a cyclic dependency. There is enough function nesting here that it's hard to spot. In practice this cycle doesn't cause problems because the closure doesn't inspect the parts of `target_machine` that depend on `internal_target_features`. But it demonstrates how startup initialization is all tangled up, and it's blocking some cleanups I am doing in rust-lang#161432 relating to the dangerous uses of `Session` before it's fully initialized. Therefore, this commit changes the first part: instead of creating and `OwnedTargetMachine` we create an `OwnedMCSubtargetInfo`. This is a smaller type that has the feature information we need but doesn't depend on `internal_target_features`. Under the covers we are now using LLVM's `Target::createMCSubtargetInfo` instead of `TargetMachine::getMCSubtargetInfo` so that we avoid having to create a `TargetMachine` at this early stage. This eliminates the cycle. (`TargetMachine` can still be created later on, once we're past this fraught initialization.) There are some slight differences between these two approaches, and the preceding commits fixed up some issues there. Some details about this commit: - The new `OwnedMCSubtargetInfo` is similar to the existing `OwnedTargetMachine`. - `create_informational_target_machine` no longer needs a `for_cfg` parameter, because the one site where `for_cfg` was true has been removed. - `LLVMRustCreateMCSubtargetInfo` mostly replicates part of `LLVMRustCreateTargetMachine` - `LLVMRustMCSubtargetInfoHasFeature` partly replicates `LLVMRustHasFeature`. - `LLVMRustHasFeature` is no longer needed. - The error message for `custom-target-invalid-llvm-target.rs` changed.
dacfcaa to
c8f2df6
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
`llvm::target_config` creates `target_machine` by calling `create_informational_target_machine`, which calls `target_machine_factory`, which uses `internal_target_features`. But this is just before `internal_target_features` is initialized! So we should move `internal_target_features` initialization before `target_machine`, right? But `internal_target_features` initialization involves a closure that inspects `target_machine`. There is a cyclic dependency. There is enough function nesting here that it's hard to spot. In practice this cycle doesn't cause problems because the closure doesn't inspect the parts of `target_machine` that depend on `internal_target_features`. But it demonstrates how startup initialization is all tangled up, and it's blocking some cleanups I am doing in rust-lang#161432 relating to the dangerous uses of `Session` before it's fully initialized. Therefore, this commit changes the first part: instead of creating and `OwnedTargetMachine` we create an `OwnedMCSubtargetInfo`. This is a smaller type that has the feature information we need but doesn't depend on `internal_target_features`. Under the covers we are now using LLVM's `Target::createMCSubtargetInfo` instead of `TargetMachine::getMCSubtargetInfo` so that we avoid having to create a `TargetMachine` at this early stage. This eliminates the cycle. (`TargetMachine` can still be created later on, once we're past this fraught initialization.) There are some slight differences between these two approaches, and the preceding commits fixed up some issues there. Some details about this commit: - The new `OwnedMCSubtargetInfo` is similar to the existing `OwnedTargetMachine`. - `create_informational_target_machine` no longer needs a `for_cfg` parameter, because the one site where `for_cfg` was true has been removed. - `LLVMRustCreateMCSubtargetInfo` mostly replicates part of `LLVMRustCreateTargetMachine` - `LLVMRustMCSubtargetInfoHasFeature` partly replicates `LLVMRustHasFeature`. - `LLVMRustHasFeature` is no longer needed. - The error message for `custom-target-invalid-llvm-target.rs` changed.
c8f2df6 to
6279910
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
6279910 to
08d1a72
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. |
|
#161903 has merged and I have rebased. This is ready for review now. |
This comment has been minimized.
This comment has been minimized.
Session creation is currently awkward: we build a mostly-initialized session, then use it to initialize a codegen backend, and then use the codegen backend to finish initializing the session. And it's not just awkward: within the Cranelift backend's `init` method `sess.lto()` is called, which consults `sess.thin_lto_supported`, *before* that field has been properly set! In practice it had no effect but it's worth fixing. This commit cleans up this mess. It introduces `EarlySession`, which contains just four `Session` fields, the ones that are needed for codegen backend initialization. It is now a field within `Session`, and `Session` derefs to `EarlySession` to avoid changing a zillion `sess.target`/`sess.opts`/etc. occurrences. `EarlySession` is passed to `init`, which returns a `CodegenBackendInit` that contains the backend-specific information needed to build a `Session`. (It replaces the `replaced_intrinsics`, `fallback_intrinsics`, and `thin_lto_supported` methods.) The `Session` can then be built in a single step. No more `Session`/`CodegenBackend` initialization intermingling. A few functions that previously took a `Session` now take something else, e.g. a `Target`. Some `Session` methods are now `EarlySession` methods. And a new `early_lto` method is used for Cranelift's LTO check.
It currently takes `&self`, which is a bit strange for an `init` method. As a result, the Cranelift and GCC backends have to use types with interior mutability. This commit changes it to `&mut self`. Benefits: - The Cranelift backend can use `Option` instead of `OnceCell` to indicate uninit vs. init. - The GCC backend can avoid `Mutex`, and use `bool` instead of `AtomicBool`, which makes things much simpler. The commit also restructures `GccCodegenBackend` to mirror `CraneliftCodegenBackend`: just contain an `Option<BackendConfig>`, which makes the uninit vs. init distinction foolproof. (E.g. no need to set `lto_supported` to false and then later overwrite it with the real value.) As part of this the `LockedTargetInfo` type is renamed `SharedTargetInfo` because that better matches its new internals. (All this compiles both with and without the "master" feature set.)
It's now possible to get the backend features (a `Vec<String>`) when the codegen backend is started, pass it back through `CodegenBackendInit`, and just store it in the `Session`. This removes the need for the query. Also: - `WriteBackendMethods::target_machine_factory` no longer needs the `target_features` parameter, because it's now available through the `sess` parameter. - `CodegenContext` no longer needs the `backend_features` field because we can use `sess.global_backend_features` instead. - `CodegenBackend::provide` is now a no-op for all the in-tree backends. I haven't removed it because out-of-tree backends still rely on it.
08d1a72 to
477ab19
Compare
|
@bors try @rust-timer queue |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
… r=<try> Improve `Session`/`CodegenBackend` construction
This comment has been minimized.
This comment has been minimized.
|
Finished benchmarking commit (a4be199): comparison URL. Overall result: ❌✅ regressions and improvements - please read:Benchmarking means the PR may be perf-sensitive. It's automatically marked not fit for rolling up. Overriding is possible but disadvised: it risks changing compiler perf. Next, please: If you can, justify the regressions found in this try perf run in writing along with @bors rollup=never rustc-perf Instruction countOur most reliable metric. Used to determine the overall result above. However, even this metric can be noisy.
Max RSS (memory usage)Results (secondary 0.1%)A less reliable metric. May be of interest, but not used to determine the overall result above.
CyclesResults (secondary -1.3%)A less reliable metric. May be of interest, but not used to determine the overall result above.
Binary sizeThis perf run didn't have relevant results for this metric. Bootstrap: 499.183s -> 495.997s (-0.64%) |
| BackendConfig::from_opts(&sess.opts.cg.llvm_args) | ||
| .unwrap_or_else(|err| sess.dcx().fatal(err)), | ||
| ); | ||
| } |
There was a problem hiding this comment.
This can use let config = self.config.get_or_insert_with(|| ...).
|
r=me with the above change |
View all comments
The creation and initialization of sessions and codegen backends is intertwined, which is confusing and error prone. This commit detangles things, and also simplifies the types used for the state within the backends. Details in individual commits.
r? @bjorn3