diff --git a/CHANGELOG.md b/CHANGELOG.md index f5ed85dc6b..7aa18accd4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -46,6 +46,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 (see its documentation for more details about each available panic handling strategy) ### Changed 🛠 +- [PR#1083](https://github.com/EmbarkStudios/rust-gpu/pull/1083) updated SPIR-T to get pretty-printer + improvements (especially for `OpExtInst`, including Rust-GPU's custom ones), and started more + aggressively deduplicating custom debuginfo instructions (to make SPIR-T dumps more readable) - [PR#1079](https://github.com/EmbarkStudios/rust-gpu/pull/1079) revised `spirv-builder`'s `README.md`, and added a way for `docs.rs` to be able to build it (via `cargo +stable doc --no-default-features`) - [PR#1070](https://github.com/EmbarkStudios/rust-gpu/pull/1070) made panics (via the `abort` intrinsic) diff --git a/Cargo.lock b/Cargo.lock index 71873d0fc0..065dfe5ca2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -462,6 +462,12 @@ dependencies = [ "web-sys", ] +[[package]] +name = "convert_case" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6245d59a3e82a7fc217c5828a6692dbc6dfb63a0c8c90495621f7b9d79704a0e" + [[package]] name = "core-foundation" version = "0.9.3" @@ -594,6 +600,19 @@ dependencies = [ "syn", ] +[[package]] +name = "derive_more" +version = "0.99.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "4fb810d30a7c1953f91334de7244731fc3f3c10d7fe163338a35b9f640960321" +dependencies = [ + "convert_case", + "proc-macro2", + "quote", + "rustc_version", + "syn", +] + [[package]] name = "diff" version = "0.1.12" @@ -2032,6 +2051,15 @@ dependencies = [ "serde", ] +[[package]] +name = "rustc_version" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bfa0f585226d2e68097d4f95d113b15b83a82e819ab25717ec0590d9584ef366" +dependencies = [ + "semver", +] + [[package]] name = "rustfix" version = "0.6.1" @@ -2145,6 +2173,12 @@ dependencies = [ "version-compare", ] +[[package]] +name = "semver" +version = "1.0.17" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "bebd363326d05ec3e2f532ab7660680f3b02130d780c299bca73469d521bc0ed" + [[package]] name = "serde" version = "1.0.156" @@ -2246,11 +2280,11 @@ dependencies = [ [[package]] name = "spirt" version = "0.2.0" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e24fa996f12f3c667efbceaa99c222b8910a295a14d2c43c3880dfab2752def7" +source = "git+https://github.com/EmbarkStudios/spirt?branch=main#82daf2516710504986cdc35e0e27455326aeef90" dependencies = [ "arrayvec", "bytemuck", + "derive_more", "elsa", "indexmap", "internal-iterator", diff --git a/Cargo.toml b/Cargo.toml index 2a729d897a..d3d4a5886d 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -52,3 +52,6 @@ codegen-units = 256 opt-level = 3 incremental = true codegen-units = 256 + +[patch.crates-io] +spirt = { git = "https://github.com/EmbarkStudios/spirt", branch = "main" } diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index ef92f02da3..a828d39d55 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -708,6 +708,23 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { }, ); } + + // HACK(eddyb) remove the previous instruction if made irrelevant. + let mut builder = self.emit(); + if let (Some(func_idx), Some(block_idx)) = + (builder.selected_function(), builder.selected_block()) + { + let block = &mut builder.module_mut().functions[func_idx].blocks[block_idx]; + match &block.instructions[..] { + [.., a, b] + if a.class.opcode == b.class.opcode + && a.operands[..2] == b.operands[..2] => + { + block.instructions.remove(block.instructions.len() - 2); + } + _ => {} + } + } } else { // We may not always have valid spans. // FIXME(eddyb) reduce the sources of this as much as possible. @@ -2878,11 +2895,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { // HACK(eddyb) redirect any possible panic call to an abort, to avoid // needing to materialize `&core::panic::Location` or `format_args!`. - self.abort_with_message_and_debug_printf_args( - // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. - format!("panicked|{message}"), - debug_printf_args, - ); + self.abort_with_kind_and_message_debug_printf("panic", message, debug_printf_args); self.undef(result_type) } else if let Some(mode) = buffer_load_intrinsic { self.codegen_buffer_load_intrinsic(result_type, args, mode) diff --git a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs index 982e47f13d..8089bd09b3 100644 --- a/crates/rustc_codegen_spirv/src/builder/intrinsics.rs +++ b/crates/rustc_codegen_spirv/src/builder/intrinsics.rs @@ -339,11 +339,7 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } fn abort(&mut self) { - // HACK(eddyb) `|` is an ad-hoc convention of `linker::spirt_passes::controlflow`. - self.abort_with_message_and_debug_printf_args( - "aborted|intrinsics::abort() called".into(), - [], - ); + self.abort_with_kind_and_message_debug_printf("abort", "intrinsics::abort() called", []); } fn assume(&mut self, _val: Self::Value) { @@ -378,24 +374,31 @@ impl<'a, 'tcx> IntrinsicCallMethods<'tcx> for Builder<'a, 'tcx> { } impl Builder<'_, '_> { - pub fn abort_with_message_and_debug_printf_args( + pub fn abort_with_kind_and_message_debug_printf( &mut self, - message: String, - debug_printf_args: impl IntoIterator, + kind: &str, + message_debug_printf_fmt_str: impl Into, + message_debug_printf_args: impl IntoIterator, ) { // FIXME(eddyb) this should be cached more efficiently. let void_ty = SpirvType::Void.def(rustc_span::DUMMY_SP, self); // HACK(eddyb) there is no `abort` or `trap` instruction in SPIR-V, // so the best thing we can do is use our own custom instruction. - let message_id = self.emit().string(message); + let kind_id = self.emit().string(kind); + let message_debug_printf_fmt_str_id = self.emit().string(message_debug_printf_fmt_str); self.custom_inst( void_ty, CustomInst::Abort { - message: Operand::IdRef(message_id), - debug_printf_args: debug_printf_args + kind: Operand::IdRef(kind_id), + message_debug_printf: [message_debug_printf_fmt_str_id] .into_iter() - .map(|arg| Operand::IdRef(arg.def(self))) + .chain( + message_debug_printf_args + .into_iter() + .map(|arg| arg.def(self)), + ) + .map(Operand::IdRef) .collect(), }, ); diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index 04e3942a42..362f89b209 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -378,8 +378,13 @@ impl CodegenArgs { ); opts.optflag( "", - "spirt-keep-custom-debuginfo-in-dumps", - "keep custom debuginfo when dumping SPIR-T (instead of lossily prettifying it)", + "spirt-strip-custom-debuginfo-from-dumps", + "strip custom debuginfo instructions when dumping SPIR-T", + ); + opts.optflag( + "", + "spirt-keep-debug-sources-in-dumps", + "keep file contents debuginfo when dumping SPIR-T", ); opts.optflag( "", @@ -548,8 +553,10 @@ impl CodegenArgs { dump_post_merge: matches_opt_dump_dir_path("dump-post-merge"), dump_post_split: matches_opt_dump_dir_path("dump-post-split"), dump_spirt_passes: matches_opt_dump_dir_path("dump-spirt-passes"), - spirt_keep_custom_debuginfo_in_dumps: matches - .opt_present("spirt-keep-custom-debuginfo-in-dumps"), + spirt_strip_custom_debuginfo_from_dumps: matches + .opt_present("spirt-strip-custom-debuginfo-from-dumps"), + spirt_keep_debug_sources_in_dumps: matches + .opt_present("spirt-keep-debug-sources-in-dumps"), specializer_debug: matches.opt_present("specializer-debug"), specializer_dump_instances: matches_opt_path("specializer-dump-instances"), print_all_zombie: matches.opt_present("print-all-zombie"), diff --git a/crates/rustc_codegen_spirv/src/custom_insts.rs b/crates/rustc_codegen_spirv/src/custom_insts.rs index 4ec02c5baa..d5721c2485 100644 --- a/crates/rustc_codegen_spirv/src/custom_insts.rs +++ b/crates/rustc_codegen_spirv/src/custom_insts.rs @@ -15,6 +15,18 @@ use smallvec::SmallVec; /// See `CUSTOM_EXT_INST_SET`'s docs for further constraints on the full name. pub const CUSTOM_EXT_INST_SET_PREFIX: &str = concat!("Rust.", env!("CARGO_PKG_NAME"), "."); +macro_rules! join_cargo_pkg_version_major_minor_patch { + ($sep:literal) => { + concat!( + env!("CARGO_PKG_VERSION_MAJOR"), + $sep, + env!("CARGO_PKG_VERSION_MINOR"), + $sep, + env!("CARGO_PKG_VERSION_PATCH"), + ) + }; +} + lazy_static! { /// `OpExtInstImport` "instruction set" name for all Rust-GPU instructions. /// @@ -30,10 +42,6 @@ lazy_static! { /// if the definitions of the custom instructions have changed - this is /// achieved by hashing the `SCHEMA` constant from `def_custom_insts!` below pub static ref CUSTOM_EXT_INST_SET: String = { - const VER_MAJOR: &str = env!("CARGO_PKG_VERSION_MAJOR"); - const VER_MINOR: &str = env!("CARGO_PKG_VERSION_MINOR"); - const VER_PATCH: &str = env!("CARGO_PKG_VERSION_PATCH"); - let schema_hash = { use rustc_data_structures::stable_hasher::StableHasher; use std::hash::Hash; @@ -43,11 +51,47 @@ lazy_static! { let (lo, hi) = hasher.finalize(); (lo as u128) | ((hi as u128) << 64) }; - - format!("{CUSTOM_EXT_INST_SET_PREFIX}{VER_MAJOR}_{VER_MINOR}_{VER_PATCH}.{schema_hash:x}") + let version = join_cargo_pkg_version_major_minor_patch!("_"); + format!("{CUSTOM_EXT_INST_SET_PREFIX}{version}.{schema_hash:x}") }; } +pub fn register_to_spirt_context(cx: &spirt::Context) { + use spirt::spv::spec::{ExtInstSetDesc, ExtInstSetInstructionDesc}; + cx.register_custom_ext_inst_set( + &CUSTOM_EXT_INST_SET, + ExtInstSetDesc { + // HACK(eddyb) this is the most compact form I've found, that isn't + // outright lossy by omitting "Rust vs Rust-GPU" or the version. + short_alias: Some( + concat!("Rust-GPU ", join_cargo_pkg_version_major_minor_patch!(".")).into(), + ), + instructions: SCHEMA + .iter() + .map(|&(i, name, operand_names)| { + ( + i, + ExtInstSetInstructionDesc { + name: name.into(), + operand_names: operand_names + .iter() + .map(|name| { + name.strip_prefix("..") + .unwrap_or(name) + .replace('_', " ") + .into() + }) + .collect(), + is_debuginfo: name.contains("Debug") + || name.contains("InlinedCallFrame"), + }, + ) + }) + .collect(), + }, + ); +} + macro_rules! def_custom_insts { ($($num:literal => $name:ident $({ $($field:ident),+ $(, ..$variadic_field:ident)? $(,)? })?),+ $(,)?) => { const SCHEMA: &[(u32, &str, &[&str])] = &[ @@ -151,9 +195,10 @@ def_custom_insts! { // users to do `catch_unwind` at the top-level of their shader to handle // panics specially (e.g. by appending to a custom buffer, or using some // specific color in a fragment shader, to indicate a panic happened). - // NOTE(eddyb) the `message` string follows `debugPrintf` rules, with remaining - // operands (collected into `debug_printf_args`) being its formatting arguments. - 4 => Abort { message, ..debug_printf_args }, + // NOTE(eddyb) `message_debug_printf` operands form a complete `debugPrintf` + // invocation (format string followed by inputs) for the "message", while + // `kind` only distinguishes broad categories like `"abort"` vs `"panic"`. + 4 => Abort { kind, ..message_debug_printf }, } impl CustomOp { diff --git a/crates/rustc_codegen_spirv/src/link.rs b/crates/rustc_codegen_spirv/src/link.rs index 84ee4db4d5..aba8f3c366 100644 --- a/crates/rustc_codegen_spirv/src/link.rs +++ b/crates/rustc_codegen_spirv/src/link.rs @@ -78,6 +78,7 @@ pub fn link( crate_type, &out_filename, codegen_results, + outputs, &disambiguated_crate_name_for_dumps, ); } @@ -123,6 +124,7 @@ fn link_exe( crate_type: CrateType, out_filename: &Path, codegen_results: &CodegenResults, + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) { let mut objects = Vec::new(); @@ -152,6 +154,7 @@ fn link_exe( &cg_args, &objects, &rlibs, + outputs, disambiguated_crate_name_for_dumps, ); let compile_result = match link_result { @@ -517,6 +520,7 @@ fn do_link( cg_args: &CodegenArgs, objects: &[PathBuf], rlibs: &[PathBuf], + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) -> linker::LinkResult { let load_modules_timer = sess.timer("link_load_modules"); @@ -570,6 +574,7 @@ fn do_link( sess, modules, &cg_args.linker_opts, + outputs, disambiguated_crate_name_for_dumps, ); diff --git a/crates/rustc_codegen_spirv/src/linker/duplicates.rs b/crates/rustc_codegen_spirv/src/linker/duplicates.rs index 24ccfe8802..3ca05022f0 100644 --- a/crates/rustc_codegen_spirv/src/linker/duplicates.rs +++ b/crates/rustc_codegen_spirv/src/linker/duplicates.rs @@ -1,9 +1,12 @@ +use crate::custom_insts::{self, CustomOp}; use rspirv::binary::Assemble; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Op, Word}; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_middle::bug; +use smallvec::SmallVec; use std::collections::hash_map; +use std::mem; // FIXME(eddyb) consider deduplicating the `OpString` and `OpSource` created for // file-level debuginfo (but using SPIR-T for linking might be better?). @@ -269,19 +272,272 @@ pub fn remove_duplicate_types(module: &mut Module) { }); } -pub fn remove_duplicate_lines(module: &mut Module) { +pub fn remove_duplicate_debuginfo(module: &mut Module) { + // FIXME(eddyb) avoid repeating this across different passes/helpers. + let custom_ext_inst_set_import = module + .ext_inst_imports + .iter() + .find(|inst| { + assert_eq!(inst.class.opcode, Op::ExtInstImport); + inst.operands[0].unwrap_literal_string() == &custom_insts::CUSTOM_EXT_INST_SET[..] + }) + .map(|inst| inst.result_id.unwrap()); + for func in &mut module.functions { for block in &mut func.blocks { - block.instructions.dedup_by(|a, b| { - if a.class.opcode == Op::Line && b.class.opcode == Op::Line { - // dedup_by removes the *second* element in a pair of duplicates. We want to - // remove the *first* (so the last OpLine is kept). So, swap them! :D - std::mem::swap(a, b); - true - } else { - false + // Ignore the terminator, it's effectively "outside" debuginfo. + let (_, insts) = block.instructions.split_last_mut().unwrap(); + + // HACK(eddyb) to make random access easier, we first replace unused + // instructions with `OpNop`, and then remove all the `OpNop`s. + + #[derive(Clone)] + struct DbgLocInst { + inst_idx: usize, + used: bool, + } + + fn nop() -> Instruction { + Instruction::new(Op::Nop, None, None, vec![]) + } + impl DbgLocInst { + fn nop_if_unused(&self, insts: &mut [Instruction]) { + if !self.used { + insts[self.inst_idx] = nop(); + } + } + } + + #[derive(Clone, Default)] + struct DbgState { + loc: Option, + has_semantic_insts: bool, + } + let mut dbg = DbgState::default(); + + struct Frame { + call_dbg: DbgState, + push_inst_idx: usize, + } + let mut inlined_frames = SmallVec::<[Frame; 8]>::new(); + + // HACK(eddyb) `PopInlinedCallFrame` moves `inlined_frames.last()` + // `fusable_freshly_popped_inlined_frames.last()`, so a sequence of + // N pops will reverse the N last entries of `inlined_frames` into + // this vector (and go from outside-in, to inside-out), which allows + // *fusing* a pop with a push (of an identical inlined frame), when + // no interverning instructions prevent it (such instructions will + // clear this vector to indicate the pops are "fully committed"). + struct PoppedFrame { + frame: Frame, + callee_has_semantic_insts: bool, + pop_inst_idx: usize, + } + let mut fusable_freshly_popped_inlined_frames = SmallVec::<[PoppedFrame; 8]>::new(); + + for inst_idx in 0..insts.len() { + let inst = &insts[inst_idx]; + let custom_op = match inst.class.opcode { + Op::ExtInst + if Some(inst.operands[0].unwrap_id_ref()) == custom_ext_inst_set_import => + { + Some(CustomOp::decode_from_ext_inst(inst)) + } + _ => None, + }; + + fn inst_eq_key(inst: &Instruction) -> impl PartialEq + '_ { + (inst.class.opcode, &inst.operands) } - }); + + // NOTE(eddyb) `fusable_freshly_popped_inlined_frames`-preserving + // cases must all use `if can_continue { continue; }` to skip the + // draining logic (`can_continue` is only `false` at the very end). + let can_continue = inst_idx < insts.len() - 1; + let prev_dbg_loc_snapshot = dbg.loc.clone(); + match (inst.class.opcode, custom_op) { + (Op::Line | Op::NoLine, _) + | (_, Some(CustomOp::SetDebugSrcLoc | CustomOp::ClearDebugSrcLoc)) => { + // HACK(eddyb) prefer keeping older active `DbgLocInst`s, + // if all the details are the same (it helps with fusion). + if dbg.loc.as_ref().is_some_and(|old_dbg_loc| { + inst_eq_key(inst) == inst_eq_key(&insts[old_dbg_loc.inst_idx]) + }) { + insts[inst_idx] = nop(); + if can_continue { + continue; + } + } else { + dbg.loc = Some(DbgLocInst { + inst_idx, + used: false, + }); + } + } + (_, Some(CustomOp::PushInlinedCallFrame)) => { + // HACK(eddyb) attempt fusing this push with the last pop. + let fuse_with_last_pop = fusable_freshly_popped_inlined_frames + .last() + .is_some_and(|last_popped| { + // HACK(eddyb) updating `dbg.loc` deduplicates eagerly, + // so here it suffices to check the (deduped) indices. + let dbg_loc_inst_idx = + |dbg: &DbgState| dbg.loc.as_ref().map(|d| d.inst_idx); + dbg_loc_inst_idx(&last_popped.frame.call_dbg) + == dbg_loc_inst_idx(&dbg) + && inst_eq_key(inst) + == inst_eq_key(&insts[last_popped.frame.push_inst_idx]) + }); + if fuse_with_last_pop { + let PoppedFrame { + frame, + callee_has_semantic_insts, + pop_inst_idx, + } = fusable_freshly_popped_inlined_frames.pop().unwrap(); + + insts[pop_inst_idx] = nop(); + + // Can't make entering an inlined function a nop, + // as it needs to reset callee-side `DbgLocInst`, + // but we can replace it in-place and hope later + // it get nop'd out by some real `DbgLocInst`. + insts[inst_idx].operands.splice( + 1.., + [Operand::LiteralExtInstInteger( + CustomOp::ClearDebugSrcLoc as u32, + )], + ); + dbg = DbgState { + loc: Some(DbgLocInst { + inst_idx, + used: false, + }), + has_semantic_insts: callee_has_semantic_insts, + }; + + inlined_frames.push(frame); + + // Allow further fusing to occur. + if can_continue { + continue; + } + } else { + // HACK(eddyb) the actual push to `inlined_frames` is + // done at the very end of the loop body, to be able + // to process any pending updates on the previous state. + } + } + (_, Some(CustomOp::PopInlinedCallFrame)) => { + // Leaving an inlined function doesn't use `DbgLocInst`. + if let Some(dbg_loc) = dbg.loc.take() { + // HACK(eddyb) only treat as "definitely unused" + // instructions that are too "recent" to have been + // used by a `PushInlinedCallFrame` with a still + // uncommitted `PopInlinedCallFrame`. + let min_safe_inst_idx_to_nop = fusable_freshly_popped_inlined_frames + .last() + .map_or(0, |last_popped| last_popped.pop_inst_idx); + if dbg_loc.inst_idx > min_safe_inst_idx_to_nop { + dbg_loc.nop_if_unused(insts); + } + } + if let Some(frame) = inlined_frames.pop() { + let callee_has_semantic_insts = dbg.has_semantic_insts; + dbg = frame.call_dbg.clone(); + dbg.has_semantic_insts |= callee_has_semantic_insts; + + // HACK(eddyb) inform future `PushInlinedCallFrame`s + // of potential fusion, by saving a copy of the frame. + fusable_freshly_popped_inlined_frames.push(PoppedFrame { + frame, + callee_has_semantic_insts, + pop_inst_idx: inst_idx, + }); + } else { + // FIXME(eddyb) this may indicate a bug elsewhere. + insts[inst_idx] = nop(); + } + if can_continue { + continue; + } + } + _ => { + if let Some(dbg_loc) = &mut dbg.loc { + dbg_loc.used = true; + } + dbg.has_semantic_insts = true; + } + } + + // NOTE(eddyb) mutable so that it may be marked as used below. + let mut freshly_replaced_dbg_loc = prev_dbg_loc_snapshot.filter(|prev_dbg_loc| { + dbg.loc.as_ref().map(|d| d.inst_idx) != Some(prev_dbg_loc.inst_idx) + }); + + // NOTE(eddyb) the iteration order doesn't matter, as this is + // effectively a set of `PopInlinedCallFrame`s which have had + // all their other side-effects processed, and didn't get a + // chance to be fused away, so they're getting committed. + for popped in fusable_freshly_popped_inlined_frames.drain(..) { + let PoppedFrame { + mut frame, + callee_has_semantic_insts, + pop_inst_idx, + } = popped; + + // HACK(eddyb) this popped frame's `call_dbg.loc` may still + // be used elsewhere, in which case that use takes precedence, + // and is effectively the new "owner" of the `DbgLocInst`. + let call_dbg_loc_used_elsewhere = + frame.call_dbg.loc.as_ref().and_then(|call_dbg_loc| { + [dbg.loc.as_mut(), freshly_replaced_dbg_loc.as_mut()] + .into_iter() + .flatten() + .find(|dbg_loc| dbg_loc.inst_idx == call_dbg_loc.inst_idx) + }); + if call_dbg_loc_used_elsewhere.is_some() { + frame.call_dbg.loc = None; + } + + if callee_has_semantic_insts { + // The `PushInlinedCallFrame` being kept requires its + // original `DbgLocInst` to also be kept around. + if let Some(call_dbg_loc) = call_dbg_loc_used_elsewhere { + call_dbg_loc.used = true; + } + } else { + // If the entire inlined call is all `OpNop`s now, + // entering/leaving it can also become `OpNop`s. + if let Some(call_dbg_loc) = &mut frame.call_dbg.loc { + call_dbg_loc.nop_if_unused(insts); + } + insts[frame.push_inst_idx] = nop(); + insts[pop_inst_idx] = nop(); + } + } + + // Only remove a replaced `DbgLocInst` after it had a chance to + // be marked as used above (for e.g. a `PushInlinedCallFrame`). + if let Some(old_dbg_loc) = freshly_replaced_dbg_loc { + old_dbg_loc.nop_if_unused(insts); + } + + // HACK(eddyb) the actual push to `inlined_frames` is + // done at the very end of the loop body, to be able + // to process any pending updates on the previous state. + if custom_op == Some(CustomOp::PushInlinedCallFrame) { + inlined_frames.push(Frame { + call_dbg: mem::take(&mut dbg), + push_inst_idx: inst_idx, + }); + } + } + + assert!(fusable_freshly_popped_inlined_frames.is_empty()); + + block + .instructions + .retain(|inst| inst.class.opcode != Op::Nop); } } } diff --git a/crates/rustc_codegen_spirv/src/linker/inline.rs b/crates/rustc_codegen_spirv/src/linker/inline.rs index 736e92c259..c036464ade 100644 --- a/crates/rustc_codegen_spirv/src/linker/inline.rs +++ b/crates/rustc_codegen_spirv/src/linker/inline.rs @@ -919,55 +919,67 @@ impl Inliner<'_, '_> { let mut blocks = callee.blocks.clone(); for block in &mut blocks { - let last = block.instructions.last().unwrap(); - if let Op::Return | Op::ReturnValue = last.class.opcode { - if Op::ReturnValue == last.class.opcode { - let return_value = last.operands[0].id_ref_any().unwrap(); - block.instructions.insert( - block.instructions.len() - 1, - Instruction::new( - Op::Store, - None, - None, - vec![ - Operand::IdRef(return_variable.unwrap()), - Operand::IdRef(return_value), - ], - ), - ); + let mut terminator = block.instructions.pop().unwrap(); + + // HACK(eddyb) strip trailing debuginfo (as it can't impact terminators). + while let Some(last) = block.instructions.last() { + let can_remove = match last.class.opcode { + Op::Line | Op::NoLine => true, + Op::ExtInst => { + last.operands[0].unwrap_id_ref() == custom_ext_inst_set_import + && matches!( + CustomOp::decode_from_ext_inst(last), + CustomOp::SetDebugSrcLoc | CustomOp::ClearDebugSrcLoc + ) + } + _ => false, + }; + if can_remove { + block.instructions.pop(); + } else { + break; + } + } + + if let Op::Return | Op::ReturnValue = terminator.class.opcode { + if Op::ReturnValue == terminator.class.opcode { + let return_value = terminator.operands[0].id_ref_any().unwrap(); + block.instructions.push(Instruction::new( + Op::Store, + None, + None, + vec![ + Operand::IdRef(return_variable.unwrap()), + Operand::IdRef(return_value), + ], + )); } else { assert!(return_variable.is_none()); } - *block.instructions.last_mut().unwrap() = + terminator = Instruction::new(Op::Branch, None, None, vec![Operand::IdRef(return_jump)]); } - let (debuginfo_prefix, debuginfo_suffix) = mk_debuginfo_prefix_and_suffix(true); - block.instructions.splice( + let num_phis = block + .instructions + .iter() + .take_while(|inst| inst.class.opcode == Op::Phi) + .count(); + + // HACK(eddyb) avoid adding debuginfo to otherwise-empty blocks. + if block.instructions.len() > num_phis { + let (debuginfo_prefix, debuginfo_suffix) = mk_debuginfo_prefix_and_suffix(true); // Insert the prefix debuginfo instructions after `OpPhi`s, // which sadly can't be covered by them. - { - let i = block - .instructions - .iter() - .position(|inst| inst.class.opcode != Op::Phi) - .unwrap(); - i..i - }, - debuginfo_prefix, - ); - block.instructions.splice( + block + .instructions + .splice(num_phis..num_phis, debuginfo_prefix); // Insert the suffix debuginfo instructions before the terminator, // which sadly can't be covered by them. - { - let last_non_terminator = block.instructions.iter().rposition(|inst| { - !rspirv::grammar::reflect::is_block_terminator(inst.class.opcode) - }); - let i = last_non_terminator.map_or(0, |x| x + 1); - i..i - }, - debuginfo_suffix, - ); + block.instructions.extend(debuginfo_suffix); + } + + block.instructions.push(terminator); } let (caller_restore_debuginfo_after_call, calleer_reset_debuginfo_before_call) = diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 1ef6509eed..3606779ee9 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -27,6 +27,7 @@ use rspirv::dr::{Block, Instruction, Loader, Module, ModuleHeader, Operand}; use rspirv::spirv::{Op, StorageClass, Word}; use rustc_data_structures::fx::FxHashMap; use rustc_errors::ErrorGuaranteed; +use rustc_session::config::OutputFilenames; use rustc_session::Session; use std::collections::BTreeMap; use std::ffi::{OsStr, OsString}; @@ -59,7 +60,8 @@ pub struct Options { pub dump_post_merge: Option, pub dump_post_split: Option, pub dump_spirt_passes: Option, - pub spirt_keep_custom_debuginfo_in_dumps: bool, + pub spirt_strip_custom_debuginfo_from_dumps: bool, + pub spirt_keep_debug_sources_in_dumps: bool, pub specializer_debug: bool, pub specializer_dump_instances: Option, pub print_all_zombie: bool, @@ -151,6 +153,7 @@ pub fn link( sess: &Session, mut inputs: Vec, opts: &Options, + outputs: &OutputFilenames, disambiguated_crate_name_for_dumps: &OsStr, ) -> Result { let mut output = { @@ -325,6 +328,13 @@ 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); + } + { let _timer = sess.timer("link_inline"); inline::inline(sess, &mut output)?; @@ -374,6 +384,13 @@ pub fn link( } } + if opts.dce { + let _timer = + sess.timer("link_dce-and-remove_duplicate_debuginfo-after-mem2reg-after-inlining"); + dce::dce(&mut output); + duplicates::remove_duplicate_debuginfo(&mut output); + } + // NOTE(eddyb) SPIR-T pipeline is entirely limited to this block. { let mut per_pass_module_for_dumping = vec![]; @@ -383,24 +400,35 @@ pub fn link( } }; + let spv_words; let spv_bytes = { let _timer = sess.timer("assemble-to-spv_bytes-for-spirt"); - spirv_tools::binary::from_binary(&output.assemble()).to_vec() + spv_words = output.assemble(); + // FIXME(eddyb) this is wastefully cloning all the bytes, but also + // `spirt::Module` should have a method that takes `Vec`. + spirv_tools::binary::from_binary(&spv_words).to_vec() }; let cx = std::rc::Rc::new(spirt::Context::new()); + crate::custom_insts::register_to_spirt_context(&cx); let mut module = { let _timer = sess.timer("spirt::Module::lower_from_spv_file"); match spirt::Module::lower_from_spv_bytes(cx.clone(), spv_bytes) { Ok(module) => module, Err(e) => { - use rspirv::binary::Disassemble; + let spv_path = outputs.temp_path_ext("spirt-lower-from-spv-input.spv", None); + + let was_saved_msg = match std::fs::write( + &spv_path, + spirv_tools::binary::from_binary(&spv_words), + ) { + Ok(()) => format!("was saved to {}", spv_path.display()), + Err(e) => format!("could not be saved: {e}"), + }; return Err(sess .struct_err(format!("{e}")) - .note(format!( - "while lowering this SPIR-V module to SPIR-T:\n{}", - output.disassemble() - )) + .note("while lowering SPIR-V module to SPIR-T (spirt::spv::lower)") + .note(format!("input SPIR-V module {was_saved_msg}")) .emit()); } } @@ -438,22 +466,52 @@ pub fn link( let _timer = sess.timer("spirt_passes::diagnostics::report_diagnostics"); spirt_passes::diagnostics::report_diagnostics(sess, opts, &module) }; + let any_spirt_bugs = report_diagnostics_result + .as_ref() + .err() + .map_or(false, |e| e.any_errors_were_spirt_bugs); - // NOTE(eddyb) this should be *before* `lift_to_spv` below, - // so if that fails, the dump could be used to debug it. - if let Some(dump_dir) = &opts.dump_spirt_passes { - let dump_spirt_file_path = dump_dir + let mut dump_spirt_file_path = opts.dump_spirt_passes.as_ref().map(|dump_dir| { + dump_dir .join(disambiguated_crate_name_for_dumps) - .with_extension("spirt"); + .with_extension("spirt") + }); - // HACK(eddyb) unless requested otherwise, clean up the pretty-printed - // SPIR-T output by converting our custom extended instructions, to - // standard SPIR-V debuginfo (which SPIR-T knows how to pretty-print). - if !opts.spirt_keep_custom_debuginfo_in_dumps { + // FIXME(eddyb) this won't allow seeing the individual passes, but it's + // better than nothing (we could theoretically put this whole block in + // a loop so that we redo everything but keeping `Module` clones?). + if any_spirt_bugs && dump_spirt_file_path.is_none() { + if per_pass_module_for_dumping.is_empty() { + per_pass_module_for_dumping.push(("", module.clone())); + } + dump_spirt_file_path = Some(outputs.temp_path_ext("spirt", None)); + } + + // NOTE(eddyb) this should be *before* `lift_to_spv` below, + // so if that fails, the dump could be used to debug it. + if let Some(dump_spirt_file_path) = &dump_spirt_file_path { + if opts.spirt_strip_custom_debuginfo_from_dumps { for (_, module) in &mut per_pass_module_for_dumping { spirt_passes::debuginfo::convert_custom_debuginfo_to_spv(module); } } + if !opts.spirt_keep_debug_sources_in_dumps { + for (_, module) in &mut per_pass_module_for_dumping { + let spirt::ModuleDebugInfo::Spv(debuginfo) = &mut module.debug_info; + for sources in debuginfo.source_languages.values_mut() { + const DOTS: &str = "⋯"; + for file in sources.file_contents.values_mut() { + *file = DOTS.into(); + } + sources.file_contents.insert( + cx.intern(DOTS), + "sources hidden, to show them use \ + `RUSTGPU_CODEGEN_ARGS=--spirt-keep-debug-sources-in-dumps`" + .into(), + ); + } + } + } let plan = spirt::print::Plan::for_versions( &cx, @@ -464,7 +522,7 @@ pub fn link( let pretty = plan.pretty_print(); // FIXME(eddyb) don't allocate whole `String`s here. - std::fs::write(&dump_spirt_file_path, pretty.to_string()).unwrap(); + std::fs::write(dump_spirt_file_path, pretty.to_string()).unwrap(); std::fs::write( dump_spirt_file_path.with_extension("spirt.html"), pretty @@ -475,6 +533,19 @@ pub fn link( .unwrap(); } + if any_spirt_bugs { + let mut note = sess.struct_note_without_error("SPIR-T bugs were reported"); + note.help(format!( + "pretty-printed SPIR-T was saved to {}.html", + dump_spirt_file_path.as_ref().unwrap().display() + )); + if opts.dump_spirt_passes.is_none() { + note.help("re-run with `RUSTGPU_CODEGEN_ARGS=\"--dump-spirt-passes=$PWD\"` for more details"); + } + note.note("pretty-printed SPIR-T is preferred when reporting Rust-GPU issues") + .emit(); + } + // NOTE(eddyb) this is late so that `--dump-spirt-passes` is processed, // even/especially when errors were reported, but lifting to SPIR-V is // skipped (since it could very well fail due to reported errors). @@ -629,8 +700,8 @@ pub fn link( } { - let _timer = sess.timer("link_remove_duplicate_lines"); - duplicates::remove_duplicate_lines(output); + let _timer = sess.timer("link_remove_duplicate_debuginfo"); + duplicates::remove_duplicate_debuginfo(output); } if opts.compact_ids { diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs index 21e263052a..5a173e43a4 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/controlflow.rs @@ -4,8 +4,8 @@ use crate::custom_insts::{self, CustomInst, CustomOp}; use smallvec::SmallVec; use spirt::func_at::FuncAt; use spirt::{ - cfg, spv, Attr, AttrSet, ConstCtor, ConstDef, ControlNodeKind, DataInstKind, DeclDef, - EntityDefs, ExportKey, Exportee, Module, Type, TypeCtor, TypeCtorArg, TypeDef, Value, + cfg, spv, Attr, AttrSet, ConstCtor, ConstDef, ControlNodeKind, DataInstFormDef, DataInstKind, + DeclDef, EntityDefs, ExportKey, Exportee, Module, Type, TypeCtor, TypeCtorArg, TypeDef, Value, }; use std::fmt::Write as _; @@ -108,14 +108,15 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( .into_iter() .filter_map(|func_at_inst| { let data_inst_def = func_at_inst.def(); - if let DataInstKind::SpvInst(spv_inst) = &data_inst_def.kind { + let data_inst_form_def = &cx[data_inst_def.form]; + if let DataInstKind::SpvInst(spv_inst) = &data_inst_form_def.kind { if spv_inst.opcode == wk.OpLoad { if let Value::Const(ct) = data_inst_def.inputs[0] { if let ConstCtor::PtrToGlobalVar(gv) = cx[ct].ctor { if interface_global_vars.contains(&gv) { return Some(( gv, - data_inst_def.output_type.unwrap(), + data_inst_form_def.output_type.unwrap(), Value::DataInstOutput(func_at_inst.position), )); } @@ -223,7 +224,7 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( let data_inst_def = func_at_inst.def(); ( func_at_inst, - match data_inst_def.kind { + match cx[data_inst_def.form].kind { DataInstKind::SpvExtInst { ext_set, inst } if ext_set == custom_ext_inst_set => { @@ -243,8 +244,8 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( if let Some(( func_at_abort_inst, CustomInst::Abort { - message, - debug_printf_args: message_debug_printf_args, + kind: abort_kind, + message_debug_printf, }, )) = custom_terminator_inst { @@ -334,26 +335,47 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( let mut fmt = String::new(); + let (message_debug_printf_fmt_str, message_debug_printf_args) = + message_debug_printf + .split_first() + .map(|(&fmt_str, args)| (&cx[const_str(fmt_str)], args)) + .unwrap_or_default(); + + let fmt_dbg_src_loc = |(file, line, col)| { + // FIXME(eddyb) figure out what is going on with + // these column number conventions, below is a + // related comment from `spirt::print`: + // > // HACK(eddyb) Rust-GPU's column numbers seem + // > // off-by-one wrt what e.g. VSCode expects + // > // for `:line:col` syntax, but it's hard to + // > // tell from the spec and `glslang` doesn't + // > // even emit column numbers at all! + let col = col + 1; + format!("{file}:{line}:{col}").replace('%', "%%") + }; + // HACK(eddyb) this improves readability w/ very verbose Vulkan loggers. fmt += "\n"; - fmt += "["; + fmt += "[Rust "; - // NB: `message` has its own `message_debug_printf_args` - // it formats, and as such any escaping is already done. - let message = &cx[const_str(message)]; - let (message_kind, message) = - message.split_once('|').unwrap_or(("", message)); + // HACK(eddyb) turn "panic" into "panicked", while the + // general case looks like "abort" -> "aborted". + match &cx[const_str(abort_kind)] { + "panic" => fmt += "panicked", + verb => { + fmt += verb; + fmt += "en"; + } + }; - if let Some((file, line, col)) = current_debug_src_loc.take() { - fmt += &format!("Rust {message_kind} at {file}:{line}:{col}") - .replace('%', "%%"); - } else { - fmt += message_kind; + if let Some(loc) = current_debug_src_loc.take() { + fmt += " at "; + fmt += &fmt_dbg_src_loc(loc); } fmt += "]\n "; - fmt += &message.replace('\n', "\n "); + fmt += &message_debug_printf_fmt_str.replace('\n', "\n "); let mut innermost = true; let mut append_call = |callsite_debug_src_loc, callee: &str| { @@ -367,9 +389,9 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( fmt += "\n called by "; } fmt += callee; - if let Some((file, line, col)) = callsite_debug_src_loc { - fmt += &format!("\n called at {file}:{line}:{col}") - .replace('%', "%%"); + if let Some(loc) = callsite_debug_src_loc { + fmt += "\n called at "; + fmt += &fmt_dbg_src_loc(loc); } current_debug_src_loc = callsite_debug_src_loc; }; @@ -381,13 +403,16 @@ pub fn convert_custom_aborts_to_unstructured_returns_in_entry_points( fmt += "\n"; let abort_inst_def = &mut func_def_body.data_insts[abort_inst]; - abort_inst_def.kind = DataInstKind::SpvExtInst { - ext_set: cx.intern("NonSemantic.DebugPrintf"), - inst: 1, - }; + abort_inst_def.form = cx.intern(DataInstFormDef { + kind: DataInstKind::SpvExtInst { + ext_set: cx.intern("NonSemantic.DebugPrintf"), + inst: 1, + }, + output_type: cx[abort_inst_def.form].output_type, + }); abort_inst_def.inputs = [Value::Const(mk_const_str(cx.intern(fmt)))] .into_iter() - .chain(message_debug_printf_args) + .chain(message_debug_printf_args.iter().copied()) .chain(debug_printf_context_inputs.iter().copied()) .collect(); diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/debuginfo.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/debuginfo.rs index 8cee5e2bf5..6121d01122 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/debuginfo.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/debuginfo.rs @@ -25,6 +25,7 @@ pub fn convert_custom_debuginfo_to_spv(module: &mut Module) { seen_types: FxIndexSet::default(), seen_consts: FxIndexSet::default(), + seen_data_inst_forms: FxIndexSet::default(), seen_global_vars: FxIndexSet::default(), seen_funcs: FxIndexSet::default(), }; @@ -82,7 +83,7 @@ impl Transformer for CustomDebuginfoToSpv<'_> { if let DataInstKind::SpvExtInst { ext_set, inst: ext_inst, - } = data_inst_def.kind + } = self.cx[data_inst_def.form].kind { if ext_set == self.custom_ext_inst_set { let custom_op = CustomOp::decode(ext_inst); diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs index 6dc7bec717..b658c4b09f 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs @@ -11,17 +11,28 @@ use spirt::func_at::FuncAt; use spirt::visit::{InnerVisit, Visitor}; use spirt::{ spv, Attr, AttrSet, AttrSetDef, Const, ConstCtor, Context, ControlNode, ControlNodeKind, - DataInstDef, DataInstKind, Diag, DiagLevel, ExportKey, Exportee, Func, FuncDecl, GlobalVar, - InternedStr, Module, Type, Value, + DataInstDef, DataInstForm, DataInstKind, Diag, DiagLevel, ExportKey, Exportee, Func, FuncDecl, + GlobalVar, InternedStr, Module, Type, Value, }; use std::marker::PhantomData; use std::{mem, str}; +pub(crate) struct ReportedDiagnostics { + pub rustc_errors_guarantee: rustc_errors::ErrorGuaranteed, + pub any_errors_were_spirt_bugs: bool, +} + +impl From for rustc_errors::ErrorGuaranteed { + fn from(r: ReportedDiagnostics) -> Self { + r.rustc_errors_guarantee + } +} + pub(crate) fn report_diagnostics( sess: &Session, linker_options: &crate::linker::Options, module: &Module, -) -> crate::linker::Result<()> { +) -> Result<(), ReportedDiagnostics> { let cx = &module.cx(); let mut reporter = DiagnosticReporter { @@ -68,28 +79,12 @@ pub(crate) fn report_diagnostics( exportee.inner_visit_with(&mut reporter); } - if reporter.any_spirt_bugs { - let mut note = sess.struct_note_without_error("SPIR-T bugs were reported"); - match &linker_options.dump_spirt_passes { - Some(dump_dir) => { - note.help(format!( - "pretty-printed SPIR-T will be saved to `{}`, as `.spirt.html` files", - dump_dir.display() - )); - } - None => { - // FIXME(eddyb) maybe just always generate the files in a tmpdir? - note.help( - "re-run with `RUSTGPU_CODEGEN_ARGS=\"--dump-spirt-passes=$PWD\"` to \ - get pretty-printed SPIR-T (`.spirt.html`)", - ); - } - } - note.note("pretty-printed SPIR-T is preferred when reporting Rust-GPU issues") - .emit(); - } - - reporter.overall_result + reporter + .overall_result + .map_err(|rustc_errors_guarantee| ReportedDiagnostics { + rustc_errors_guarantee, + any_errors_were_spirt_bugs: reporter.any_spirt_bugs, + }) } // HACK(eddyb) version of `decorations::LazilyDecoded` that works for SPIR-T. @@ -256,7 +251,7 @@ impl UseOrigin<'_> { let wk = &super::SpvSpecWithExtras::get().well_known; // FIXME(eddyb) deduplicate with `spirt_passes::diagnostics`. - let custom_op = match debug_inst_def.kind { + let custom_op = match cx[debug_inst_def.form].kind { DataInstKind::SpvExtInst { ext_set, inst: ext_inst, @@ -523,6 +518,11 @@ impl<'a> Visitor<'a> for DiagnosticReporter<'a> { } } } + fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) { + // NOTE(eddyb) this contains no deduplication because each `DataInstDef` + // will have any diagnostics reported separately. + self.visit_data_inst_form_def(&self.cx[data_inst_form]); + } fn visit_global_var_use(&mut self, gv: GlobalVar) { if self.seen_global_vars.insert(gv) { @@ -622,7 +622,7 @@ impl<'a> Visitor<'a> for DiagnosticReporter<'a> { if let DataInstKind::SpvExtInst { ext_set, inst: ext_inst, - } = data_inst_def.kind + } = self.cx[data_inst_def.form].kind { if ext_set == self.custom_ext_inst_set { match CustomOp::decode(ext_inst) { @@ -692,7 +692,7 @@ impl<'a> Visitor<'a> for DiagnosticReporter<'a> { _ => unreachable!(), } - if let DataInstKind::FuncCall(func) = data_inst_def.kind { + if let DataInstKind::FuncCall(func) = self.cx[data_inst_def.form].kind { // HACK(eddyb) visit `func` early, to control its `use_stack`, with // the later visit from `inner_visit_with` ignored as a duplicate. let old_origin = replace_origin(self, IntraFuncUseOrigin::CallCallee); diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs index 4b0e159c31..78ef09725a 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs @@ -13,8 +13,8 @@ use spirt::transform::InnerInPlaceTransform; use spirt::visit::{InnerVisit, Visitor}; use spirt::{ spv, AttrSet, Const, Context, ControlNode, ControlNodeKind, ControlRegion, DataInstDef, - DataInstKind, DeclDef, EntityOrientedDenseMap, Func, FuncDefBody, GlobalVar, Module, Type, - Value, + DataInstForm, DataInstFormDef, DataInstKind, DeclDef, EntityOrientedDenseMap, Func, + FuncDefBody, GlobalVar, Module, Type, Value, }; use std::collections::VecDeque; use std::hash::Hash; @@ -130,6 +130,7 @@ pub(super) fn run_func_passes

( seen_types: FxIndexSet::default(), seen_consts: FxIndexSet::default(), + seen_data_inst_forms: FxIndexSet::default(), seen_global_vars: FxIndexSet::default(), seen_funcs: FxIndexSet::default(), }; @@ -181,7 +182,7 @@ pub(super) fn run_func_passes

( pass_fn(cx, func_def_body); // FIXME(eddyb) avoid doing this except where changes occurred. - remove_unused_values_in_func(func_def_body); + remove_unused_values_in_func(cx, func_def_body); } } after_pass(full_name, module, profiler); @@ -196,6 +197,7 @@ struct ReachableUseCollector<'a> { // FIXME(eddyb) build some automation to avoid ever repeating these. seen_types: FxIndexSet, seen_consts: FxIndexSet, + seen_data_inst_forms: FxIndexSet, seen_global_vars: FxIndexSet, seen_funcs: FxIndexSet, } @@ -213,6 +215,11 @@ impl Visitor<'_> for ReachableUseCollector<'_> { self.visit_const_def(&self.cx[ct]); } } + fn visit_data_inst_form_use(&mut self, data_inst_form: DataInstForm) { + if self.seen_data_inst_forms.insert(data_inst_form) { + self.visit_data_inst_form_def(&self.cx[data_inst_form]); + } + } fn visit_global_var_use(&mut self, gv: GlobalVar) { if self.seen_global_vars.insert(gv) { @@ -247,6 +254,7 @@ const _: () = { fn visit_attr_set_use(&mut self, _: AttrSet) {} fn visit_type_use(&mut self, _: Type) {} fn visit_const_use(&mut self, _: Const) {} + fn visit_data_inst_form_use(&mut self, _: DataInstForm) {} fn visit_global_var_use(&mut self, _: GlobalVar) {} fn visit_func_use(&mut self, _: Func) {} @@ -354,7 +362,7 @@ const _: () = { /// a function body (both `DataInst`s and `ControlRegion` inputs/outputs). // // FIXME(eddyb) should this be a dedicated pass? -fn remove_unused_values_in_func(func_def_body: &mut FuncDefBody) { +fn remove_unused_values_in_func(cx: &Context, func_def_body: &mut FuncDefBody) { // Avoid having to support unstructured control-flow. if func_def_body.unstructured_cfg.is_some() { return; @@ -473,7 +481,9 @@ fn remove_unused_values_in_func(func_def_body: &mut FuncDefBody) { for func_at_inst in func_at_control_node.at(insts) { // Ignore pure instructions (i.e. they're only used // if their output value is used, from somewhere else). - if let DataInstKind::SpvInst(spv_inst) = &func_at_inst.def().kind { + if let DataInstKind::SpvInst(spv_inst) = + &cx[func_at_inst.def().form].kind + { // HACK(eddyb) small selection relevant for now, // but should be extended using e.g. a bitset. if [wk.OpNop, wk.OpCompositeInsert].contains(&spv_inst.opcode) { @@ -518,7 +528,9 @@ fn remove_unused_values_in_func(func_def_body: &mut FuncDefBody) { let mut all_nops = true; let mut func_at_inst_iter = func_def_body.at_mut(insts).into_iter(); while let Some(mut func_at_inst) = func_at_inst_iter.next() { - if let DataInstKind::SpvInst(spv_inst) = &func_at_inst.reborrow().def().kind { + if let DataInstKind::SpvInst(spv_inst) = + &cx[func_at_inst.reborrow().def().form].kind + { if spv_inst.opcode == wk.OpNop { continue; } @@ -528,10 +540,14 @@ fn remove_unused_values_in_func(func_def_body: &mut FuncDefBody) { { // Replace the removed `DataInstDef` itself with `OpNop`, // removing the ability to use its "name" as a value. + // + // FIXME(eddyb) cache the interned `OpNop`. *func_at_inst.def() = DataInstDef { attrs: Default::default(), - kind: DataInstKind::SpvInst(wk.OpNop.into()), - output_type: None, + form: cx.intern(DataInstFormDef { + kind: DataInstKind::SpvInst(wk.OpNop.into()), + output_type: None, + }), inputs: iter::empty().collect(), }; continue; diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/reduce.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/reduce.rs index 7b6760c095..cfe24dd31c 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/reduce.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/reduce.rs @@ -6,8 +6,8 @@ use spirt::visit::InnerVisit; use spirt::{ spv, Const, ConstCtor, ConstDef, Context, ControlNode, ControlNodeDef, ControlNodeKind, ControlNodeOutputDecl, ControlRegion, ControlRegionInputDecl, DataInst, DataInstDef, - DataInstKind, EntityOrientedDenseMap, FuncDefBody, SelectionKind, Type, TypeCtor, TypeDef, - Value, + DataInstFormDef, DataInstKind, EntityOrientedDenseMap, FuncDefBody, SelectionKind, Type, + TypeCtor, TypeDef, Value, }; use std::collections::hash_map::Entry; use std::convert::{TryFrom, TryInto}; @@ -65,7 +65,7 @@ pub(crate) fn reduce_in_func(cx: &Context, func_def_body: &mut FuncDefBody) { .. } => { for func_at_inst in func_at_control_node.at(insts) { - if let Ok(redu) = Reducible::try_from(func_at_inst.def()) { + if let Ok(redu) = Reducible::try_from((cx, func_at_inst.def())) { let redu_target = ReductionTarget::DataInst(func_at_inst.position); reduction_queue.push((redu_target, redu)); } @@ -202,10 +202,14 @@ pub(crate) fn reduce_in_func(cx: &Context, func_def_body: &mut FuncDefBody) { // Replace the reduced `DataInstDef` itself with `OpNop`, // removing the ability to use its "name" as a value. + // + // FIXME(eddyb) cache the interned `OpNop`. *func_def_body.at_mut(inst).def() = DataInstDef { attrs: Default::default(), - kind: DataInstKind::SpvInst(wk.OpNop.into()), - output_type: None, + form: cx.intern(DataInstFormDef { + kind: DataInstKind::SpvInst(wk.OpNop.into()), + output_type: None, + }), inputs: iter::empty().collect(), }; } @@ -235,15 +239,23 @@ pub(crate) fn reduce_in_func(cx: &Context, func_def_body: &mut FuncDefBody) { break; } - func_def_body.inner_in_place_transform_with(&mut ReplaceValueWith(|v| match v { - Value::Const(_) => None, - _ => value_replacements - .get(&HashableValue(v)) - .copied() - .map(|new| { - any_changes = true; - new - }), + func_def_body.inner_in_place_transform_with(&mut ReplaceValueWith(|mut v| { + let old = v; + loop { + match v { + Value::Const(_) => break, + _ => match value_replacements.get(&HashableValue(v)) { + Some(&new) => v = new, + None => break, + }, + } + } + if v != old { + any_changes = true; + Some(v) + } else { + None + } })); } } @@ -491,12 +503,14 @@ impl Reducible { } } -impl TryFrom<&DataInstDef> for Reducible { +// FIXME(eddyb) instead of taking a `&Context`, could `Reducible` hold a `DataInstForm`? +impl TryFrom<(&Context, &DataInstDef)> for Reducible { type Error = (); - fn try_from(inst_def: &DataInstDef) -> Result { - if let DataInstKind::SpvInst(spv_inst) = &inst_def.kind { + fn try_from((cx, inst_def): (&Context, &DataInstDef)) -> Result { + let inst_form_def = &cx[inst_def.form]; + if let DataInstKind::SpvInst(spv_inst) = &inst_form_def.kind { let op = PureOp::try_from(spv_inst)?; - let output_type = inst_def.output_type.unwrap(); + let output_type = inst_form_def.output_type.unwrap(); if let [input] = inst_def.inputs[..] { return Ok(Self { op, @@ -509,15 +523,21 @@ impl TryFrom<&DataInstDef> for Reducible { } } -// HACK(eddyb) `IntToBool` is the only reason this is `TryFrom` not `From`. -impl TryFrom for DataInstDef { - type Error = (); - fn try_from(redu: Reducible) -> Result { - Ok(Self { +impl Reducible { + // HACK(eddyb) `IntToBool` is the only reason this can return `None`. + fn try_into_inst(self, cx: &Context) -> Option { + let Self { + op, + output_type, + input, + } = self; + Some(DataInstDef { attrs: Default::default(), - kind: DataInstKind::SpvInst(redu.op.try_into()?), - output_type: Some(redu.output_type), - inputs: iter::once(redu.input).collect(), + form: cx.intern(DataInstFormDef { + kind: DataInstKind::SpvInst(op.try_into().ok()?), + output_type: Some(output_type), + }), + inputs: iter::once(input).collect(), }) } } @@ -616,11 +636,11 @@ enum ReductionStep { impl Reducible<&DataInstDef> { // FIXME(eddyb) force the input to actually be itself some kind of pure op. - fn try_reduce_output_of_data_inst(&self) -> Option { + fn try_reduce_output_of_data_inst(&self, cx: &Context) -> Option { let wk = &super::SpvSpecWithExtras::get().well_known; let input_inst_def = self.input; - if let DataInstKind::SpvInst(input_spv_inst) = &input_inst_def.kind { + if let DataInstKind::SpvInst(input_spv_inst) = &cx[input_inst_def.form].kind { // NOTE(eddyb) do not destroy information left in e.g. comments. #[allow(clippy::match_same_arms)] match self.op { @@ -729,8 +749,9 @@ impl Reducible { .try_reduce(cx, func.reborrow(), value_replacements, parent_map, cache)?; // HACK(eddyb) this is here because it can fail, see the comment // on `output_from_updated_state` for what's actually going on. - let output_from_updated_state_inst = - DataInstDef::try_from(self.with_input(input_from_updated_state)).ok()?; + let output_from_updated_state_inst = self + .with_input(input_from_updated_state) + .try_into_inst(cx)?; // Now that the reduction succeeded for the initial state, // we can proceed with augmenting the loop with the extra state. @@ -866,7 +887,10 @@ impl Reducible { } Value::DataInstOutput(inst) => { let inst_def = &*func.reborrow().at(inst).def(); - match self.with_input(inst_def).try_reduce_output_of_data_inst()? { + match self + .with_input(inst_def) + .try_reduce_output_of_data_inst(cx)? + { ReductionStep::Complete(v) => Some(v), // FIXME(eddyb) actually use a loop instead of recursing here. ReductionStep::Partial(redu) => { diff --git a/crates/rustc_codegen_spirv/src/linker/test.rs b/crates/rustc_codegen_spirv/src/linker/test.rs index 0dfbcade1c..849951db31 100644 --- a/crates/rustc_codegen_spirv/src/linker/test.rs +++ b/crates/rustc_codegen_spirv/src/linker/test.rs @@ -2,7 +2,8 @@ use super::{link, LinkResult}; use pipe::pipe; use rspirv::dr::{Loader, Module}; use rustc_errors::{registry::Registry, TerminalUrl}; -use rustc_session::{config::Input, CompilerIO}; +use rustc_session::config::{Input, OutputFilenames, OutputTypes}; +use rustc_session::CompilerIO; use rustc_span::FileName; use std::io::Read; @@ -148,7 +149,20 @@ fn link_with_linker_opts( ) }; - let res = link(&sess, modules, opts, Default::default()); + let res = link( + &sess, + modules, + opts, + &OutputFilenames::new( + "".into(), + "".into(), + None, + None, + "".into(), + OutputTypes::new(&[]), + ), + Default::default(), + ); assert_eq!(sess.has_errors(), res.as_ref().err().copied()); res.map(|res| match res { LinkResult::SingleModule(m) => *m, diff --git a/docs/src/codegen-args.md b/docs/src/codegen-args.md index 060bbbcb6e..2dda3864b9 100644 --- a/docs/src/codegen-args.md +++ b/docs/src/codegen-args.md @@ -192,11 +192,19 @@ _Note: passes that are not already enabled by default are considered experimenta Dump the `SPIR-🇹` module across passes (i.e. all of the versions before/after each pass), as a combined report, to a pair of files (`.spirt` and `.spirt.html`) in `DIR`. (the `.spirt.html` version of the report is the recommended form for viewing, as it uses tabling for versions, syntax-highlighting-like styling, and use->def linking) -### `--spirt-keep-custom-debuginfo-in-dumps` +### `--spirt-strip-custom-debuginfo-from-dumps` -When dumping (pretty-printed) `SPIR-🇹` (e.g. with `--dump-spirt-passes`), preserve -all the custom (Rust-GPU-specific) debuginfo instructions, instead of converting -them to the standard SPIR-V debuginfo (which `SPIR-🇹` pretty-prints specially). +When dumping (pretty-printed) `SPIR-🇹` (e.g. with `--dump-spirt-passes`), strip +all the custom (Rust-GPU-specific) debuginfo instructions, by converting them +to the standard SPIR-V debuginfo (which `SPIR-🇹` understands more directly). -The default (of performing that conversion) has prettier results, but is lossier +The default (keeping the custom instructions) is more verbose, but also lossless, if you want to see all instructions exactly as e.g. `--spirt-passes` see them. + +### `--spirt-keep-debug-sources-in-dumps` + +When dumping (pretty-printed) `SPIR-🇹` (e.g. with `--dump-spirt-passes`), preserve +all the "file contents debuginfo" (i.e. from SPIR-V `OpSource` instructions), +which will end up being included, in full, at the start of the dump. + +The default (of hiding the file contents) is less verbose, but (arguably) lossier.