From d429da3cb4b98a3345850a744bdcfe3bb8d902a0 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 25 Jul 2025 02:24:42 +0300 Subject: [PATCH 1/4] linker: make DCE mandatory and run it very early on, as well. --- .../rustc_codegen_spirv/src/codegen_cx/mod.rs | 2 - crates/rustc_codegen_spirv/src/linker/mod.rs | 56 +++++++++---------- crates/rustc_codegen_spirv/src/linker/test.rs | 1 - docs/src/codegen-args.md | 8 +-- 4 files changed, 27 insertions(+), 40 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 3b8f7651de..f3811f8a19 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -397,7 +397,6 @@ impl CodegenArgs { // FIXME(eddyb) should these be handled as `-C linker-args="..."` instead? { // FIXME(eddyb) clean up this `no-` "negation prefix" situation. - opts.optflag("", "no-dce", "disables running dead code elimination"); opts.optflag( "", "no-compact-ids", @@ -604,7 +603,6 @@ impl CodegenArgs { // FIXME(eddyb) should these be handled as `-C linker-args="..."` instead? let linker_opts = crate::linker::Options { // FIXME(eddyb) clean up this `no-` "negation prefix" situation. - dce: !matches.opt_present("no-dce"), compact_ids: !matches.opt_present("no-compact-ids"), early_report_zombies: !matches.opt_present("no-early-report-zombies"), infer_storage_classes: !matches.opt_present("no-infer-storage-classes"), diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index d1e704cab7..a752aae899 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -38,7 +38,6 @@ pub type Result = std::result::Result; #[derive(Default)] pub struct Options { pub compact_ids: bool, - pub dce: bool, pub early_report_zombies: bool, pub infer_storage_classes: bool, pub structurize: bool, @@ -272,6 +271,13 @@ pub fn link( output }; + // FIXME(eddyb) do this before ever saving the original `.spv`s to disk. + { + let _timer = sess.timer("link_dce-post-merge"); + dce::dce(&mut output); + } + + // HACK(eddyb) this has to be after DCE, to not break SPIR-T w/ dead decorations. if let Some(dir) = &opts.dump_post_merge { dump_spv_and_spirt(&output, dir.join(disambiguated_crate_name_for_dumps)); } @@ -292,6 +298,11 @@ pub fn link( import_export_link::run(opts, sess, &mut output)?; } + { + let _timer = sess.timer("link_dce-post-link"); + dce::dce(&mut output); + } + { let _timer = sess.timer("link_fragment_inst_check"); simple_passes::check_fragment_insts(sess, &output)?; @@ -306,29 +317,11 @@ pub fn link( } if opts.early_report_zombies { - // HACK(eddyb) `report_and_remove_zombies` is bad at determining whether - // some things are dead (such as whole blocks), and there's no reason to - // *not* run DCE, given SPIR-T exists and makes DCE mandatory, but we're - // still only going to do the minimum necessary ("block ordering"). - { - let _timer = sess.timer("link_block_ordering_pass-before-report_and_remove_zombies"); - for func in &mut output.functions { - simple_passes::block_ordering_pass(func); - } - } - let _timer = sess.timer("link_report_and_remove_zombies"); zombies::report_and_remove_zombies(sess, &mut output)?; } if opts.infer_storage_classes { - // HACK(eddyb) this is not the best approach, but storage class inference - // can still fail in entirely legitimate ways (i.e. mismatches in zombies). - if !opts.early_report_zombies { - let _timer = sess.timer("link_dce-before-specialize_generic_storage_class"); - dce::dce(&mut output); - } - let _timer = sess.timer("specialize_generic_storage_class"); // HACK(eddyb) `specializer` requires functions' blocks to be in RPO order // (i.e. `block_ordering_pass`) - this could be relaxed by using RPO visit @@ -361,7 +354,7 @@ pub fn link( // NOTE(eddyb) with SPIR-T, we can do `mem2reg` before inlining, too! { - if opts.dce { + { let _timer = sess.timer("link_dce-before-inlining"); dce::dce(&mut output); } @@ -404,13 +397,14 @@ pub fn link( } } - if opts.dce { + { let _timer = sess.timer("link_dce-and-remove_duplicate_debuginfo-after-mem2reg-before-inlining"); dce::dce(&mut output); duplicates::remove_duplicate_debuginfo(&mut output); } + // HACK(eddyb) this has to be after DCE, to not break SPIR-T w/ dead decorations. if let Some(dir) = &opts.dump_pre_inline { dump_spv_and_spirt(&output, dir.join(disambiguated_crate_name_for_dumps)); } @@ -420,7 +414,7 @@ pub fn link( inline::inline(sess, &mut output)?; } - if opts.dce { + { let _timer = sess.timer("link_dce-after-inlining"); dce::dce(&mut output); } @@ -469,7 +463,7 @@ pub fn link( } } - if opts.dce { + { let _timer = sess.timer("link_dce-and-remove_duplicate_debuginfo-after-mem2reg-after-inlining"); dce::dce(&mut output); @@ -711,6 +705,15 @@ pub fn link( ), }; for (file_stem, output) in output_module_iter { + // Run DCE again, even if module_output_type == ModuleOutputType::Multiple - the first DCE ran before + // structurization and mem2reg (for perf reasons), and mem2reg may remove references to + // invalid types, so we need to DCE again. + { + let _timer = sess.timer("link_dce-post-split"); + dce::dce(output); + } + + // HACK(eddyb) this has to be after DCE, to not break SPIR-T w/ dead decorations. if let Some(dir) = &opts.dump_post_split { let mut file_name = disambiguated_crate_name_for_dumps.to_os_string(); if let Some(file_stem) = file_stem { @@ -720,13 +723,6 @@ pub fn link( dump_spv_and_spirt(output, dir.join(file_name)); } - // Run DCE again, even if module_output_type == ModuleOutputType::Multiple - the first DCE ran before - // structurization and mem2reg (for perf reasons), and mem2reg may remove references to - // invalid types, so we need to DCE again. - if opts.dce { - let _timer = sess.timer("link_dce_2"); - dce::dce(output); - } { let _timer = sess.timer("link_remove_duplicate_debuginfo"); diff --git a/crates/rustc_codegen_spirv/src/linker/test.rs b/crates/rustc_codegen_spirv/src/linker/test.rs index adb55d94af..d5316f2f60 100644 --- a/crates/rustc_codegen_spirv/src/linker/test.rs +++ b/crates/rustc_codegen_spirv/src/linker/test.rs @@ -70,7 +70,6 @@ fn assemble_and_link(binaries: &[&[u8]]) -> Result { binaries, &crate::linker::Options { compact_ids: true, - dce: true, keep_link_exports: true, ..Default::default() }, diff --git a/docs/src/codegen-args.md b/docs/src/codegen-args.md index 7c5d35913d..71fb69d6d2 100644 --- a/docs/src/codegen-args.md +++ b/docs/src/codegen-args.md @@ -89,8 +89,7 @@ Dumps the module, to a file in `DIR`, immediately after the inliner pass runs. ### `--dump-post-split DIR` -Dumps the modules, to files in `DIR`, immediately after multimodule splitting, but before final cleanup passes (e.g. -DCE to remove the other entry points). +Dumps the modules, to files in `DIR`, immediately after multimodule splitting. ### `--dump-post-link DIR` @@ -113,11 +112,6 @@ Disables running `spirv-val` on the final output. Spooky scary option, can cause Forcibly disables running `spirv-opt` on the final output, even if optimizations are enabled. -### `--no-dce` - -Disables running dead code elimination. Can and probably will generate invalid modules or crash the -linker, hasn't been tested for a while. - ### `--no-compact-ids` Disables compaction of SPIR-V IDs at the end of linking. Causes absolutely ginormous IDs to be From 84929d9f6d1d2a1280e2f16c32a762d9cf754458 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 25 Jul 2025 03:02:40 +0300 Subject: [PATCH 2/4] linker: early zombie reporting doesn't need to mutate the module anymore (thanks to DCE). --- crates/rustc_codegen_spirv/src/linker/mod.rs | 8 +- .../rustc_codegen_spirv/src/linker/zombies.rs | 94 +------------------ docs/src/tracing.md | 2 - 3 files changed, 8 insertions(+), 96 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index a752aae899..5d510755df 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -308,8 +308,8 @@ pub fn link( simple_passes::check_fragment_insts(sess, &output)?; } - // HACK(eddyb) this has to run before the `report_and_remove_zombies` pass, - // so that any zombies that are passed as call arguments, but eventually unused, + // HACK(eddyb) this has to run before the `report_zombies` pass, so that + // any zombies that are passed as call arguments, but eventually unused, // won't be (incorrectly) considered used. { let _timer = sess.timer("link_remove_unused_params"); @@ -317,8 +317,8 @@ pub fn link( } if opts.early_report_zombies { - let _timer = sess.timer("link_report_and_remove_zombies"); - zombies::report_and_remove_zombies(sess, &mut output)?; + let _timer = sess.timer("link_report_zombies"); + zombies::report_zombies(sess, &output)?; } if opts.infer_storage_classes { diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index 207548c38f..988c368226 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -1,16 +1,14 @@ //! See documentation on `CodegenCx::zombie` for a description of the zombie system. -use super::{get_name, get_names}; +use super::get_names; use crate::custom_decorations::{CustomDecoration, SpanRegenerator, ZombieDecoration}; use crate::custom_insts::{self, CustomOp}; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Op, Word}; -use rustc_data_structures::fx::{FxHashMap, FxHashSet, FxIndexMap}; +use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; use rustc_errors::Diag; use rustc_session::Session; use rustc_span::{DUMMY_SP, Span}; -use smallvec::SmallVec; -use tracing::{Level, debug}; #[derive(Copy, Clone)] struct Zombie<'a> { @@ -322,7 +320,7 @@ impl<'a> ZombieReporter<'a> { } } -pub fn report_and_remove_zombies(sess: &Session, module: &mut Module) -> super::Result<()> { +pub fn report_zombies(sess: &Session, module: &Module) -> super::Result<()> { let mut zombies = Zombies { // FIXME(eddyb) avoid repeating this across different passes/helpers. custom_ext_inst_set_import: module @@ -341,89 +339,5 @@ pub fn report_and_remove_zombies(sess: &Session, module: &mut Module) -> super:: // Note: This is O(n^2). while zombies.spread(module) {} - let result = ZombieReporter::new(sess, module, &zombies).report_all(); - if tracing::enabled!(target: "print_all_zombie", Level::DEBUG) { - let mut span_regen = SpanRegenerator::new(sess.source_map(), module); - for &zombie_id in zombies.id_to_zombie_kind.keys() { - let mut zombie_leaf_id = zombie_id; - let mut infection_chain = SmallVec::<[_; 4]>::new(); - loop { - zombie_leaf_id = match zombies.get_zombie_by_id(zombie_leaf_id).unwrap().kind { - ZombieKind::Leaf => break, - // FIXME(eddyb) this is all very lossy and should probably go away. - ZombieKind::Uses(zombie_uses) => zombie_uses[0].used_zombie_id, - }; - infection_chain.push(zombie_leaf_id); - } - - let reason = span_regen.zombie_for_id(zombie_leaf_id).unwrap().reason; - debug!( - target: "print_all_zombie", - "zombie'd %{zombie_id} because {reason}" - ); - if !infection_chain.is_empty() { - debug!( - target: "print_all_zombie", - " (infected via {:?})", infection_chain - ); - } - debug!(target: "print_all_zombie", ""); - } - } - - if tracing::enabled!(target: "print_zombie", Level::DEBUG) { - let mut span_regen = SpanRegenerator::new(sess.source_map(), module); - let names = get_names(module); - for f in &module.functions { - if let Some(zombie) = zombies.get_zombie_by_id(f.def_id().unwrap()) { - let mut zombie_leaf_id = zombie.id; - loop { - zombie_leaf_id = match zombies.get_zombie_by_id(zombie_leaf_id).unwrap().kind { - ZombieKind::Leaf => break, - // FIXME(eddyb) this is all very lossy and should probably go away. - ZombieKind::Uses(zombie_uses) => zombie_uses[0].used_zombie_id, - }; - } - - let name = get_name(&names, f.def_id().unwrap()); - let reason = span_regen.zombie_for_id(zombie_leaf_id).unwrap().reason; - debug!( - target: "print_zombie", - "function removed {name:?} because {reason:?}" - ); - } - } - } - - // FIXME(eddyb) this should be unnecessary, either something is unused, and - // it will get DCE'd *anyway*, or it caused an error. - { - // HACK(eddyb) cannot use the original map because it borrows the `Module`. - let all_zombies: FxHashSet<_> = zombies.id_to_zombie_kind.into_keys().collect(); - let keep = |inst: &Instruction| { - if let Some(result_id) = inst.result_id { - !all_zombies.contains(&result_id) - } else { - let mut inst_ids = inst - .result_type - .into_iter() - .chain(inst.operands.iter().filter_map(|op| op.id_ref_any())); - !inst_ids.any(|id| all_zombies.contains(&id)) - } - }; - module.capabilities.retain(keep); - module.extensions.retain(keep); - module.ext_inst_imports.retain(keep); - module.memory_model = module.memory_model.take().filter(keep); - module.entry_points.retain(keep); - module.execution_modes.retain(keep); - module.debug_string_source.retain(keep); - module.debug_names.retain(keep); - module.debug_module_processed.retain(keep); - module.annotations.retain(keep); - module.types_global_values.retain(keep); - module.functions.retain(|f| keep(f.def.as_ref().unwrap())); - } - - result + ZombieReporter::new(sess, module, &zombies).report_all() } diff --git a/docs/src/tracing.md b/docs/src/tracing.md index fab61effce..df05a059b8 100644 --- a/docs/src/tracing.md +++ b/docs/src/tracing.md @@ -19,5 +19,3 @@ arguments](./codegen-args.md) to aid observability. As of have been removed and replaced with the following: - `--specializer-debug` → `RUSTGPU_LOG=rustc_codegen_spirv::specializer=debug` -- `--print-zombie` → `RUSTGPU_LOG=print_zombie=debug` -- `--print-all-zombie` → `RUSTGPU_LOG=print_all_zombie=debug` From 447cfa21f231d3169300bac5edf673875bed1580 Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 25 Jul 2025 03:22:31 +0300 Subject: [PATCH 3/4] Triage `WriteBackendMethods` into "unimplemented" (w/ better errors) vs `optimize*` (centralized, even if noop). --- crates/rustc_codegen_spirv/src/lib.rs | 154 +++++++++++------- crates/rustc_codegen_spirv/src/link.rs | 21 ++- crates/rustc_codegen_spirv/src/linker/mod.rs | 33 ++-- crates/rustc_codegen_spirv/src/linker/test.rs | 6 +- 4 files changed, 129 insertions(+), 85 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/lib.rs b/crates/rustc_codegen_spirv/src/lib.rs index 2954fd31d4..6d0da56427 100644 --- a/crates/rustc_codegen_spirv/src/lib.rs +++ b/crates/rustc_codegen_spirv/src/lib.rs @@ -167,7 +167,7 @@ use rustc_session::Session; use rustc_session::config::{self, OutputFilenames, OutputType}; use rustc_span::symbol::Symbol; use std::any::Any; -use std::fs::{File, create_dir_all}; +use std::fs; use std::io::Cursor; use std::io::Write; use std::path::Path; @@ -175,8 +175,8 @@ use std::sync::Arc; use tracing::{error, warn}; fn dump_mir(tcx: TyCtxt<'_>, mono_items: &[(MonoItem<'_>, MonoItemData)], path: &Path) { - create_dir_all(path.parent().unwrap()).unwrap(); - let mut file = File::create(path).unwrap(); + fs::create_dir_all(path.parent().unwrap()).unwrap(); + let mut file = fs::File::create(path).unwrap(); for &(mono_item, _) in mono_items { if let MonoItem::Fn(instance) = mono_item && matches!(instance.def, InstanceKind::Item(_)) @@ -189,27 +189,6 @@ fn dump_mir(tcx: TyCtxt<'_>, mono_items: &[(MonoItem<'_>, MonoItemData)], path: } } -// TODO: Should this store Vec or Module? -struct SpirvModuleBuffer(Vec); - -impl ModuleBufferMethods for SpirvModuleBuffer { - fn data(&self) -> &[u8] { - spirv_tools::binary::from_binary(&self.0) - } -} - -// TODO: Should this store Vec or Module? -struct SpirvThinBuffer(Vec); - -impl ThinBufferMethods for SpirvThinBuffer { - fn data(&self) -> &[u8] { - spirv_tools::binary::from_binary(&self.0) - } - fn thin_link_data(&self) -> &[u8] { - &[] - } -} - #[derive(Clone)] struct SpirvCodegenBackend; @@ -306,28 +285,76 @@ impl CodegenBackend for SpirvCodegenBackend { } } +struct SpirvModuleBuffer(Vec); + +impl SpirvModuleBuffer { + fn as_bytes(&self) -> &[u8] { + spirv_tools::binary::from_binary(&self.0) + } +} +impl ModuleBufferMethods for SpirvModuleBuffer { + fn data(&self) -> &[u8] { + self.as_bytes() + } +} +impl ThinBufferMethods for SpirvModuleBuffer { + fn data(&self) -> &[u8] { + self.as_bytes() + } + fn thin_link_data(&self) -> &[u8] { + &[] + } +} + +impl SpirvCodegenBackend { + fn optimize_common( + _cgcx: &CodegenContext, + _module: &mut ModuleCodegen<::Module>, + ) -> Result<(), FatalError> { + // FIXME(eddyb) actually run as many optimization passes as possible, + // before ever serializing `.spv` files that will later get linked. + Ok(()) + } +} + impl WriteBackendMethods for SpirvCodegenBackend { - type Module = Vec; + type Module = rspirv::dr::Module; type TargetMachine = (); type TargetMachineError = String; type ModuleBuffer = SpirvModuleBuffer; type ThinData = (); - type ThinBuffer = SpirvThinBuffer; + type ThinBuffer = SpirvModuleBuffer; + // FIXME(eddyb) reuse the "merge" stage of `crate::linker` for this, or even + // delegate to `run_fat_lto` (although `-Zcombine-cgu` is much more niche). fn run_link( - _cgcx: &CodegenContext, - _diag_handler: DiagCtxtHandle<'_>, + cgcx: &CodegenContext, + diag_handler: DiagCtxtHandle<'_>, _modules: Vec>, ) -> Result, FatalError> { - todo!() + assert!( + cgcx.opts.unstable_opts.combine_cgu, + "`run_link` (for `WorkItemResult::NeedsLink`) should \ + only be invoked due to `-Zcombine-cgu`" + ); + diag_handler.fatal("Rust-GPU does not support `-Zcombine-cgu`") } + // FIXME(eddyb) reuse the "merge" stage of `crate::linker` for this, or even + // consider setting `requires_lto = true` in the target specs and moving the + // entirety of `crate::linker` into this stage (lacking diagnostics may be + // an issue - it's surprising `CodegenBackend::link` has `Session` at all). fn run_fat_lto( - _: &CodegenContext, - _: Vec>, - _: Vec<(SerializedModule, WorkProduct)>, + cgcx: &CodegenContext, + _modules: Vec>, + _cached_modules: Vec<(SerializedModule, WorkProduct)>, ) -> Result, FatalError> { - todo!() + assert!( + cgcx.lto == rustc_session::config::Lto::Fat, + "`run_fat_lto` (for `WorkItemResult::NeedsFatLto`) should \ + only be invoked due to `-Clto` (or equivalent)" + ); + unreachable!("Rust-GPU does not support fat LTO") } fn run_thin_lto( @@ -347,35 +374,39 @@ impl WriteBackendMethods for SpirvCodegenBackend { } fn optimize( - _: &CodegenContext, - _: DiagCtxtHandle<'_>, - _: &mut ModuleCodegen, - _: &ModuleConfig, + cgcx: &CodegenContext, + _dcx: DiagCtxtHandle<'_>, + module: &mut ModuleCodegen, + _config: &ModuleConfig, ) -> Result<(), FatalError> { - // TODO: Implement - Ok(()) + Self::optimize_common(cgcx, module) } fn optimize_thin( - _cgcx: &CodegenContext, + cgcx: &CodegenContext, thin_module: ThinModule, ) -> Result, FatalError> { - let module = ModuleCodegen { - module_llvm: spirv_tools::binary::to_binary(thin_module.data()) - .unwrap() - .to_vec(), + // FIXME(eddyb) the inefficiency of Module -> [u8] -> Module roundtrips + // comes from upstream and it applies to `rustc_codegen_llvm` as well, + // eventually it should be properly addressed (for `ThinLocal` at least). + let mut module = ModuleCodegen { + module_llvm: link::with_rspirv_loader(|loader| { + rspirv::binary::parse_bytes(thin_module.data(), loader) + }) + .unwrap(), name: thin_module.name().to_string(), kind: ModuleKind::Regular, thin_lto_buffer: None, }; + Self::optimize_common(cgcx, &mut module)?; Ok(module) } fn optimize_fat( - _: &CodegenContext, - _: &mut ModuleCodegen, + cgcx: &CodegenContext, + module: &mut ModuleCodegen, ) -> Result<(), FatalError> { - todo!() + Self::optimize_common(cgcx, module) } fn codegen( @@ -384,20 +415,19 @@ impl WriteBackendMethods for SpirvCodegenBackend { module: ModuleCodegen, _config: &ModuleConfig, ) -> Result { + let kind = module.kind; + let (name, module_buffer) = Self::serialize_module(module); + let path = cgcx.output_filenames.temp_path_for_cgu( OutputType::Object, - &module.name, + &name, cgcx.invocation_temp.as_deref(), ); - // Note: endianness doesn't matter, readers deduce endianness from magic header. - let spirv_module = spirv_tools::binary::from_binary(&module.module_llvm); - File::create(&path) - .unwrap() - .write_all(spirv_module) - .unwrap(); + fs::write(&path, module_buffer.as_bytes()).unwrap(); + Ok(CompiledModule { - name: module.name, - kind: module.kind, + name, + kind, object: Some(path), dwarf_object: None, bytecode: None, @@ -411,11 +441,14 @@ impl WriteBackendMethods for SpirvCodegenBackend { module: ModuleCodegen, _want_summary: bool, ) -> (String, Self::ThinBuffer) { - (module.name, SpirvThinBuffer(module.module_llvm)) + Self::serialize_module(module) } fn serialize_module(module: ModuleCodegen) -> (String, Self::ModuleBuffer) { - (module.name, SpirvModuleBuffer(module.module_llvm)) + ( + module.name, + SpirvModuleBuffer(module.module_llvm.assemble()), + ) } fn autodiff( @@ -424,7 +457,7 @@ impl WriteBackendMethods for SpirvCodegenBackend { _diff_fncs: Vec, _config: &ModuleConfig, ) -> Result<(), FatalError> { - todo!() + unreachable!("Rust-GPU does not support autodiff") } } @@ -490,12 +523,11 @@ impl ExtraBackendMethods for SpirvCodegenBackend { } else { with_no_trimmed_paths!(do_codegen(&mut cx)); } - let spirv_module = cx.finalize_module().assemble(); ( ModuleCodegen { name: cgu_name.to_string(), - module_llvm: spirv_module, + module_llvm: cx.finalize_module(), kind: ModuleKind::Regular, thin_lto_buffer: None, }, diff --git a/crates/rustc_codegen_spirv/src/link.rs b/crates/rustc_codegen_spirv/src/link.rs index 540d8a2841..30c45f433a 100644 --- a/crates/rustc_codegen_spirv/src/link.rs +++ b/crates/rustc_codegen_spirv/src/link.rs @@ -2,7 +2,7 @@ use crate::maybe_pqp_cg_ssa as rustc_codegen_ssa; use crate::codegen_cx::{CodegenArgs, SpirvMetadata}; -use crate::{SpirvCodegenBackend, SpirvModuleBuffer, SpirvThinBuffer, linker}; +use crate::{SpirvCodegenBackend, SpirvModuleBuffer, linker}; use ar::{Archive, GnuBuilder, Header}; use rspirv::binary::Assemble; use rspirv::dr::Module; @@ -548,6 +548,16 @@ fn create_archive(files: &[&Path], metadata: &[u8], out_filename: &Path) { builder.into_inner().unwrap(); } +// HACK(eddyb) hiding the actual implementation to avoid `rspirv::dr::Loader` +// being hardcoded (as future work may need to customize it for various reasons). +pub fn with_rspirv_loader( + f: impl FnOnce(&mut dyn rspirv::binary::Consumer) -> Result<(), E>, +) -> Result { + let mut loader = rspirv::dr::Loader::new(); + f(&mut loader)?; + Ok(loader.module()) +} + /// This is the actual guts of linking: the rest of the link-related functions are just digging through rustc's /// shenanigans to collect all the object files we need to link. fn do_link( @@ -562,11 +572,8 @@ fn do_link( let mut modules = Vec::new(); let mut add_module = |file_name: &OsStr, bytes: &[u8]| { - let module = { - let mut loader = rspirv::dr::Loader::new(); - rspirv::binary::parse_bytes(bytes, &mut loader).unwrap(); - loader.module() - }; + let module = + with_rspirv_loader(|loader| rspirv::binary::parse_bytes(bytes, loader)).unwrap(); if let Some(dir) = &cg_args.dump_pre_link { // FIXME(eddyb) is it a good idea to re-`assemble` the `rspirv::dr` // module, or should this just save the original bytes? @@ -625,7 +632,7 @@ fn do_link( // TODO: WorkProduct impl pub(crate) fn run_thin( cgcx: &CodegenContext, - modules: Vec<(String, SpirvThinBuffer)>, + modules: Vec<(String, SpirvModuleBuffer)>, cached_modules: Vec<(SerializedModule, WorkProduct)>, ) -> Result<(Vec>, Vec), FatalError> { if cgcx.opts.cg.linker_plugin_lto.enabled() { diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 5d510755df..09b4db481c 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -22,8 +22,8 @@ use crate::codegen_cx::{ModuleOutputType, SpirvMetadata}; use crate::custom_decorations::{CustomDecoration, SrcLocDecoration, ZombieDecoration}; use crate::custom_insts; use either::Either; -use rspirv::binary::{Assemble, Consumer}; -use rspirv::dr::{Block, Loader, Module, ModuleHeader, Operand}; +use rspirv::binary::Assemble; +use rspirv::dr::{Block, Module, ModuleHeader, Operand}; use rspirv::spirv::{Op, StorageClass, Word}; use rustc_data_structures::fx::FxHashMap; use rustc_errors::ErrorGuaranteed; @@ -255,15 +255,21 @@ pub fn link( } // merge the binaries - let mut loader = Loader::new(); - - for module in inputs { - module.all_inst_iter().for_each(|inst| { - loader.consume_instruction(inst.clone()); - }); - } + let mut output = crate::link::with_rspirv_loader(|loader| { + for module in inputs { + for inst in module.all_inst_iter() { + use rspirv::binary::ParseAction; + match loader.consume_instruction(inst.clone()) { + ParseAction::Continue => {} + ParseAction::Stop => unreachable!(), + ParseAction::Error(err) => return Err(err), + } + } + } + Ok(()) + }) + .unwrap(); - let mut output = loader.module(); let mut header = ModuleHeader::new(bound + 1); header.set_version(version.0, version.1); header.generator = 0x001B_0000; @@ -583,9 +589,10 @@ pub fn link( // FIXME(eddyb) dump both SPIR-T and `spv_words` if there's an error here. output = { let _timer = sess.timer("parse-spv_words-from-spirt"); - let mut loader = Loader::new(); - rspirv::binary::parse_words(&spv_words, &mut loader).unwrap(); - loader.module() + crate::link::with_rspirv_loader(|loader| { + rspirv::binary::parse_words(&spv_words, loader) + }) + .unwrap() }; } diff --git a/crates/rustc_codegen_spirv/src/linker/test.rs b/crates/rustc_codegen_spirv/src/linker/test.rs index d5316f2f60..07ae32c67d 100644 --- a/crates/rustc_codegen_spirv/src/linker/test.rs +++ b/crates/rustc_codegen_spirv/src/linker/test.rs @@ -1,5 +1,5 @@ use super::{LinkResult, link}; -use rspirv::dr::{Loader, Module}; +use rspirv::dr::Module; use rustc_errors::registry::Registry; use rustc_session::CompilerIO; use rustc_session::config::{Input, OutputFilenames, OutputTypes}; @@ -59,9 +59,7 @@ fn validate(spirv: &[u32]) { } fn load(bytes: &[u8]) -> Module { - let mut loader = Loader::new(); - rspirv::binary::parse_bytes(bytes, &mut loader).unwrap(); - loader.module() + crate::link::with_rspirv_loader(|loader| rspirv::binary::parse_bytes(bytes, loader)).unwrap() } // FIXME(eddyb) shouldn't this be named just `link`? (`assemble_spirv` is separate) From 7ac082b295583c43035a76351169d8fbd427de3d Mon Sep 17 00:00:00 2001 From: Eduard-Mihai Burtescu Date: Fri, 25 Jul 2025 20:44:42 +0300 Subject: [PATCH 4/4] Run DCE in per-crate `optimize`, too (before saving SPIR-V for `.rlib`s/linking/etc.). --- crates/rustc_codegen_spirv/src/lib.rs | 13 ++++++++++--- crates/rustc_codegen_spirv/src/linker/mod.rs | 9 +-------- 2 files changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/rustc_codegen_spirv/src/lib.rs b/crates/rustc_codegen_spirv/src/lib.rs index 6d0da56427..3e57d8648d 100644 --- a/crates/rustc_codegen_spirv/src/lib.rs +++ b/crates/rustc_codegen_spirv/src/lib.rs @@ -309,10 +309,17 @@ impl ThinBufferMethods for SpirvModuleBuffer { impl SpirvCodegenBackend { fn optimize_common( _cgcx: &CodegenContext, - _module: &mut ModuleCodegen<::Module>, + module: &mut ModuleCodegen<::Module>, ) -> Result<(), FatalError> { - // FIXME(eddyb) actually run as many optimization passes as possible, - // before ever serializing `.spv` files that will later get linked. + // Apply DCE ("dead code elimination") to modules before ever serializing + // them as `.spv` files (technically, `.rcgu.o` files inside `.rlib`s), + // that will later get linked (potentially many times, esp. if this is + // some big upstream library, e.g. `core` itself), and will therefore + // benefit from not having to clean up all sorts of unreachable helpers. + linker::dce::dce(&mut module.module_llvm); + + // FIXME(eddyb) run as many optimization passes as possible, not just DCE. + Ok(()) } } diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 09b4db481c..ddde1561f2 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -1,7 +1,7 @@ #[cfg(test)] mod test; -mod dce; +pub(crate) mod dce; mod destructure_composites; mod duplicates; mod entry_interface; @@ -277,13 +277,6 @@ pub fn link( output }; - // FIXME(eddyb) do this before ever saving the original `.spv`s to disk. - { - let _timer = sess.timer("link_dce-post-merge"); - dce::dce(&mut output); - } - - // HACK(eddyb) this has to be after DCE, to not break SPIR-T w/ dead decorations. if let Some(dir) = &opts.dump_post_merge { dump_spv_and_spirt(&output, dir.join(disambiguated_crate_name_for_dumps)); }