From 757c290fba5278989a78c636a41db97204693b3e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 20 Jun 2023 11:45:47 +1000 Subject: [PATCH 1/5] Move `Message::CodegenItem` to a separate type. `Message` is an enum with multiple variants, for messages sent to the coordinator thread. *Except* for `Message::CodegenItem`, which is entirely disjoint, being for messages sent from the coordinator thread to the main thread. This commit move `Message::CodegenItem` into a separate type, `CguMessage`, which makes the code much clearer. --- compiler/rustc_codegen_ssa/src/back/write.rs | 21 ++++++++++---------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 4ccef98afc3f6..a1f252fe672ce 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -951,10 +951,13 @@ pub enum Message { work_product: WorkProduct, }, CodegenComplete, - CodegenItem, CodegenAborted, } +/// A message sent from the coordinator thread to the main thread telling it to +/// process another codegen unit. +pub struct CguMessage; + type DiagnosticArgName<'source> = Cow<'source, str>; struct Diagnostic { @@ -976,7 +979,7 @@ fn start_executing_work( tcx: TyCtxt<'_>, crate_info: &CrateInfo, shared_emitter: SharedEmitter, - codegen_worker_send: Sender>, + codegen_worker_send: Sender, coordinator_receive: Receiver>, total_cgus: usize, jobserver: Client, @@ -1284,9 +1287,9 @@ fn start_executing_work( let anticipated_running = running + additional_running + 1; if !queue_full_enough(work_items.len(), anticipated_running) { - // The queue is not full enough, codegen more items: - if codegen_worker_send.send(Message::CodegenItem).is_err() { - panic!("Could not send Message::CodegenItem to main thread") + // The queue is not full enough, process more codegen units: + if codegen_worker_send.send(CguMessage).is_err() { + panic!("Could not send CguMessage to main thread") } main_thread_worker_state = MainThreadWorkerState::Codegenning; } else { @@ -1522,7 +1525,6 @@ fn start_executing_work( codegen_done = true; codegen_aborted = true; } - Message::CodegenItem => bug!("the coordinator should not receive codegen requests"), } } @@ -1879,7 +1881,7 @@ pub struct OngoingCodegen { pub metadata: EncodedMetadata, pub metadata_module: Option, pub crate_info: CrateInfo, - pub codegen_worker_receive: Receiver>, + pub codegen_worker_receive: Receiver, pub shared_emitter_main: SharedEmitterMain, pub output_filenames: Arc, pub coordinator: Coordinator, @@ -1953,10 +1955,9 @@ impl OngoingCodegen { pub fn wait_for_signal_to_codegen_item(&self) { match self.codegen_worker_receive.recv() { - Ok(Message::CodegenItem) => { - // Nothing to do + Ok(CguMessage) => { + // Ok to proceed. } - Ok(_) => panic!("unexpected message"), Err(_) => { // One of the LLVM threads must have panicked, fall through so // error handling can be reached. From 88cd8f93247a09fb0c2e43e82e0dfedea23e9642 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 20 Jun 2023 12:43:29 +1000 Subject: [PATCH 2/5] Simplify `Message`. `Message` is an enum with multiple variants. Four of those variants map directly onto the four variants of `WorkItemResult`. This commit reduces those four `Message` variants to a single variant containing a `WorkItemResult`. This requires increasing `WorkItemResult`'s visibility to `pub(crate)` visibility, but `WorkItem` and `Message` can also have their visibility reduced to `pub(crate)`. This change avoids some boilerplate enum translation code, and makes `Message` easier to understand. --- compiler/rustc_codegen_ssa/src/back/write.rs | 108 ++++++++----------- 1 file changed, 44 insertions(+), 64 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index a1f252fe672ce..7eb6916ec8433 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -685,7 +685,7 @@ fn produce_final_output_artifacts( // These are used in linking steps and will be cleaned up afterward. } -pub enum WorkItem { +pub(crate) enum WorkItem { /// Optimize a newly codegened, totally unoptimized module. Optimize(ModuleCodegen), /// Copy the post-LTO artifacts from the incremental cache to the output @@ -731,7 +731,7 @@ impl WorkItem { } } -enum WorkItemResult { +pub(crate) enum WorkItemResult { Compiled(CompiledModule), NeedsLink(ModuleCodegen), NeedsFatLTO(FatLTOInput), @@ -923,23 +923,10 @@ fn finish_intra_module_work( } } -pub enum Message { +pub(crate) enum Message { Token(io::Result), - NeedsFatLTO { - result: FatLTOInput, - worker_id: usize, - }, - NeedsThinLTO { - name: String, - thin_buffer: B::ThinBuffer, - worker_id: usize, - }, - NeedsLink { - module: ModuleCodegen, - worker_id: usize, - }, - Done { - result: Result>, + WorkItem { + result: Result, Option>, worker_id: usize, }, CodegenDone { @@ -1481,33 +1468,47 @@ fn start_executing_work( codegen_done = true; codegen_aborted = true; } - Message::Done { result: Ok(compiled_module), worker_id } => { + + Message::WorkItem { result, worker_id } => { free_worker(worker_id); - match compiled_module.kind { - ModuleKind::Regular => { - compiled_modules.push(compiled_module); + + match result { + Ok(WorkItemResult::Compiled(compiled_module)) => { + match compiled_module.kind { + ModuleKind::Regular => { + compiled_modules.push(compiled_module); + } + ModuleKind::Allocator => { + assert!(compiled_allocator_module.is_none()); + compiled_allocator_module = Some(compiled_module); + } + ModuleKind::Metadata => bug!("Should be handled separately"), + } + } + Ok(WorkItemResult::NeedsLink(module)) => { + needs_link.push(module); + } + Ok(WorkItemResult::NeedsFatLTO(fat_lto_input)) => { + assert!(!started_lto); + needs_fat_lto.push(fat_lto_input); + } + Ok(WorkItemResult::NeedsThinLTO(name, thin_buffer)) => { + assert!(!started_lto); + needs_thin_lto.push((name, thin_buffer)); } - ModuleKind::Allocator => { - assert!(compiled_allocator_module.is_none()); - compiled_allocator_module = Some(compiled_module); + Err(Some(WorkerFatalError)) => { + // Like `CodegenAborted`, wait for remaining work to finish. + codegen_done = true; + codegen_aborted = true; + } + Err(None) => { + // If the thread failed that means it panicked, so + // we abort immediately. + bug!("worker thread panicked"); } - ModuleKind::Metadata => bug!("Should be handled separately"), } } - Message::NeedsLink { module, worker_id } => { - free_worker(worker_id); - needs_link.push(module); - } - Message::NeedsFatLTO { result, worker_id } => { - assert!(!started_lto); - free_worker(worker_id); - needs_fat_lto.push(result); - } - Message::NeedsThinLTO { name, thin_buffer, worker_id } => { - assert!(!started_lto); - free_worker(worker_id); - needs_thin_lto.push((name, thin_buffer)); - } + Message::AddImportOnlyModule { module_data, work_product } => { assert!(!started_lto); assert!(!codegen_done); @@ -1515,16 +1516,6 @@ fn start_executing_work( lto_import_only_modules.push((module_data, work_product)); main_thread_worker_state = MainThreadWorkerState::Idle; } - // If the thread failed that means it panicked, so we abort immediately. - Message::Done { result: Err(None), worker_id: _ } => { - bug!("worker thread panicked"); - } - Message::Done { result: Err(Some(WorkerFatalError)), worker_id } => { - // Similar to CodegenAborted, wait for remaining work to finish. - free_worker(worker_id); - codegen_done = true; - codegen_aborted = true; - } } } @@ -1643,22 +1634,11 @@ fn spawn_work(cgcx: CodegenContext, work: WorkItem fn drop(&mut self) { let worker_id = self.worker_id; let msg = match self.result.take() { - Some(Ok(WorkItemResult::Compiled(m))) => { - Message::Done:: { result: Ok(m), worker_id } - } - Some(Ok(WorkItemResult::NeedsLink(m))) => { - Message::NeedsLink:: { module: m, worker_id } - } - Some(Ok(WorkItemResult::NeedsFatLTO(m))) => { - Message::NeedsFatLTO:: { result: m, worker_id } - } - Some(Ok(WorkItemResult::NeedsThinLTO(name, thin_buffer))) => { - Message::NeedsThinLTO:: { name, thin_buffer, worker_id } - } + Some(Ok(result)) => Message::WorkItem:: { result: Ok(result), worker_id }, Some(Err(FatalError)) => { - Message::Done:: { result: Err(Some(WorkerFatalError)), worker_id } + Message::WorkItem:: { result: Err(Some(WorkerFatalError)), worker_id } } - None => Message::Done:: { result: Err(None), worker_id }, + None => Message::WorkItem:: { result: Err(None), worker_id }, }; drop(self.coordinator_send.send(Box::new(msg))); } From a521ba400dbfb2e9907238a4e7a5650d22bf2068 Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 20 Jun 2023 12:57:38 +1000 Subject: [PATCH 3/5] Add comments to `Message` and `WorkItem`. This is particularly useful for `Message`. --- compiler/rustc_codegen_ssa/src/back/write.rs | 30 ++++++++++++++------ 1 file changed, 22 insertions(+), 8 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 7eb6916ec8433..9f4d767067ec0 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -731,6 +731,7 @@ impl WorkItem { } } +/// A result produced by the backend. pub(crate) enum WorkItemResult { Compiled(CompiledModule), NeedsLink(ModuleCodegen), @@ -923,21 +924,34 @@ fn finish_intra_module_work( } } +/// Messages sent to the coordinator. pub(crate) enum Message { + /// A jobserver token has become available. Sent from the jobserver helper + /// thread. Token(io::Result), - WorkItem { - result: Result, Option>, - worker_id: usize, - }, - CodegenDone { - llvm_work_item: WorkItem, - cost: u64, - }, + + /// The backend has finished processing a work item for a codegen unit. + /// Sent from a backend worker thread. + WorkItem { result: Result, Option>, worker_id: usize }, + + /// The frontend has finished generating something (backend IR or a + /// post-LTO artifact) for a codegen unit, and it should be passed to the + /// backend. Sent from the main thread. + CodegenDone { llvm_work_item: WorkItem, cost: u64 }, + + /// Similar to `CodegenDone`, but for reusing a pre-LTO artifact + /// Sent from the main thread. AddImportOnlyModule { module_data: SerializedModule, work_product: WorkProduct, }, + + /// The frontend has finished generating everything for all codegen units. + /// Sent from the main thread. CodegenComplete, + + /// Some normal-ish compiler error occurred, and codegen should be wound + /// down. Sent from the main thread. CodegenAborted, } From 3bbf9f0128517815946a0c20efd7d80142dc050d Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 20 Jun 2023 14:05:55 +1000 Subject: [PATCH 4/5] Introduce `CodegenState`. The codegen main loop has two bools, `codegen_done` and `codegen_aborted`. There are only three valid combinations: `(false, false)`, `(true, false)`, `(true, true)`. This commit replaces them with a single tri-state enum, which makes things clearer. --- compiler/rustc_codegen_ssa/src/back/write.rs | 49 ++++++++++++-------- 1 file changed, 29 insertions(+), 20 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 9f4d767067ec0..95de9b61515d5 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -1239,10 +1239,19 @@ fn start_executing_work( let mut needs_thin_lto = Vec::new(); let mut lto_import_only_modules = Vec::new(); let mut started_lto = false; - let mut codegen_aborted = false; - // This flag tracks whether all items have gone through codegens - let mut codegen_done = false; + /// Possible state transitions: + /// - Ongoing -> Completed + /// - Ongoing -> Aborted + /// - Completed -> Aborted + #[derive(Debug, PartialEq)] + enum CodegenState { + Ongoing, + Completed, + Aborted, + } + use CodegenState::*; + let mut codegen_state = Ongoing; // This is the queue of LLVM work items that still need processing. let mut work_items = Vec::<(WorkItem, u64)>::new(); @@ -1262,10 +1271,10 @@ fn start_executing_work( // wait for all existing work to finish, so many of the conditions here // only apply if codegen hasn't been aborted as they represent pending // work to be done. - while !codegen_done + while codegen_state == Ongoing || running > 0 || main_thread_worker_state == MainThreadWorkerState::LLVMing - || (!codegen_aborted + || (codegen_state == Completed && !(work_items.is_empty() && needs_fat_lto.is_empty() && needs_thin_lto.is_empty() @@ -1275,7 +1284,7 @@ fn start_executing_work( // While there are still CGUs to be codegened, the coordinator has // to decide how to utilize the compiler processes implicit Token: // For codegenning more CGU or for running them through LLVM. - if !codegen_done { + if codegen_state == Ongoing { if main_thread_worker_state == MainThreadWorkerState::Idle { // Compute the number of workers that will be running once we've taken as many // items from the work queue as we can, plus one for the main thread. It's not @@ -1312,10 +1321,7 @@ fn start_executing_work( spawn_work(cgcx, item); } } - } else if codegen_aborted { - // don't queue up any more work if codegen was aborted, we're - // just waiting for our existing children to finish - } else { + } else if codegen_state == Completed { // If we've finished everything related to normal codegen // then it must be the case that we've got some LTO work to do. // Perform the serial work here of figuring out what we're @@ -1382,11 +1388,15 @@ fn start_executing_work( // Already making good use of that token } } + } else { + // Don't queue up any more work if codegen was aborted, we're + // just waiting for our existing children to finish. + assert!(codegen_state == Aborted); } // Spin up what work we can, only doing this while we've got available // parallelism slots and work left to spawn. - while !codegen_aborted && !work_items.is_empty() && running < tokens.len() { + while codegen_state != Aborted && !work_items.is_empty() && running < tokens.len() { let (item, _) = work_items.pop().unwrap(); maybe_start_llvm_timer(prof, cgcx.config(item.module_kind()), &mut llvm_start_time); @@ -1438,8 +1448,7 @@ fn start_executing_work( Err(e) => { let msg = &format!("failed to acquire jobserver token: {}", e); shared_emitter.fatal(msg); - codegen_done = true; - codegen_aborted = true; + codegen_state = Aborted; } } } @@ -1467,7 +1476,9 @@ fn start_executing_work( } Message::CodegenComplete => { - codegen_done = true; + if codegen_state != Aborted { + codegen_state = Completed; + } assert_eq!(main_thread_worker_state, MainThreadWorkerState::Codegenning); main_thread_worker_state = MainThreadWorkerState::Idle; } @@ -1479,8 +1490,7 @@ fn start_executing_work( // then conditions above will ensure no more work is spawned but // we'll keep executing this loop until `running` hits 0. Message::CodegenAborted => { - codegen_done = true; - codegen_aborted = true; + codegen_state = Aborted; } Message::WorkItem { result, worker_id } => { @@ -1512,8 +1522,7 @@ fn start_executing_work( } Err(Some(WorkerFatalError)) => { // Like `CodegenAborted`, wait for remaining work to finish. - codegen_done = true; - codegen_aborted = true; + codegen_state = Aborted; } Err(None) => { // If the thread failed that means it panicked, so @@ -1525,7 +1534,7 @@ fn start_executing_work( Message::AddImportOnlyModule { module_data, work_product } => { assert!(!started_lto); - assert!(!codegen_done); + assert_eq!(codegen_state, Ongoing); assert_eq!(main_thread_worker_state, MainThreadWorkerState::Codegenning); lto_import_only_modules.push((module_data, work_product)); main_thread_worker_state = MainThreadWorkerState::Idle; @@ -1533,7 +1542,7 @@ fn start_executing_work( } } - if codegen_aborted { + if codegen_state == Aborted { return Err(()); } From fae4f452141568193c1a833b16b5589beb19334e Mon Sep 17 00:00:00 2001 From: Nicholas Nethercote Date: Tue, 20 Jun 2023 15:08:49 +1000 Subject: [PATCH 5/5] Remove unused fields from `CodegenContext`. --- compiler/rustc_codegen_ssa/src/back/write.rs | 12 ------------ compiler/rustc_codegen_ssa/src/base.rs | 12 +++--------- 2 files changed, 3 insertions(+), 21 deletions(-) diff --git a/compiler/rustc_codegen_ssa/src/back/write.rs b/compiler/rustc_codegen_ssa/src/back/write.rs index 95de9b61515d5..51ac441a7a425 100644 --- a/compiler/rustc_codegen_ssa/src/back/write.rs +++ b/compiler/rustc_codegen_ssa/src/back/write.rs @@ -320,7 +320,6 @@ pub type ExportedSymbols = FxHashMap { // Resources needed when running LTO - pub backend: B, pub prof: SelfProfilerRef, pub lto: Lto, pub save_temps: bool, @@ -338,14 +337,10 @@ pub struct CodegenContext { pub msvc_imps_needed: bool, pub is_pe_coff: bool, pub target_can_use_split_dwarf: bool, - pub target_pointer_width: u32, pub target_arch: String, - pub debuginfo: config::DebugInfo, pub split_debuginfo: rustc_target::spec::SplitDebuginfo, pub split_dwarf_kind: rustc_session::config::SplitDwarfKind, - /// Number of cgus excluding the allocator/metadata modules - pub total_cgus: usize, /// Handler to use for diagnostics produced during codegen. pub diag_emitter: SharedEmitter, /// LLVM optimizations for which we want to print remarks. @@ -441,7 +436,6 @@ pub fn start_async_codegen( target_cpu: String, metadata: EncodedMetadata, metadata_module: Option, - total_cgus: usize, ) -> OngoingCodegen { let (coordinator_send, coordinator_receive) = channel(); let sess = tcx.sess; @@ -469,7 +463,6 @@ pub fn start_async_codegen( shared_emitter, codegen_worker_send, coordinator_receive, - total_cgus, sess.jobserver.clone(), Arc::new(regular_config), Arc::new(metadata_config), @@ -982,7 +975,6 @@ fn start_executing_work( shared_emitter: SharedEmitter, codegen_worker_send: Sender, coordinator_receive: Receiver>, - total_cgus: usize, jobserver: Client, regular_config: Arc, metadata_config: Arc, @@ -1050,7 +1042,6 @@ fn start_executing_work( }; let backend_features = tcx.global_backend_features(()); let cgcx = CodegenContext:: { - backend: backend.clone(), crate_types: sess.crate_types().to_vec(), each_linked_rlib_for_lto, lto: sess.lto(), @@ -1071,13 +1062,10 @@ fn start_executing_work( metadata_module_config: metadata_config, allocator_module_config: allocator_config, tm_factory: backend.target_machine_factory(tcx.sess, ol, backend_features), - total_cgus, msvc_imps_needed: msvc_imps_needed(tcx), is_pe_coff: tcx.sess.target.is_like_windows, target_can_use_split_dwarf: tcx.sess.target_can_use_split_dwarf(), - target_pointer_width: tcx.sess.target.pointer_width, target_arch: tcx.sess.target.arch.to_string(), - debuginfo: tcx.sess.opts.debuginfo, split_debuginfo: tcx.sess.split_debuginfo(), split_dwarf_kind: tcx.sess.opts.unstable_opts.split_dwarf_kind, }; diff --git a/compiler/rustc_codegen_ssa/src/base.rs b/compiler/rustc_codegen_ssa/src/base.rs index 6abc56d597562..28f3c23364cf2 100644 --- a/compiler/rustc_codegen_ssa/src/base.rs +++ b/compiler/rustc_codegen_ssa/src/base.rs @@ -580,7 +580,7 @@ pub fn codegen_crate( ) -> OngoingCodegen { // Skip crate items and just output metadata in -Z no-codegen mode. if tcx.sess.opts.unstable_opts.no_codegen || !tcx.sess.opts.output_types.should_codegen() { - let ongoing_codegen = start_async_codegen(backend, tcx, target_cpu, metadata, None, 1); + let ongoing_codegen = start_async_codegen(backend, tcx, target_cpu, metadata, None); ongoing_codegen.codegen_finished(tcx); @@ -631,14 +631,8 @@ pub fn codegen_crate( }) }); - let ongoing_codegen = start_async_codegen( - backend.clone(), - tcx, - target_cpu, - metadata, - metadata_module, - codegen_units.len(), - ); + let ongoing_codegen = + start_async_codegen(backend.clone(), tcx, target_cpu, metadata, metadata_module); // Codegen an allocator shim, if necessary. if let Some(kind) = allocator_kind_for_codegen(tcx) {