diff --git a/CHANGELOG.md b/CHANGELOG.md index dc37d7c34e..bd43801491 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,6 +33,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - [PR#1031](https://github.com/EmbarkStudios/rust-gpu/pull/1031) added `Components` generic parameter to `Image` type, allowing images to return lower dimensional vectors and even scalars from the sampling API ### Changed 🛠 +- [PR#1040](https://github.com/EmbarkStudios/rust-gpu/pull/1040) refactored "zombie" (delayed error) reporting to use SPIR-V `OpSource`, be more helpful, and added `--no-early-report-zombies` to delay it even further + (see also [the `--no-early-report-zombies` codegen args docs](docs/src/codegen-args.md#--no-early-report-zombies)) - [PR#1035](https://github.com/EmbarkStudios/rust-gpu/pull/1035) reduced the number of CGUs ("codegen units") used by `spirv-builder` to just `1` - [PR#1011](https://github.com/EmbarkStudios/rust-gpu/pull/1011) made `NonWritable` all read-only storage buffers (i.e. those typed `&T`, where `T` doesn't have interior mutability) - [PR#1029](https://github.com/EmbarkStudios/rust-gpu/pull/1029) fixed SampledImage::sample() fns being unnecessarily marked as unsafe diff --git a/Cargo.lock b/Cargo.lock index 9460318f89..93538bd4d2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1973,6 +1973,7 @@ dependencies = [ "either", "hashbrown 0.11.2", "indexmap", + "itertools", "lazy_static", "libc", "num-traits", diff --git a/crates/rustc_codegen_spirv/Cargo.toml b/crates/rustc_codegen_spirv/Cargo.toml index 97f317bb2d..d24bd78143 100644 --- a/crates/rustc_codegen_spirv/Cargo.toml +++ b/crates/rustc_codegen_spirv/Cargo.toml @@ -60,6 +60,7 @@ spirv-tools = { version = "0.9", default-features = false } rustc_codegen_spirv-types.workspace = true spirt = "0.1.0" lazy_static = "1.4.0" +itertools = "0.10.5" [dev-dependencies] pipe = "0.4" diff --git a/crates/rustc_codegen_spirv/src/abi.rs b/crates/rustc_codegen_spirv/src/abi.rs index 91c28476dc..d4e2004b58 100644 --- a/crates/rustc_codegen_spirv/src/abi.rs +++ b/crates/rustc_codegen_spirv/src/abi.rs @@ -210,7 +210,7 @@ impl<'tcx> RecursivePointeeCache<'tcx> { cx.zombie_with_span( new_id, span, - "Cannot create self-referential types, even through pointers", + "cannot create self-referential types, even through pointers", ); Some(new_id) } diff --git a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs index 312b6f5366..f2cca482ef 100644 --- a/crates/rustc_codegen_spirv/src/builder/builder_methods.rs +++ b/crates/rustc_codegen_spirv/src/builder/builder_methods.rs @@ -171,7 +171,8 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { if invalid_seq_cst { self.zombie( semantics.def(self), - "Cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model. Check if AcquireRelease fits your needs.", + "cannot use AtomicOrdering=SequentiallyConsistent on Vulkan memory model \ + (check if AcquireRelease fits your needs)", ); } semantics @@ -352,11 +353,11 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { } fn zombie_convert_ptr_to_u(&self, def: Word) { - self.zombie(def, "Cannot convert pointers to integers"); + self.zombie(def, "cannot convert pointers to integers"); } fn zombie_convert_u_to_ptr(&self, def: Word) { - self.zombie(def, "Cannot convert integers to pointers"); + self.zombie(def, "cannot convert integers to pointers"); } fn zombie_ptr_equal(&self, def: Word, inst: &str) { @@ -656,12 +657,15 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { fn set_span(&mut self, span: Span) { self.current_span = Some(span); - let loc = self.cx.tcx.sess.source_map().lookup_char_pos(span.lo()); - let file = self - .builder - .def_string(format!("{}", loc.file.name.prefer_remapped())); - self.emit() - .line(file, loc.line as u32, loc.col_display as u32); + + // We may not always have valid spans. + // FIXME(eddyb) reduce the sources of this as much as possible. + if span.is_dummy() { + self.emit().no_line(); + } else { + let (file, line, col) = self.builder.file_line_col_for_op_line(span); + self.emit().line(file.file_name_op_string_id, line, col); + } } // FIXME(eddyb) change `Self::Function` to be more like a function index. @@ -1450,21 +1454,16 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { .with_type(dest_ty); if val_is_ptr || dest_is_ptr { - if self.is_system_crate(self.span()) { - self.zombie( - result.def(self), - &format!( - "Cannot cast between pointer and non-pointer types. From: {}. To: {}.", - self.debug_type(val.ty), - self.debug_type(dest_ty) - ), - ); - } else { - self.struct_err("Cannot cast between pointer and non-pointer types") - .note(&format!("from: {}", self.debug_type(val.ty))) - .note(&format!("to: {}", self.debug_type(dest_ty))) - .emit(); - } + self.zombie( + result.def(self), + &format!( + "cannot cast between pointer and non-pointer types\ + \nfrom `{}`\ + \n to `{}`", + self.debug_type(val.ty), + self.debug_type(dest_ty) + ), + ); } result @@ -1881,7 +1880,7 @@ impl<'a, 'tcx> BuilderMethods<'a, 'tcx> for Builder<'a, 'tcx> { empty(), ) .unwrap(); - self.zombie(dst.def(self), "Cannot memcpy dynamically sized data"); + self.zombie(dst.def(self), "cannot memcpy dynamically sized data"); } } diff --git a/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs b/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs index ceb38c43a3..6128752ff9 100644 --- a/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs +++ b/crates/rustc_codegen_spirv/src/builder/byte_addressable_buffer.rs @@ -11,7 +11,7 @@ use rustc_target::abi::{Align, Size}; impl<'a, 'tcx> Builder<'a, 'tcx> { fn load_err(&mut self, original_type: Word, invalid_type: Word) -> SpirvValue { let mut err = self.struct_err(&format!( - "Cannot load type {} in an untyped buffer load", + "cannot load type {} in an untyped buffer load", self.debug_type(original_type) )); if original_type != invalid_type { @@ -206,7 +206,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { fn store_err(&mut self, original_type: Word, value: SpirvValue) -> Result<(), ErrorGuaranteed> { let mut err = self.struct_err(&format!( - "Cannot store type {} in an untyped buffer store", + "cannot store type {} in an untyped buffer store", self.debug_type(original_type) )); if original_type != value.ty { diff --git a/crates/rustc_codegen_spirv/src/builder/mod.rs b/crates/rustc_codegen_spirv/src/builder/mod.rs index 16bc8ef489..4771ad2463 100644 --- a/crates/rustc_codegen_spirv/src/builder/mod.rs +++ b/crates/rustc_codegen_spirv/src/builder/mod.rs @@ -208,7 +208,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { .with_type(result_type); self.zombie( result.def(self), - "Cannot offset a pointer to an arbitrary element", + "cannot offset a pointer to an arbitrary element", ); result } @@ -219,7 +219,7 @@ impl<'a, 'tcx> Builder<'a, 'tcx> { let width = match self.lookup_type(shift.ty) { SpirvType::Integer(width, _) => width, other => self.fatal(&format!( - "Cannot rotate non-integer type: {}", + "cannot rotate non-integer type: {}", other.debug(shift.ty, self) )), }; diff --git a/crates/rustc_codegen_spirv/src/builder_spirv.rs b/crates/rustc_codegen_spirv/src/builder_spirv.rs index 0c5e660e38..3c003c6a34 100644 --- a/crates/rustc_codegen_spirv/src/builder_spirv.rs +++ b/crates/rustc_codegen_spirv/src/builder_spirv.rs @@ -5,14 +5,23 @@ use crate::symbols::Symbols; use crate::target::SpirvTarget; use crate::target_feature::TargetFeature; use rspirv::dr::{Block, Builder, Module, Operand}; -use rspirv::spirv::{AddressingModel, Capability, MemoryModel, Op, StorageClass, Word}; +use rspirv::spirv::{ + AddressingModel, Capability, MemoryModel, Op, SourceLanguage, StorageClass, Word, +}; use rspirv::{binary::Assemble, binary::Disassemble}; +use rustc_arena::DroplessArena; use rustc_data_structures::fx::{FxHashMap, FxHashSet}; +use rustc_data_structures::sync::Lrc; use rustc_middle::bug; +use rustc_middle::ty::TyCtxt; +use rustc_span::source_map::SourceMap; use rustc_span::symbol::Symbol; -use rustc_span::{Span, DUMMY_SP}; +use rustc_span::{FileName, FileNameDisplayPreference, SourceFile, Span, DUMMY_SP}; use std::assert_matches::assert_matches; use std::cell::{RefCell, RefMut}; +use std::hash::{Hash, Hasher}; +use std::iter; +use std::str; use std::{fs::File, io::Write, path::Path}; #[derive(Copy, Clone, Debug, Ord, PartialOrd, Eq, PartialEq, Hash)] @@ -131,9 +140,7 @@ impl SpirvValue { IllegalConst::Indirect(cause) => cause.message(), }; - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - cx.zombie_even_in_user_code(id, span, msg); + cx.zombie_with_span(id, span, msg); id } @@ -149,25 +156,15 @@ impl SpirvValue { } SpirvValueKind::FnAddr { .. } => { - if cx.is_system_crate(span) { - cx.builder - .const_to_id - .borrow() - .get(&WithType { - ty: self.ty, - val: SpirvConst::ZombieUndefForFnAddr, - }) - .expect("FnAddr didn't go through proper undef registration") - .val - } else { - cx.tcx.sess.span_err( - span, - "Cannot use this function pointer for anything other than calls", - ); - // Because we never get beyond compilation (into e.g. linking), - // emitting an invalid ID reference here is OK. - 0 - } + cx.builder + .const_to_id + .borrow() + .get(&WithType { + ty: self.ty, + val: SpirvConst::ZombieUndefForFnAddr, + }) + .expect("FnAddr didn't go through proper undef registration") + .val } SpirvValueKind::LogicalPtrCast { @@ -175,24 +172,17 @@ impl SpirvValue { original_pointee_ty, zombie_target_undef, } => { - if cx.is_system_crate(span) { - cx.zombie_with_span( - zombie_target_undef, - span, - &format!( - "Cannot cast between pointer types. From: {}. To: {}.", - cx.debug_type(original_pointee_ty), - cx.debug_type(self.ty) - ), - ); - } else { - cx.tcx - .sess - .struct_span_err(span, "Cannot cast between pointer types") - .note(&format!("from: *{}", cx.debug_type(original_pointee_ty))) - .note(&format!("to: {}", cx.debug_type(self.ty))) - .emit(); - } + cx.zombie_with_span( + zombie_target_undef, + span, + &format!( + "cannot cast between pointer types\ + \nfrom `*{}`\ + \n to `{}`", + cx.debug_type(original_pointee_ty), + cx.debug_type(self.ty) + ), + ); zombie_target_undef } @@ -321,6 +311,44 @@ struct WithConstLegality { legal: Result<(), IllegalConst>, } +/// `HashMap` key type (for `debug_file_cache` in `BuilderSpirv`), which is +/// equivalent to a whole `rustc` `SourceFile`, but has O(1) `Eq` and `Hash` +/// implementations (i.e. not involving the path or the contents of the file). +/// +/// This is possible because we can compare `Lrc`s by equality, as +/// `rustc`'s `SourceMap` already ensures that only one `SourceFile` will be +/// allocated for some given file. For hashing, we could hash the address, or +/// +struct DebugFileKey(Lrc); + +impl PartialEq for DebugFileKey { + fn eq(&self, other: &Self) -> bool { + let (Self(self_sf), Self(other_sf)) = (self, other); + Lrc::ptr_eq(self_sf, other_sf) + } +} +impl Eq for DebugFileKey {} + +impl Hash for DebugFileKey { + fn hash(&self, state: &mut H) { + let Self(sf) = self; + sf.name_hash.hash(state); + sf.src_hash.hash(state); + } +} + +#[derive(Copy, Clone)] +pub struct DebugFileSpirv<'tcx> { + pub file_name: &'tcx str, + + /// The SPIR-V ID for the result of the `OpString` instruction containing + /// `file_name` - this is what e.g. `OpLine` uses to identify the file. + /// + /// All other details about the file are also attached to this ID, using + /// other instructions that don't produce their own IDs (e.g. `OpSource`). + pub file_name_op_string_id: Word, +} + /// Cursor system: /// /// The LLVM module builder model (and therefore `codegen_ssa`) assumes that there is a central @@ -351,6 +379,9 @@ pub struct BuilderCursor { } pub struct BuilderSpirv<'tcx> { + source_map: &'tcx SourceMap, + dropless_arena: &'tcx DroplessArena, + builder: RefCell, // Bidirectional maps between `SpirvConst` and the ID of the defined global @@ -359,14 +390,20 @@ pub struct BuilderSpirv<'tcx> { // allows getting that legality information without additional lookups. const_to_id: RefCell>, WithConstLegality>>, id_to_const: RefCell>>>, - string_cache: RefCell>, + + debug_file_cache: RefCell>>, enabled_capabilities: FxHashSet, enabled_extensions: FxHashSet, } impl<'tcx> BuilderSpirv<'tcx> { - pub fn new(sym: &Symbols, target: &SpirvTarget, features: &[TargetFeature]) -> Self { + pub fn new( + tcx: TyCtxt<'tcx>, + sym: &Symbols, + target: &SpirvTarget, + features: &[TargetFeature], + ) -> Self { let version = target.spirv_version(); let memory_model = target.memory_model(); @@ -427,10 +464,12 @@ impl<'tcx> BuilderSpirv<'tcx> { builder.memory_model(AddressingModel::Logical, memory_model); Self { + source_map: tcx.sess.source_map(), + dropless_arena: &tcx.arena.dropless, builder: RefCell::new(builder), const_to_id: Default::default(), id_to_const: Default::default(), - string_cache: Default::default(), + debug_file_cache: Default::default(), enabled_capabilities, enabled_extensions, } @@ -637,15 +676,109 @@ impl<'tcx> BuilderSpirv<'tcx> { } } - pub fn def_string(&self, s: String) -> Word { - use std::collections::hash_map::Entry; - match self.string_cache.borrow_mut().entry(s) { - Entry::Occupied(entry) => *entry.get(), - Entry::Vacant(entry) => { - let key = entry.key().clone(); - *entry.insert(self.builder(Default::default()).string(key)) - } - } + pub fn file_line_col_for_op_line(&self, span: Span) -> (DebugFileSpirv<'tcx>, u32, u32) { + let loc = self.source_map.lookup_char_pos(span.lo()); + ( + self.def_debug_file(loc.file), + loc.line as u32, + loc.col_display as u32, + ) + } + + fn def_debug_file(&self, sf: Lrc) -> DebugFileSpirv<'tcx> { + *self + .debug_file_cache + .borrow_mut() + .entry(DebugFileKey(sf)) + .or_insert_with_key(|DebugFileKey(sf)| { + let mut builder = self.builder(Default::default()); + + // FIXME(eddyb) it would be nicer if we could just rely on + // `RealFileName::to_string_lossy` returning `Cow<'_, str>`, + // but sadly that `'_` is the lifetime of the temporary `Lrc`, + // not `'tcx`, so we have to arena-allocate to get `&'tcx str`. + let file_name = match &sf.name { + FileName::Real(name) => { + name.to_string_lossy(FileNameDisplayPreference::Remapped) + } + _ => sf.name.prefer_remapped().to_string().into(), + }; + let file_name = { + // FIXME(eddyb) it should be possible to arena-allocate a + // `&str` directly, but it would require upstream changes, + // and strings are handled by string interning in `rustc`. + fn arena_alloc_slice<'tcx, T: Copy>( + dropless_arena: &'tcx DroplessArena, + xs: &[T], + ) -> &'tcx [T] { + if xs.is_empty() { + &[] + } else { + dropless_arena.alloc_slice(xs) + } + } + str::from_utf8(arena_alloc_slice(self.dropless_arena, file_name.as_bytes())) + .unwrap() + }; + let file_name_op_string_id = builder.string(file_name.to_owned()); + + let file_contents = self + .source_map + .span_to_snippet(Span::with_root_ctxt(sf.start_pos, sf.end_pos)) + .ok(); + + // HACK(eddyb) this logic is duplicated from `spirt::spv::lift`. + let op_source_and_continued_chunks = file_contents.as_ref().map(|contents| { + // The maximum word count is `2**16 - 1`, the first word is + // taken up by the opcode & word count, and one extra byte is + // taken up by the nil byte at the end of the LiteralString. + const MAX_OP_SOURCE_CONT_CONTENTS_LEN: usize = (0xffff - 1) * 4 - 1; + + // `OpSource` has 3 more operands than `OpSourceContinued`, + // and each of them take up exactly one word. + const MAX_OP_SOURCE_CONTENTS_LEN: usize = + MAX_OP_SOURCE_CONT_CONTENTS_LEN - 3 * 4; + + let (op_source_str, mut all_op_source_continued_str) = + contents.split_at(contents.len().min(MAX_OP_SOURCE_CONTENTS_LEN)); + + // FIXME(eddyb) `spirt::spv::lift` should use this. + let all_op_source_continued_str_chunks = iter::from_fn(move || { + let contents_rest = &mut all_op_source_continued_str; + if contents_rest.is_empty() { + return None; + } + + // FIXME(eddyb) test with UTF-8! this `split_at` should + // actually take *less* than the full possible size, to + // avoid cutting a UTF-8 sequence. + let (cont_chunk, rest) = contents_rest + .split_at(contents_rest.len().min(MAX_OP_SOURCE_CONT_CONTENTS_LEN)); + *contents_rest = rest; + Some(cont_chunk) + }); + (op_source_str, all_op_source_continued_str_chunks) + }); + + if let Some((op_source_str, all_op_source_continued_str_chunks)) = + op_source_and_continued_chunks + { + builder.source( + SourceLanguage::Unknown, + 0, + Some(file_name_op_string_id), + Some(op_source_str), + ); + for cont_chunk in all_op_source_continued_str_chunks { + builder.source_continued(cont_chunk); + } + } + + DebugFileSpirv { + file_name, + file_name_op_string_id, + } + }) } pub fn set_global_initializer(&self, global: Word, initializer: Word) { diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs index 28b82a995d..dd75cc687c 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/constant.rs @@ -255,11 +255,8 @@ impl<'tcx> ConstMethods<'tcx> for CodegenCx<'tcx> { self.constant_null(ty) } else { let result = self.undef(ty); - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - self.zombie_even_in_user_code( + self.zombie_no_span( result.def_cx(self), - DUMMY_SP, "pointer has non-null integer address", ); result @@ -408,7 +405,7 @@ impl<'tcx> CodegenCx<'tcx> { SpirvType::Void => self .tcx .sess - .fatal("Cannot create const alloc of type void"), + .fatal("cannot create const alloc of type void"), SpirvType::Bool | SpirvType::Integer(..) | SpirvType::Float(_) @@ -552,37 +549,36 @@ impl<'tcx> CodegenCx<'tcx> { *data = *c + asdf->y[*c]; } */ - // HACK(eddyb) we don't know whether this constant originated - // in a system crate, so it's better to always zombie. - self.zombie_even_in_user_code( - result.def_cx(self), - DUMMY_SP, - "constant runtime array value", - ); + // NOTE(eddyb) the above description is a bit outdated, it's now + // clear `OpTypeRuntimeArray` does not belong in user code, and + // is only for dynamically-sized SSBOs and descriptor indexing, + // and a general solution looks similar to `union` handling, but + // for the length of a fixed-length array. + self.zombie_no_span(result.def_cx(self), "constant `OpTypeRuntimeArray` value"); result } SpirvType::Function { .. } => self .tcx .sess .fatal("TODO: SpirvType::Function not supported yet in create_const_alloc"), - SpirvType::Image { .. } => self.tcx.sess.fatal("Cannot create a constant image value"), + SpirvType::Image { .. } => self.tcx.sess.fatal("cannot create a constant image value"), SpirvType::Sampler => self .tcx .sess - .fatal("Cannot create a constant sampler value"), + .fatal("cannot create a constant sampler value"), SpirvType::SampledImage { .. } => self .tcx .sess - .fatal("Cannot create a constant sampled image value"), + .fatal("cannot create a constant sampled image value"), SpirvType::InterfaceBlock { .. } => self .tcx .sess - .fatal("Cannot create a constant interface block value"), + .fatal("cannot create a constant interface block value"), SpirvType::AccelerationStructureKhr => self .tcx .sess - .fatal("Cannot create a constant acceleration structure"), - SpirvType::RayQueryKhr => self.tcx.sess.fatal("Cannot create a constant ray query"), + .fatal("cannot create a constant acceleration structure"), + SpirvType::RayQueryKhr => self.tcx.sess.fatal("cannot create a constant ray query"), } } } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs index 7757eecace..b155d535bc 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/declare.rs @@ -2,6 +2,7 @@ use super::CodegenCx; use crate::abi::ConvSpirvType; use crate::attr::AggregatedSpirvAttributes; use crate::builder_spirv::{SpirvConst, SpirvValue, SpirvValueExt}; +use crate::decorations::{CustomDecoration, SrcLocDecoration}; use crate::spirv_type::SpirvType; use rspirv::spirv::{FunctionControl, LinkageType, StorageClass, Word}; use rustc_attr::InlineAttr; @@ -74,20 +75,36 @@ impl<'tcx> CodegenCx<'tcx> { self.zombie_with_span(result.def_cx(self), span, "called blocklisted fn"); return result; } - let mut emit = self.emit_global(); - let fn_id = emit - .begin_function(return_type, None, control, function_type) - .unwrap(); - if linkage != Some(LinkageType::Import) { - let parameter_values = argument_types - .iter() - .map(|&ty| emit.function_parameter(ty).unwrap().with_type(ty)) - .collect::>(); - self.function_parameter_values - .borrow_mut() - .insert(fn_id, parameter_values); - } - emit.end_function().unwrap(); + let fn_id = { + let mut emit = self.emit_global(); + let fn_id = emit + .begin_function(return_type, None, control, function_type) + .unwrap(); + if linkage != Some(LinkageType::Import) { + let parameter_values = argument_types + .iter() + .map(|&ty| emit.function_parameter(ty).unwrap().with_type(ty)) + .collect::>(); + self.function_parameter_values + .borrow_mut() + .insert(fn_id, parameter_values); + } + emit.end_function().unwrap(); + fn_id + }; + + // HACK(eddyb) this is a temporary workaround due to our use of `rspirv`, + // which prevents us from attaching `OpLine`s to `OpFunction` definitions, + // but we can use our custom `SrcLocDecoration` instead. + let src_loc_inst = SrcLocDecoration::from_rustc_span( + self.tcx.def_ident_span(instance.def_id()).unwrap_or(span), + &self.builder, + ) + .map(|src_loc| src_loc.encode_to_inst(fn_id)); + self.emit_global() + .module_mut() + .annotations + .extend(src_loc_inst); // HACK(eddyb) this is a bit roundabout, but the easiest way to get a // fully absolute path that contains at least as much information as @@ -99,9 +116,8 @@ impl<'tcx> CodegenCx<'tcx> { // (as some sort of opt-in, or toggled based on the platform, etc.). let symbol_name = self.tcx.symbol_name(instance).name; let demangled_symbol_name = format!("{:#}", rustc_demangle::demangle(symbol_name)); - emit.name(fn_id, &demangled_symbol_name); + self.emit_global().name(fn_id, &demangled_symbol_name); - drop(emit); // set_linkage uses emit if let Some(linkage) = linkage { self.set_linkage(fn_id, symbol_name.to_owned(), linkage); } @@ -201,7 +217,8 @@ impl<'tcx> CodegenCx<'tcx> { .variable(ptr_ty, None, StorageClass::Private, None) .with_type(ptr_ty); // TODO: These should be StorageClass::Private, so just zombie for now. - self.zombie_with_span(result.def_cx(self), span, "Globals are not supported yet"); + // FIXME(eddyb) why zombie? this looks like it should just work nowadays. + self.zombie_with_span(result.def_cx(self), span, "globals are not supported yet"); result } } diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs index 461f35fac0..3d3a62f876 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/entry.rs @@ -70,7 +70,7 @@ impl<'tcx> CodegenCx<'tcx> { } else { self.tcx .sess - .span_err(span, format!("Cannot declare {name} as an entry point")); + .span_err(span, format!("cannot declare {name} as an entry point")); return; }; let body = self diff --git a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs index d1e7add8b4..e900107ed6 100644 --- a/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs +++ b/crates/rustc_codegen_spirv/src/codegen_cx/mod.rs @@ -5,7 +5,7 @@ mod type_; use crate::builder::{ExtInst, InstructionTable}; use crate::builder_spirv::{BuilderCursor, BuilderSpirv, SpirvConst, SpirvValue, SpirvValueKind}; -use crate::decorations::{CustomDecoration, SerializedSpan, ZombieDecoration}; +use crate::decorations::{CustomDecoration, SrcLocDecoration, ZombieDecoration}; use crate::spirv_type::{SpirvType, SpirvTypePrinter, TypeCache}; use crate::symbols::Symbols; use crate::target::SpirvTarget; @@ -25,7 +25,7 @@ use rustc_middle::ty::layout::{HasParamEnv, HasTyCtxt}; use rustc_middle::ty::{Instance, ParamEnv, PolyExistentialTraitRef, Ty, TyCtxt}; use rustc_session::Session; use rustc_span::def_id::DefId; -use rustc_span::symbol::{sym, Symbol}; +use rustc_span::symbol::Symbol; use rustc_span::{SourceFile, Span, DUMMY_SP}; use rustc_target::abi::call::{FnAbi, PassMode}; use rustc_target::abi::{HasDataLayout, TargetDataLayout}; @@ -50,10 +50,11 @@ pub struct CodegenCx<'tcx> { /// Cache generated vtables pub vtables: RefCell, Option>), SpirvValue>>, pub ext_inst: RefCell, - /// Invalid spir-v IDs that should be stripped from the final binary, + /// Invalid SPIR-V IDs that should be stripped from the final binary, /// each with its own reason and span that should be used for reporting /// (in the event that the value is actually needed) - zombie_decorations: RefCell>, + zombie_decorations: + RefCell, Option>)>>, /// Cache of all the builtin symbols we need pub sym: Rc, pub instruction_table: InstructionTable, @@ -117,7 +118,7 @@ impl<'tcx> CodegenCx<'tcx> { Self { tcx, codegen_unit, - builder: BuilderSpirv::new(&sym, &target, &features), + builder: BuilderSpirv::new(tcx, &sym, &target, &features), instances: Default::default(), function_parameter_values: Default::default(), type_cache: Default::default(), @@ -163,71 +164,43 @@ impl<'tcx> CodegenCx<'tcx> { } /// Zombie system: - /// When compiling libcore and other system libraries, if something unrepresentable is - /// encountered, we don't want to fail the compilation. Instead, we emit something bogus - /// (usually it's fairly faithful, though, e.g. u128 emits `OpTypeInt 128`), and then mark the - /// resulting ID as a "zombie". We continue compiling the rest of the crate, then, at the very - /// end, anything that transtively references a zombie value is stripped from the binary. /// - /// If an exported function is stripped, then we emit a special "zombie export" item, which is - /// consumed by the linker, which continues to infect other values that reference it. + /// If something unrepresentable is encountered, we don't want to fail + /// the compilation. Instead, we emit something bogus (usually it's fairly + /// faithful, though, e.g. `u128` emits `OpTypeInt 128 0`), and then mark the + /// resulting ID as a "zombie". We continue compiling the rest of the crate, + /// then, at the very end, anything that transitively references a zombie value + /// is stripped from the binary. /// - /// Finally, if *user* code is marked as zombie, then this means that the user tried to do - /// something that isn't supported, and should be an error. + /// Errors will only be emitted (by `linker::zombies`) for reachable zombies. pub fn zombie_with_span(&self, word: Word, span: Span, reason: &str) { - if self.is_system_crate(span) { - self.zombie_even_in_user_code(word, span, reason); - } else { - self.tcx.sess.span_err(span, reason); - } - } - pub fn zombie_no_span(&self, word: Word, reason: &str) { - self.zombie_with_span(word, DUMMY_SP, reason); - } - pub fn zombie_even_in_user_code(&self, word: Word, span: Span, reason: &str) { self.zombie_decorations.borrow_mut().insert( word, - ZombieDecoration { - reason: reason.to_string(), - span: SerializedSpan::from_rustc(span, self.tcx.sess.source_map()), - }, + ( + ZombieDecoration { + // FIXME(eddyb) this could take advantage of `Cow` and use + // either `&'static str` or `String`, on a case-by-case basis. + reason: reason.to_string().into(), + }, + SrcLocDecoration::from_rustc_span(span, &self.builder), + ), ); } - - /// Returns `true` if the originating crate of `span` (which could very well - /// be a different crate, e.g. a generic/`#[inline]` function, or a macro), - /// is a "system crate", and therefore allowed to have some errors deferred - /// as "zombies" (see `zombie_with_span`'s docs above for more details). - pub fn is_system_crate(&self, span: Span) -> bool { - // HACK(eddyb) this ignores `.lo` vs `.hi` potentially resulting in - // different `SourceFile`s (which is likely a bug anyway). - let cnum = self - .tcx - .sess - .source_map() - .lookup_source_file(span.data().lo) - .cnum; - - self.tcx - .get_attr(cnum.as_def_id(), sym::compiler_builtins) - .is_some() - || [ - sym::core, - self.sym.spirv_std, - self.sym.libm, - self.sym.num_traits, - ] - .contains(&self.tcx.crate_name(cnum)) + pub fn zombie_no_span(&self, word: Word, reason: &str) { + self.zombie_with_span(word, DUMMY_SP, reason); } pub fn finalize_module(self) -> Module { let mut result = self.builder.finalize(); - result.annotations.extend( - self.zombie_decorations - .into_inner() - .into_iter() - .map(|(id, zombie)| zombie.encode(id)), - ); + result + .annotations + .extend(self.zombie_decorations.into_inner().into_iter().flat_map( + |(id, (zombie, src_loc))| { + [zombie.encode_to_inst(id)] + .into_iter() + .chain(src_loc.map(|src_loc| src_loc.encode_to_inst(id))) + }, + )); result } @@ -357,6 +330,11 @@ impl CodegenArgs { "no-compact-ids", "disables compaction of SPIR-V IDs at the end of linking", ); + opts.optflag( + "", + "no-early-report-zombies", + "delays reporting zombies (to allow more legalization)", + ); opts.optflag("", "no-structurize", "disables CFG structurization"); opts.optflag( @@ -536,6 +514,7 @@ impl CodegenArgs { // 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"), structurize: !matches.opt_present("no-structurize"), spirt: !matches.opt_present("no-spirt"), spirt_passes: matches @@ -788,11 +767,9 @@ impl<'tcx> MiscMethods<'tcx> for CodegenCx<'tcx> { } .def(span, self); - if self.is_system_crate(span) { - // Create these undefs up front instead of on demand in SpirvValue::def because - // SpirvValue::def can't use cx.emit() - self.def_constant(ty, SpirvConst::ZombieUndefForFnAddr); - } + // Create these `OpUndef`s up front, instead of on-demand in `SpirvValue::def`, + // because `SpirvValue::def` can't use `cx.emit()`. + self.def_constant(ty, SpirvConst::ZombieUndefForFnAddr); SpirvValue { kind: SpirvValueKind::FnAddr { diff --git a/crates/rustc_codegen_spirv/src/decorations.rs b/crates/rustc_codegen_spirv/src/decorations.rs index b4a937bab7..c3cbb843a0 100644 --- a/crates/rustc_codegen_spirv/src/decorations.rs +++ b/crates/rustc_codegen_spirv/src/decorations.rs @@ -1,17 +1,25 @@ //! SPIR-V decorations specific to `rustc_codegen_spirv`, produced during //! the original codegen of a crate, and consumed by the `linker`. +use crate::builder_spirv::BuilderSpirv; +use either::Either; +use itertools::Itertools; use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Decoration, Op, Word}; -use rustc_span::{source_map::SourceMap, FileName, Pos, Span}; -use serde::{Deserialize, Serialize}; +use rustc_data_structures::fx::FxIndexMap; +use rustc_data_structures::sync::Lrc; +use rustc_span::{source_map::SourceMap, Span}; +use rustc_span::{FileName, SourceFile}; +use smallvec::SmallVec; +use std::borrow::Cow; use std::marker::PhantomData; +use std::ops::Range; use std::path::PathBuf; -use std::{iter, slice}; +use std::{fmt, iter, slice, str}; /// Decorations not native to SPIR-V require some form of encoding into existing /// SPIR-V constructs, for which we use `OpDecorateString` with decoration type -/// `UserTypeGOOGLE` and a JSON-encoded Rust value as the decoration string. +/// `UserTypeGOOGLE` and some encoded Rust value as the decoration string. /// /// Each decoration type has to implement this trait, and use a different /// `ENCODING_PREFIX` from any other decoration type, to disambiguate them. @@ -22,16 +30,15 @@ use std::{iter, slice}; /// /// TODO: uses `non_semantic` instead of piggybacking off of `UserTypeGOOGLE` /// -pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { +pub trait CustomDecoration<'a>: Sized { const ENCODING_PREFIX: &'static str; - fn encode(self, id: Word) -> Instruction { - // FIXME(eddyb) this allocates twice, because there is no functionality - // in `serde_json` for writing to something that impls `fmt::Write`, - // only for `io::Write`, which would require performing redundant UTF-8 - // (re)validation, or relying on `unsafe` code, to use with `String`. - let json = serde_json::to_string(&self).unwrap(); - let encoded = [Self::ENCODING_PREFIX, &json].concat(); + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result; + fn decode(s: &'a str) -> Self; + + fn encode_to_inst(self, id: Word) -> Instruction { + let mut encoded = Self::ENCODING_PREFIX.to_string(); + self.encode(&mut encoded).unwrap(); Instruction::new( Op::DecorateString, @@ -45,18 +52,18 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { ) } - fn try_decode(inst: &Instruction) -> Option<(Word, LazilyDeserialized<'_, Self>)> { + fn try_decode_from_inst(inst: &Instruction) -> Option<(Word, LazilyDecoded<'_, Self>)> { if inst.class.opcode == Op::DecorateString && inst.operands[1].unwrap_decoration() == Decoration::UserTypeGOOGLE { let id = inst.operands[0].unwrap_id_ref(); - let encoded = inst.operands[2].unwrap_literal_string(); - let json = encoded.strip_prefix(Self::ENCODING_PREFIX)?; + let prefixed_encoded = inst.operands[2].unwrap_literal_string(); + let encoded = prefixed_encoded.strip_prefix(Self::ENCODING_PREFIX)?; Some(( id, - LazilyDeserialized { - json, + LazilyDecoded { + encoded, _marker: PhantomData, }, )) @@ -69,150 +76,391 @@ pub trait CustomDecoration: for<'de> Deserialize<'de> + Serialize { module .annotations .iter() - .filter_map(Self::try_decode as fn(_) -> _) + .filter_map(Self::try_decode_from_inst as fn(_) -> _) } fn remove_all(module: &mut Module) { module .annotations - .retain(|inst| Self::try_decode(inst).is_none()); + .retain(|inst| Self::try_decode_from_inst(inst).is_none()); } } // HACK(eddyb) return type of `CustomDecoration::decode_all`, in lieu of -// `-> impl Iterator)` in the trait. +// `-> impl Iterator)` in the trait. type DecodeAllIter<'a, D> = iter::FilterMap< slice::Iter<'a, Instruction>, - fn(&'a Instruction) -> Option<(Word, LazilyDeserialized<'a, D>)>, + fn(&'a Instruction) -> Option<(Word, LazilyDecoded<'a, D>)>, >; -/// Helper allowing full deserialization to be avoided where possible. -#[derive(Copy, Clone)] -pub struct LazilyDeserialized<'a, D> { - json: &'a str, +/// Helper allowing full decoding to be avoided where possible. +// +// FIXME(eddyb) is this even needed? (decoding impls are now much cheaper) +pub struct LazilyDecoded<'a, D> { + encoded: &'a str, _marker: PhantomData, } -impl<'a, D: Deserialize<'a>> LazilyDeserialized<'a, D> { - pub fn deserialize(self) -> D { - serde_json::from_str(self.json).unwrap() +impl<'a, D: CustomDecoration<'a>> LazilyDecoded<'a, D> { + pub fn decode(&self) -> D { + D::decode(self.encoded) } } -#[derive(Deserialize, Serialize)] -pub struct ZombieDecoration { - pub reason: String, - - #[serde(flatten)] - pub span: Option, +pub struct ZombieDecoration<'a> { + pub reason: Cow<'a, str>, } -impl CustomDecoration for ZombieDecoration { +impl<'a> CustomDecoration<'a> for ZombieDecoration<'a> { const ENCODING_PREFIX: &'static str = "Z"; -} -/// Representation of a `rustc` `Span` that can be turned into a `Span` again -/// in another compilation, by reloading the file. However, note that this will -/// fail if the file changed since, which is detected using the serialized `hash`. -#[derive(Deserialize, Serialize)] -pub struct SerializedSpan { - file: PathBuf, - hash: serde_adapters::SourceFileHash, - lo: u32, - hi: u32, + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result { + let Self { reason } = self; + w.write_str(&reason) + } + fn decode(s: &'a str) -> Self { + Self { reason: s.into() } + } } -// HACK(eddyb) `rustc_span` types implement only `rustc_serialize` traits, but -// not `serde` traits, and the easiest workaround is to have our own types. -mod serde_adapters { - use serde::{Deserialize, Serialize}; +/// Equivalent of `OpLine`, for the places where `rspirv` currently doesn't let +/// us actually emit a real `OpLine`, the way generating SPIR-T directly might. +// +// NOTE(eddyb) by keeping `line`+`col`, we mimick `OpLine` limitations +// (which could be lifted in the future using custom SPIR-T debuginfo). +// NOTE(eddyb) `NonSemantic.Shader.DebugInfo`'s `DebugLine` has both start & end, +// might be good to invest in SPIR-T being able to use NonSemantic debuginfo. +#[derive(Copy, Clone)] +pub struct SrcLocDecoration<'a> { + pub file_name: &'a str, + pub line: u32, + pub col: u32, +} - #[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)] - pub enum SourceFileHashAlgorithm { - Md5, - Sha1, - Sha256, - } +impl<'a> CustomDecoration<'a> for SrcLocDecoration<'a> { + const ENCODING_PREFIX: &'static str = "L"; - impl From for SourceFileHashAlgorithm { - fn from(kind: rustc_span::SourceFileHashAlgorithm) -> Self { - match kind { - rustc_span::SourceFileHashAlgorithm::Md5 => Self::Md5, - rustc_span::SourceFileHashAlgorithm::Sha1 => Self::Sha1, - rustc_span::SourceFileHashAlgorithm::Sha256 => Self::Sha256, - } - } + fn encode(self, w: &mut impl fmt::Write) -> fmt::Result { + let Self { + file_name, + line, + col, + } = self; + write!(w, "{file_name}:{line}:{col}") } + fn decode(s: &'a str) -> Self { + #[derive(Copy, Clone, Debug)] + struct InvalidSrcLoc<'a>(&'a str); + let err = InvalidSrcLoc(s); - #[derive(Copy, Clone, PartialEq, Eq, Deserialize, Serialize)] - pub struct SourceFileHash { - kind: SourceFileHashAlgorithm, - value: [u8; 32], - } + let (s, col) = s.rsplit_once(':').ok_or(err).unwrap(); + let (s, line) = s.rsplit_once(':').ok_or(err).unwrap(); + let file_name = s; - impl From for SourceFileHash { - fn from(hash: rustc_span::SourceFileHash) -> Self { - let bytes = hash.hash_bytes(); - let mut hash = Self { - kind: hash.kind.into(), - value: Default::default(), - }; - hash.value[..bytes.len()].copy_from_slice(bytes); - hash + Self { + file_name, + line: line.parse().unwrap(), + col: col.parse().unwrap(), } } } -impl SerializedSpan { - pub fn from_rustc(span: Span, source_map: &SourceMap) -> Option { - // Decorations may not always have valid spans. +impl<'tcx> SrcLocDecoration<'tcx> { + pub fn from_rustc_span(span: Span, builder: &BuilderSpirv<'tcx>) -> Option { + // We may not always have valid spans. // FIXME(eddyb) reduce the sources of this as much as possible. if span.is_dummy() { return None; } - let (lo, hi) = (span.lo(), span.hi()); - if lo > hi { - // FIXME(eddyb) broken `Span` - potentially turn this into an assert? - return None; + let (file, line, col) = builder.file_line_col_for_op_line(span); + + Some(Self { + file_name: file.file_name, + line, + col, + }) + } +} + +/// Helper type to delay most of the work necessary to turn a `SrcLocDecoration` +/// back into an usable `Span`, until it's actually needed (i.e. for an error). +pub struct SpanRegenerator<'a> { + source_map: &'a SourceMap, + module: Either<&'a Module, &'a spirt::Module>, + + src_loc_decorations: Option>>>, + + // HACK(eddyb) this has no really good reason to belong here, but it's easier + // to handle it together with `SrcLocDecoration`, than separately. + zombie_decorations: Option>>>, + + // HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. + spv_debug_info: Option>, +} + +#[derive(Default)] +struct SpvDebugInfo<'a> { + id_to_op_string: FxIndexMap, + files: FxIndexMap<&'a str, SpvDebugFile<'a>>, +} + +impl<'a> SpvDebugInfo<'a> { + fn collect(module: Either<&'a Module, &'a spirt::Module>) -> Self { + let mut this = Self::default(); + + let module = match module { + Either::Left(module) => module, + + // HACK(eddyb) the SPIR-T codepath is simpler, and kind of silly, + // but we need the `SpvDebugFile`'s `regenerated_rustc_source_file` + // caching, so for now it reuses `SpvDebugInfo` overall. + Either::Right(module) => { + let cx = module.cx_ref(); + match &module.debug_info { + spirt::ModuleDebugInfo::Spv(debug_info) => { + for sources in debug_info.source_languages.values() { + for (&file_name, src) in &sources.file_contents { + // FIXME(eddyb) what if the file is already present, + // should it be considered ambiguous overall? + this.files + .entry(&cx[file_name]) + .or_default() + .op_source_parts = [&src[..]].into_iter().collect(); + } + } + } + } + return this; + } + }; + + let mut insts = module.debug_string_source.iter().peekable(); + while let Some(inst) = insts.next() { + match inst.class.opcode { + Op::String => { + this.id_to_op_string.insert( + inst.result_id.unwrap(), + inst.operands[0].unwrap_literal_string(), + ); + } + Op::Source if inst.operands.len() == 4 => { + let file_name_id = inst.operands[2].unwrap_id_ref(); + if let Some(&file_name) = this.id_to_op_string.get(&file_name_id) { + let mut file = SpvDebugFile::default(); + file.op_source_parts + .push(inst.operands[3].unwrap_literal_string()); + while let Some(&next_inst) = insts.peek() { + if next_inst.class.opcode != Op::SourceContinued { + break; + } + insts.next(); + + file.op_source_parts + .push(next_inst.operands[0].unwrap_literal_string()); + } + + // FIXME(eddyb) what if the file is already present, + // should it be considered ambiguous overall? + this.files.insert(file_name, file); + } + } + _ => {} + } } + this + } +} - let file = source_map.lookup_source_file(lo); - if !(file.start_pos <= lo && hi <= file.end_pos) { - // FIXME(eddyb) broken `Span` - potentially turn this into an assert? - return None; +// HACK(eddyb) this is mostly replicating SPIR-T's module-level debuginfo. +#[derive(Default)] +struct SpvDebugFile<'a> { + /// Source strings from one `OpSource`, and any number of `OpSourceContinued`. + op_source_parts: SmallVec<[&'a str; 1]>, + + regenerated_rustc_source_file: Option>, +} + +impl<'a> SpanRegenerator<'a> { + pub fn new(source_map: &'a SourceMap, module: &'a Module) -> Self { + Self { + source_map, + module: Either::Left(module), + + src_loc_decorations: None, + zombie_decorations: None, + + spv_debug_info: None, } + } - Some(Self { - file: match &file.name { - // We can only support real files, not "synthetic" ones (which - // are almost never exposed to the compiler backend anyway). - FileName::Real(real_name) => real_name.local_path()?.to_path_buf(), - _ => return None, - }, - hash: file.src_hash.into(), - lo: (lo - file.start_pos).to_u32(), - hi: (hi - file.start_pos).to_u32(), - }) + pub fn new_spirt(source_map: &'a SourceMap, module: &'a spirt::Module) -> Self { + Self { + source_map, + module: Either::Right(module), + + src_loc_decorations: None, + zombie_decorations: None, + + spv_debug_info: None, + } } - pub fn to_rustc(&self, source_map: &SourceMap) -> Option { - let file = source_map.load_file(&self.file).ok()?; + pub fn src_loc_for_id(&mut self, id: Word) -> Option> { + self.src_loc_decorations + .get_or_insert_with(|| { + SrcLocDecoration::decode_all(self.module.left().unwrap()).collect() + }) + .get(&id) + .map(|src_loc| src_loc.decode()) + } - // If the file has changed since serializing, there's not much we can do, - // other than avoid creating invalid/confusing `Span`s. - // FIXME(eddyb) we could still indicate some of this to the user. - if self.hash != file.src_hash.into() { - return None; + // HACK(eddyb) this has no really good reason to belong here, but it's easier + // to handle it together with `SrcLocDecoration`, than separately. + pub(crate) fn zombie_for_id(&mut self, id: Word) -> Option> { + self.zombie_decorations + .get_or_insert_with(|| { + ZombieDecoration::decode_all(self.module.left().unwrap()).collect() + }) + .get(&id) + .map(|zombie| zombie.decode()) + } + + pub fn src_loc_from_op_line( + &mut self, + file_id: Word, + line: u32, + col: u32, + ) -> Option> { + self.spv_debug_info + .get_or_insert_with(|| SpvDebugInfo::collect(self.module)) + .id_to_op_string + .get(&file_id) + .map(|&file_name| SrcLocDecoration { + file_name, + line, + col, + }) + } + + fn regenerate_rustc_source_file(&mut self, file_name: &str) -> Option<&SourceFile> { + let spv_debug_file = self + .spv_debug_info + .get_or_insert_with(|| SpvDebugInfo::collect(self.module)) + .files + .get_mut(file_name)?; + + let file = &mut spv_debug_file.regenerated_rustc_source_file; + if file.is_none() { + // FIXME(eddyb) reduce allocations here by checking if the file is + // already loaded, and not allocating just to compare the source, + // but at least it's cheap when `OpSourceContinued` isn't used. + let src = match &spv_debug_file.op_source_parts[..] { + &[part] => Cow::Borrowed(part), + parts => parts.concat().into(), + }; + + // HACK(eddyb) in case the file has changed, and because `SourceMap` + // is strictly monotonic, we need to come up with some other name. + let mut sm_file_name_candidates = [PathBuf::from(file_name).into()] + .into_iter() + .chain((0..).map(|i| FileName::Custom(format!("outdated({i}) {file_name}")))); + + *file = sm_file_name_candidates.find_map(|sm_file_name_candidate| { + let sf = self + .source_map + .new_source_file(sm_file_name_candidate, src.clone().into_owned()); + + // Only use this `FileName` candidate if we either: + // 1. reused a `SourceFile` with the right `src`/`external_src` + // 2. allocated a new `SourceFile` with our choice of `src` + self.source_map + .ensure_source_file_source_present(sf.clone()); + let sf_src_matches = sf + .src + .as_ref() + .map(|sf_src| sf_src[..] == src[..]) + .or_else(|| { + sf.external_src + .borrow() + .get_source() + .map(|sf_src| sf_src[..] == src[..]) + }) + .unwrap_or(false); + + if sf_src_matches { + Some(sf) + } else { + None + } + }); } + file.as_deref() + } + + pub fn src_loc_to_rustc(&mut self, src_loc: SrcLocDecoration<'_>) -> Option { + let SrcLocDecoration { + file_name, + line, + col, + } = src_loc; + + let file = self.regenerate_rustc_source_file(file_name)?; - // Sanity check - assuming `SerializedSpan` isn't corrupted, this assert - // could only ever fail because of a hash collision. - assert!(self.lo <= self.hi && self.hi <= (file.end_pos.0 - file.start_pos.0)); + let line_bpos_range = file.line_bounds(line.checked_sub(1)? as usize); + + // Find the special cases (`MultiByteChar`s/`NonNarrowChar`s) in the line. + let multibyte_chars = { + let find = |bpos| { + file.multibyte_chars + .binary_search_by_key(&bpos, |mbc| mbc.pos) + .unwrap_or_else(|x| x) + }; + let Range { start, end } = line_bpos_range; + file.multibyte_chars[find(start)..find(end)].iter() + }; + let non_narrow_chars = { + let find = |bpos| { + file.non_narrow_chars + .binary_search_by_key(&bpos, |nnc| nnc.pos()) + .unwrap_or_else(|x| x) + }; + let Range { start, end } = line_bpos_range; + file.non_narrow_chars[find(start)..find(end)].iter() + }; + let mut special_chars = multibyte_chars + .merge_join_by(non_narrow_chars, |mbc, nnc| mbc.pos.cmp(&nnc.pos())) + .peekable(); + + // Increment the `BytePos` until we reach the right `col_display`, using + // `MultiByteChar`s/`NonNarrowChar`s to track non-trivial contributions + // (this may look inefficient, but lines tend to be short, and `rustc` + // itself is even worse than this, when it comes to `BytePos` lookups). + let (mut cur_bpos, mut cur_col_display) = (line_bpos_range.start, 0); + while cur_bpos < line_bpos_range.end && cur_col_display < col { + let next_special_bpos = special_chars.peek().map(|special| { + special + .as_ref() + .map_any(|mbc| mbc.pos, |nnc| nnc.pos()) + .reduce(|x, _| x) + }); + + // Batch trivial chars (i.e. chars 1:1 wrt `BytePos` vs `col_display`). + let following_trivial_chars = + next_special_bpos.unwrap_or(line_bpos_range.end).0 - cur_bpos.0; + if following_trivial_chars > 0 { + let wanted_trivial_chars = following_trivial_chars.min(col - cur_col_display); + cur_bpos.0 += wanted_trivial_chars; + cur_col_display += wanted_trivial_chars; + continue; + } + + // Add a special char's `BytePos` and `col_display` contributions. + let mbc_nnc = special_chars.next().unwrap(); + cur_bpos.0 += mbc_nnc.as_ref().left().map_or(1, |mbc| mbc.bytes as u32); + cur_col_display += mbc_nnc.as_ref().right().map_or(1, |nnc| nnc.width() as u32); + } - Some(Span::with_root_ctxt( - file.start_pos + Pos::from_u32(self.lo), - file.start_pos + Pos::from_u32(self.hi), - )) + Some(Span::with_root_ctxt(cur_bpos, cur_bpos)) } } diff --git a/crates/rustc_codegen_spirv/src/lib.rs b/crates/rustc_codegen_spirv/src/lib.rs index 071cfc8523..705a69386f 100644 --- a/crates/rustc_codegen_spirv/src/lib.rs +++ b/crates/rustc_codegen_spirv/src/lib.rs @@ -40,6 +40,7 @@ compile_error!( ); extern crate rustc_apfloat; +extern crate rustc_arena; extern crate rustc_ast; extern crate rustc_attr; extern crate rustc_codegen_ssa; diff --git a/crates/rustc_codegen_spirv/src/linker/duplicates.rs b/crates/rustc_codegen_spirv/src/linker/duplicates.rs index 7e9e0f7ff0..24ccfe8802 100644 --- a/crates/rustc_codegen_spirv/src/linker/duplicates.rs +++ b/crates/rustc_codegen_spirv/src/linker/duplicates.rs @@ -5,6 +5,9 @@ use rustc_data_structures::fx::{FxHashMap, FxHashSet}; use rustc_middle::bug; use std::collections::hash_map; +// FIXME(eddyb) consider deduplicating the `OpString` and `OpSource` created for +// file-level debuginfo (but using SPIR-T for linking might be better?). + pub fn remove_duplicate_extensions(module: &mut Module) { let mut set = FxHashSet::default(); diff --git a/crates/rustc_codegen_spirv/src/linker/mod.rs b/crates/rustc_codegen_spirv/src/linker/mod.rs index 1536c307b5..afe740920e 100644 --- a/crates/rustc_codegen_spirv/src/linker/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/mod.rs @@ -20,6 +20,7 @@ mod zombies; use std::borrow::Cow; use crate::codegen_cx::SpirvMetadata; +use crate::decorations::{CustomDecoration, SrcLocDecoration, ZombieDecoration}; use either::Either; use rspirv::binary::{Assemble, Consumer}; use rspirv::dr::{Block, Instruction, Loader, Module, ModuleHeader, Operand}; @@ -37,6 +38,7 @@ pub type Result = std::result::Result; pub struct Options { pub compact_ids: bool, pub dce: bool, + pub early_report_zombies: bool, pub structurize: bool, pub spirt: bool, pub spirt_passes: Vec, @@ -213,20 +215,27 @@ pub fn link( simple_passes::check_fragment_insts(sess, &output)?; } - // HACK(eddyb) this has to run before the `remove_zombies` pass, so that any - // zombies that are passed as call arguments, but eventually unused, won't - // be (incorrectly) considered used. + // 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, + // won't be (incorrectly) considered used. { let _timer = sess.timer("link_remove_unused_params"); output = param_weakening::remove_unused_params(output); } - { - let _timer = sess.timer("link_remove_zombies"); - zombies::remove_zombies(sess, opts, &mut output)?; + if opts.early_report_zombies { + let _timer = sess.timer("link_report_and_remove_zombies"); + zombies::report_and_remove_zombies(sess, opts, &mut output)?; } { + // 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 @@ -410,6 +419,11 @@ pub fn link( ); } + let report_diagnostics_result = { + let _timer = sess.timer("spirt_passes::diagnostics::report_diagnostics"); + spirt_passes::diagnostics::report_diagnostics(sess, opts, &module) + }; + // 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 { @@ -445,6 +459,11 @@ pub fn link( .unwrap(); } + // NOTE(eddyb) this is late so that `--dump-spirt-passes` is processed, + // even/especially when error were reported, but lifting to SPIR-V is + // skipped (since it could very well fail due to reported errors). + report_diagnostics_result?; + let spv_words = { let _timer = sess.timer("spirt::Module::lift_to_spv_module_emitter"); module.lift_to_spv_module_emitter().unwrap().words @@ -572,6 +591,14 @@ pub fn link( // compact the ids https://github.com/KhronosGroup/SPIRV-Tools/blob/e02f178a716b0c3c803ce31b9df4088596537872/source/opt/compact_ids_pass.cpp#L43 output.header.as_mut().unwrap().bound = simple_passes::compact_ids(output); }; + + // FIXME(eddyb) convert these into actual `OpLine`s with a SPIR-T pass, + // but that'd require keeping the modules in SPIR-T form (once lowered), + // and never loading them back into `rspirv` once lifted back to SPIR-V. + SrcLocDecoration::remove_all(output); + + // FIXME(eddyb) might make more sense to rewrite these away on SPIR-T. + ZombieDecoration::remove_all(output); } Ok(output) diff --git a/crates/rustc_codegen_spirv/src/linker/specializer.rs b/crates/rustc_codegen_spirv/src/linker/specializer.rs index 50e0dcb52b..582daf1da9 100644 --- a/crates/rustc_codegen_spirv/src/linker/specializer.rs +++ b/crates/rustc_codegen_spirv/src/linker/specializer.rs @@ -576,24 +576,26 @@ impl Specializer { let mut forward_declared_pointers = FxHashSet::default(); for inst in types_global_values_and_functions { - let result_id = inst.result_id.unwrap_or_else(|| { - unreachable!( - "Op{:?} is in `types_global_values` but not have a result ID", - inst.class.opcode - ); - }); - - if inst.class.opcode == Op::TypeForwardPointer { + let result_id = if inst.class.opcode == Op::TypeForwardPointer { forward_declared_pointers.insert(inst.operands[0].unwrap_id_ref()); - } - if forward_declared_pointers.remove(&result_id) { - // HACK(eddyb) this is a forward-declared pointer, pretend - // it's not "generic" at all to avoid breaking the rest of - // the logic - see module-level docs for how this should be - // handled in the future to support recursive data types. - assert_eq!(inst.class.opcode, Op::TypePointer); - continue; - } + inst.operands[0].unwrap_id_ref() + } else { + let result_id = inst.result_id.unwrap_or_else(|| { + unreachable!( + "Op{:?} is in `types_global_values` but not have a result ID", + inst.class.opcode + ); + }); + if forward_declared_pointers.remove(&result_id) { + // HACK(eddyb) this is a forward-declared pointer, pretend + // it's not "generic" at all to avoid breaking the rest of + // the logic - see module-level docs for how this should be + // handled in the future to support recursive data types. + assert_eq!(inst.class.opcode, Op::TypePointer); + continue; + } + result_id + }; // Record all integer `OpConstant`s (used for `IndexComposite`). if inst.class.opcode == Op::Constant { @@ -1326,7 +1328,13 @@ impl<'a, S: Specialization> InferCx<'a, S> { } } TyPat::SampledImage(pat) => simple(Op::TypeSampledImage, pat), - TyPat::Struct(fields_pat) => self.match_ty_list_pat(fields_pat, ty_operands), + TyPat::Struct(fields_pat) => { + if generic.def.class.opcode == Op::TypeStruct { + self.match_ty_list_pat(fields_pat, ty_operands) + } else { + Err(Unapplicable) + } + } TyPat::Function(ret_pat, params_pat) => { let (ret_ty, params_ty_list) = ty_operands.split_first(self).unwrap(); Ok(self diff --git a/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs new file mode 100644 index 0000000000..94e1370cb7 --- /dev/null +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/diagnostics.rs @@ -0,0 +1,383 @@ +use crate::decorations::{CustomDecoration, SpanRegenerator, SrcLocDecoration, ZombieDecoration}; +use rustc_data_structures::fx::FxIndexSet; +use rustc_errors::{DiagnosticBuilder, ErrorGuaranteed}; +use rustc_session::Session; +use rustc_span::{Span, DUMMY_SP}; +use smallvec::SmallVec; +use spirt::visit::{InnerVisit, Visitor}; +use spirt::{ + spv, Attr, AttrSet, AttrSetDef, Const, Context, DataInstDef, DataInstKind, ExportKey, Exportee, + Func, GlobalVar, Module, Type, +}; +use std::marker::PhantomData; +use std::{mem, str}; + +pub(crate) fn report_diagnostics( + sess: &Session, + linker_options: &crate::linker::Options, + module: &Module, +) -> crate::linker::Result<()> { + let cx = &module.cx(); + + let mut reporter = DiagnosticReporter { + sess, + linker_options, + + cx, + module, + + seen_attrs: FxIndexSet::default(), + seen_types: FxIndexSet::default(), + seen_consts: FxIndexSet::default(), + seen_global_vars: FxIndexSet::default(), + seen_funcs: FxIndexSet::default(), + + use_stack: SmallVec::new(), + span_regen: SpanRegenerator::new_spirt(sess.source_map(), module), + overall_result: Ok(()), + }; + for (export_key, &exportee) in &module.exports { + assert_eq!(reporter.use_stack.len(), 0); + + if let Exportee::Func(func) = exportee { + let func_decl = &module.funcs[func]; + reporter.use_stack.push(UseOrigin::IntraFunc { + func_attrs: func_decl.attrs, + func_export_key: Some(export_key), + inst_attrs: AttrSet::default(), + origin: IntraFuncUseOrigin::Other, + }); + export_key.inner_visit_with(&mut reporter); + if reporter.seen_funcs.insert(func) { + reporter.visit_func_decl(func_decl); + } + reporter.use_stack.pop(); + } + export_key.inner_visit_with(&mut reporter); + exportee.inner_visit_with(&mut reporter); + } + reporter.overall_result +} + +// HACK(eddyb) version of `decorations::LazilyDecoded` that works for SPIR-T. +struct LazilyDecoded { + encoded: String, + _marker: PhantomData, +} + +impl LazilyDecoded { + fn decode<'a>(&'a self) -> D + where + D: CustomDecoration<'a>, + { + D::decode(&self.encoded) + } +} + +fn decode_spv_lit_str_with(imms: &[spv::Imm], f: impl FnOnce(&str) -> R) -> R { + let wk = &super::SpvSpecWithExtras::get().well_known; + + // FIXME(eddyb) deduplicate with `spirt::spv::extract_literal_string`. + let words = imms.iter().enumerate().map(|(i, &imm)| match (i, imm) { + (0, spirt::spv::Imm::Short(k, w) | spirt::spv::Imm::LongStart(k, w)) + | (1.., spirt::spv::Imm::LongCont(k, w)) => { + // FIXME(eddyb) use `assert_eq!` after updating to latest SPIR-T. + assert!(k == wk.LiteralString); + w + } + _ => unreachable!(), + }); + let bytes: SmallVec<[u8; 64]> = words + .flat_map(u32::to_le_bytes) + .take_while(|&byte| byte != 0) + .collect(); + + f(str::from_utf8(&bytes).expect("invalid UTF-8 in string literal")) +} + +fn try_decode_custom_decoration<'a, D: CustomDecoration<'a>>( + attrs_def: &AttrSetDef, +) -> Option> { + let wk = &super::SpvSpecWithExtras::get().well_known; + + attrs_def.attrs.iter().find_map(|attr| { + let spv_inst = match attr { + Attr::SpvAnnotation(spv_inst) if spv_inst.opcode == wk.OpDecorateString => spv_inst, + _ => return None, + }; + let str_imms = spv_inst + .imms + .strip_prefix(&[spv::Imm::Short(wk.Decoration, wk.UserTypeGOOGLE)])?; + + decode_spv_lit_str_with(str_imms, |prefixed_encoded| { + let encoded = prefixed_encoded.strip_prefix(D::ENCODING_PREFIX)?; + + Some(LazilyDecoded { + encoded: encoded.to_string(), + _marker: PhantomData, + }) + }) + }) +} + +// FIXME(eddyb) this looks a lot like `ReachableUseCollector`, maybe some +// automation should be built around "deep visitors" in general? +struct DiagnosticReporter<'a> { + sess: &'a Session, + linker_options: &'a crate::linker::Options, + + cx: &'a Context, + module: &'a Module, + + seen_attrs: FxIndexSet, + seen_types: FxIndexSet, + seen_consts: FxIndexSet, + seen_global_vars: FxIndexSet, + seen_funcs: FxIndexSet, + + use_stack: SmallVec<[UseOrigin<'a>; 8]>, + span_regen: SpanRegenerator<'a>, + overall_result: crate::linker::Result<()>, +} + +enum UseOrigin<'a> { + Global { + kind: &'static &'static str, + attrs: AttrSet, + }, + IntraFunc { + func_attrs: AttrSet, + func_export_key: Option<&'a ExportKey>, + + inst_attrs: AttrSet, + origin: IntraFuncUseOrigin, + }, +} + +enum IntraFuncUseOrigin { + CallCallee, + Other, +} + +impl UseOrigin<'_> { + fn to_rustc_span(&self, cx: &Context, span_regen: &mut SpanRegenerator<'_>) -> Option { + let mut from_attrs = |attrs: AttrSet| { + let attrs_def = &cx[attrs]; + attrs_def + .attrs + .iter() + .find_map(|attr| match attr { + &Attr::SpvDebugLine { + file_path, + line, + col, + } => span_regen.src_loc_to_rustc(SrcLocDecoration { + file_name: &cx[file_path.0], + line, + col, + }), + _ => None, + }) + .or_else(|| { + span_regen.src_loc_to_rustc( + try_decode_custom_decoration::>(attrs_def)?.decode(), + ) + }) + }; + match *self { + Self::Global { attrs, .. } => from_attrs(attrs), + Self::IntraFunc { + func_attrs, + inst_attrs, + .. + } => from_attrs(inst_attrs).or_else(|| from_attrs(func_attrs)), + } + } + + fn note( + &self, + cx: &Context, + span_regen: &mut SpanRegenerator<'_>, + err: &mut DiagnosticBuilder<'_, ErrorGuaranteed>, + ) { + let wk = &super::SpvSpecWithExtras::get().well_known; + + let name_from_attrs = |attrs: AttrSet, kind| { + cx[attrs] + .attrs + .iter() + .find_map(|attr| match attr { + Attr::SpvAnnotation(spv_inst) if spv_inst.opcode == wk.OpName => { + Some(decode_spv_lit_str_with(&spv_inst.imms, |name| { + format!("`{name}`") + })) + } + _ => None, + }) + .unwrap_or_else(|| format!("unnamed {kind}")) + }; + let note = match self { + &Self::Global { kind, attrs } => { + format!("used by {}", name_from_attrs(attrs, *kind)) + } + Self::IntraFunc { + func_attrs, + func_export_key, + inst_attrs: _, + origin, + } => { + let func_desc = func_export_key + .map(|export_key| match export_key { + &ExportKey::LinkName(name) => format!("function export `{}`", &cx[name]), + ExportKey::SpvEntryPoint { imms, .. } => match imms[..] { + [em @ spv::Imm::Short(em_kind, _), ref name_imms @ ..] => { + // FIXME(eddyb) use `assert_eq!` after updating to latest SPIR-T. + assert!(em_kind == wk.ExecutionModel); + let em = spv::print::operand_from_imms([em]).concat_to_plain_text(); + decode_spv_lit_str_with(name_imms, |name| { + format!( + "{} entry-point `{name}`", + em.strip_prefix("ExecutionModel.").unwrap() + ) + }) + } + _ => unreachable!(), + }, + }) + .unwrap_or_else(|| name_from_attrs(*func_attrs, "function")); + match origin { + IntraFuncUseOrigin::CallCallee => format!("called by {func_desc}"), + IntraFuncUseOrigin::Other => format!("used from within {func_desc}"), + } + } + }; + + let span = self.to_rustc_span(cx, span_regen).unwrap_or(DUMMY_SP); + err.span_note(span, note); + } +} + +impl DiagnosticReporter<'_> { + fn report_from_attrs(&mut self, attrs: AttrSet) { + if attrs == AttrSet::default() { + return; + } + + // Split off the last entry in `self.use_stack` if it's for the definition + // that `attrs` come from - this should almost always be the case, except + // for instructions inside a function body, or visitor bugs. + let (current_def, use_stack_for_def) = self + .use_stack + .split_last() + .filter( + |( + &UseOrigin::Global { + attrs: use_attrs, .. + } + | &UseOrigin::IntraFunc { + func_attrs: use_attrs, + .. + }, + _, + )| { use_attrs == attrs }, + ) + .map_or((None, &self.use_stack[..]), |(current, stack)| { + (Some(current), stack) + }); + + let attrs_def = &self.cx[attrs]; + if !self.linker_options.early_report_zombies { + if let Some(zombie) = try_decode_custom_decoration::>(attrs_def) { + let ZombieDecoration { reason } = zombie.decode(); + let def_span = current_def + .and_then(|def| def.to_rustc_span(self.cx, &mut self.span_regen)) + .unwrap_or(DUMMY_SP); + let mut err = self.sess.struct_span_err(def_span, reason); + for use_origin in use_stack_for_def.iter().rev() { + use_origin.note(self.cx, &mut self.span_regen, &mut err); + } + self.overall_result = Err(err.emit()); + } + } + } +} + +impl Visitor<'_> for DiagnosticReporter<'_> { + fn visit_attr_set_use(&mut self, attrs: AttrSet) { + // HACK(eddyb) this avoids reporting the same diagnostics more than once. + if self.seen_attrs.insert(attrs) { + self.report_from_attrs(attrs); + } + } + fn visit_type_use(&mut self, ty: Type) { + if self.seen_types.insert(ty) { + let ty_def = &self.cx[ty]; + self.use_stack.push(UseOrigin::Global { + kind: &"type", + attrs: ty_def.attrs, + }); + self.visit_type_def(ty_def); + self.use_stack.pop(); + } + } + fn visit_const_use(&mut self, ct: Const) { + if self.seen_consts.insert(ct) { + let ct_def = &self.cx[ct]; + self.use_stack.push(UseOrigin::Global { + kind: &"constant", + attrs: ct_def.attrs, + }); + self.visit_const_def(ct_def); + self.use_stack.pop(); + } + } + + fn visit_global_var_use(&mut self, gv: GlobalVar) { + if self.seen_global_vars.insert(gv) { + let gv_decl = &self.module.global_vars[gv]; + self.use_stack.push(UseOrigin::Global { + // FIXME(eddyb) may be a `&CONST`, or an interface variable, + // not necessarily an user variable, so this could be confusing. + kind: &"global variable", + attrs: gv_decl.attrs, + }); + self.visit_global_var_decl(gv_decl); + self.use_stack.pop(); + } + } + fn visit_func_use(&mut self, func: Func) { + if self.seen_funcs.insert(func) { + let func_decl = &self.module.funcs[func]; + self.use_stack.push(UseOrigin::IntraFunc { + func_attrs: func_decl.attrs, + func_export_key: None, + inst_attrs: AttrSet::default(), + origin: IntraFuncUseOrigin::Other, + }); + self.visit_func_decl(func_decl); + self.use_stack.pop(); + } + } + + fn visit_data_inst_def(&mut self, data_inst_def: &DataInstDef) { + match self.use_stack.last_mut() { + Some(UseOrigin::IntraFunc { inst_attrs, .. }) => *inst_attrs = data_inst_def.attrs, + _ => unreachable!(), + } + + if let DataInstKind::FuncCall(func) = data_inst_def.kind { + let replace_origin = |this: &mut Self, new_origin| match this.use_stack.last_mut() { + Some(UseOrigin::IntraFunc { origin, .. }) => mem::replace(origin, new_origin), + _ => unreachable!(), + }; + + // 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); + self.visit_func_use(func); + replace_origin(self, old_origin); + } + + data_inst_def.inner_visit_with(self); + } +} 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 335202c137..1772234e3b 100644 --- a/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs +++ b/crates/rustc_codegen_spirv/src/linker/spirt_passes/mod.rs @@ -1,5 +1,6 @@ //! SPIR-T pass infrastructure and supporting utilities. +pub(crate) mod diagnostics; mod fuse_selects; mod reduce; @@ -59,8 +60,17 @@ macro_rules! def_spv_spec_with_extra_well_known { } let spv_spec = spv::spec::Spec::get(); + let wk = &spv_spec.well_known; + + let decorations = match &spv_spec.operand_kinds[wk.Decoration] { + spv::spec::OperandKindDef::ValueEnum { variants } => variants, + _ => unreachable!(), + }; + let lookup_fns = PerWellKnownGroup { opcode: |name| spv_spec.instructions.lookup(name).unwrap(), + operand_kind: |name| spv_spec.operand_kinds.lookup(name).unwrap(), + decoration: |name| decorations.lookup(name).unwrap().into(), }; SpvSpecWithExtras { @@ -87,6 +97,12 @@ def_spv_spec_with_extra_well_known! { OpCompositeInsert, OpCompositeExtract, ], + operand_kind: spv::spec::OperandKind = [ + ExecutionModel, + ], + decoration: u32 = [ + UserTypeGOOGLE, + ], } /// Run intra-function passes on all `Func` definitions in the `Module`. @@ -112,7 +128,8 @@ pub(super) fn run_func_passes

( seen_global_vars: FxIndexSet::default(), seen_funcs: FxIndexSet::default(), }; - for &exportee in module.exports.values() { + for (export_key, &exportee) in &module.exports { + export_key.inner_visit_with(&mut collector); exportee.inner_visit_with(&mut collector); } collector.seen_funcs diff --git a/crates/rustc_codegen_spirv/src/linker/zombies.rs b/crates/rustc_codegen_spirv/src/linker/zombies.rs index d2e4c14dad..1e54e0213e 100644 --- a/crates/rustc_codegen_spirv/src/linker/zombies.rs +++ b/crates/rustc_codegen_spirv/src/linker/zombies.rs @@ -1,236 +1,396 @@ //! See documentation on `CodegenCx::zombie` for a description of the zombie system. use super::{get_name, get_names}; -use crate::decorations::{CustomDecoration, ZombieDecoration}; -use rspirv::dr::{Instruction, Module}; +use crate::decorations::{CustomDecoration, SpanRegenerator, ZombieDecoration}; +use rspirv::dr::{Instruction, Module, Operand}; use rspirv::spirv::{Op, Word}; -use rustc_data_structures::fx::FxHashMap; +use rustc_data_structures::fx::{FxHashMap, FxIndexMap}; +use rustc_errors::{DiagnosticBuilder, ErrorGuaranteed}; use rustc_session::Session; use rustc_span::{Span, DUMMY_SP}; -use std::iter::once; +use smallvec::SmallVec; -#[derive(Clone)] -struct ZombieInfo<'a> { - reason: &'a str, - span: Span, - stack: Vec, +#[derive(Copy, Clone)] +struct Zombie<'a> { + id: Word, + kind: &'a ZombieKind, } -impl<'a> ZombieInfo<'a> { - fn new(reason: &'a str, span: Span) -> Self { - Self { - reason, - span, - stack: Vec::new(), - } - } - fn push_stack(&self, word: Word) -> Self { - Self { - reason: self.reason, - span: self.span, - stack: self.stack.iter().cloned().chain(once(word)).collect(), - } - } +enum ZombieKind { + /// Definition annotated with `ZombieDecoration`. + Leaf, + + /// Transitively zombie'd by using other zombies, from an instruction. + Uses(Vec), } -fn contains_zombie<'h, 'a>( - inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { - if let Some(result_type) = inst.result_type { - if let Some(reason) = zombie.get(&result_type) { - return Some(reason); - } - } - inst.operands - .iter() - .find_map(|op| op.id_ref_any().and_then(|w| zombie.get(&w))) +struct ZombieUse { + used_zombie_id: Word, + + /// Operands of the active `OpLine` at the time of the use, if any. + use_file_id_line_col: Option<(Word, u32, u32)>, + + origin: UseOrigin, } -fn is_zombie<'h, 'a>( - inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { - if let Some(result_id) = inst.result_id { - zombie.get(&result_id) - } else { - contains_zombie(inst, zombie) - } +enum UseOrigin { + GlobalOperandOrResultType, + IntraFuncOperandOrResultType { parent_func_id: Word }, + CallCalleeOperand { caller_func_id: Word }, } -fn is_or_contains_zombie<'h, 'a>( - inst: &Instruction, - zombie: &'h FxHashMap>, -) -> Option<&'h ZombieInfo<'a>> { - let result_zombie = inst.result_id.and_then(|result_id| zombie.get(&result_id)); - result_zombie.or_else(|| contains_zombie(inst, zombie)) +struct Zombies { + id_to_zombie_kind: FxIndexMap, } -fn spread_zombie(module: &mut Module, zombie: &mut FxHashMap>) -> bool { - let mut any = false; - // globals are easy - for inst in module.global_inst_iter() { - if let Some(result_id) = inst.result_id { - if let Some(reason) = contains_zombie(inst, zombie) { - if zombie.contains_key(&result_id) { - continue; +impl Zombies { + // FIXME(eddyb) rename all the other methods to say `_inst` explicitly. + fn get_zombie_by_id(&self, id: Word) -> Option> { + self.id_to_zombie_kind + .get(&id) + .map(|kind| Zombie { id, kind }) + } + + fn zombies_used_from_inst<'a>( + &'a self, + inst: &'a Instruction, + ) -> impl Iterator> + 'a { + inst.result_type + .into_iter() + .chain(inst.operands.iter().filter_map(|op| op.id_ref_any())) + .filter_map(move |id| self.get_zombie_by_id(id)) + } + + fn spread(&mut self, module: &Module) -> bool { + let mut any = false; + // globals are easy + { + let mut file_id_line_col = None; + for inst in module.global_inst_iter() { + match inst.class.opcode { + Op::Line => { + file_id_line_col = Some(( + inst.operands[0].unwrap_id_ref(), + inst.operands[1].unwrap_literal_int32(), + inst.operands[2].unwrap_literal_int32(), + )); + } + Op::NoLine => file_id_line_col = None, + _ => {} + } + + if let Some(result_id) = inst.result_id { + if self.id_to_zombie_kind.contains_key(&result_id) { + continue; + } + let zombie_uses: Vec<_> = self + .zombies_used_from_inst(inst) + .map(|zombie| ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin: UseOrigin::GlobalOperandOrResultType, + }) + .collect(); + if !zombie_uses.is_empty() { + self.id_to_zombie_kind + .insert(result_id, ZombieKind::Uses(zombie_uses)); + any = true; + } } - let reason = reason.clone(); - zombie.insert(result_id, reason); - any = true; } } - } - // No need to zombie defs within a function: If any def within a function is zombied, then the - // whole function is zombied. But, we don't have to mark the defs within a function as zombie, - // because the defs can't escape the function. - // HACK(eddyb) one exception to this is function-local variables, which may - // be unused and as such cannot be allowed to always zombie the function. - for func in &module.functions { - let func_id = func.def_id().unwrap(); - // Can't use zombie.entry() here, due to using the map in contains_zombie - if zombie.contains_key(&func_id) { - // Func is already zombie, no need to scan it again. - continue; - } - for inst in func.all_inst_iter() { - if inst.class.opcode == Op::Variable { - let result_id = inst.result_id.unwrap(); - if let Some(reason) = contains_zombie(inst, zombie) { - if zombie.contains_key(&result_id) { + // No need to zombie defs within a function: If any def within a function is zombied, then the + // whole function is zombied. But, we don't have to mark the defs within a function as zombie, + // because the defs can't escape the function. + // HACK(eddyb) one exception to this is function-local variables, which may + // be unused and as such cannot be allowed to always zombie the function. + for func in &module.functions { + let func_id = func.def_id().unwrap(); + if self.id_to_zombie_kind.contains_key(&func_id) { + // Func is already zombie, no need to scan it again. + continue; + } + + let mut all_zombie_uses_in_func = vec![]; + let mut file_id_line_col = None; + for inst in func.all_inst_iter() { + match inst.class.opcode { + Op::Line => { + file_id_line_col = Some(( + inst.operands[0].unwrap_id_ref(), + inst.operands[1].unwrap_literal_int32(), + inst.operands[2].unwrap_literal_int32(), + )); + } + Op::NoLine => file_id_line_col = None, + _ => {} + } + + if inst.class.opcode == Op::Variable { + let result_id = inst.result_id.unwrap(); + if self.id_to_zombie_kind.contains_key(&result_id) { continue; } - let reason = reason.clone(); - zombie.insert(result_id, reason); - any = true; + let zombie_uses: Vec<_> = self + .zombies_used_from_inst(inst) + .map(|zombie| ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin: UseOrigin::IntraFuncOperandOrResultType { + parent_func_id: func_id, + }, + }) + .collect(); + if !zombie_uses.is_empty() { + self.id_to_zombie_kind + .insert(result_id, ZombieKind::Uses(zombie_uses)); + any = true; + } + continue; } - } else if let Some(reason) = is_or_contains_zombie(inst, zombie) { - let pushed_reason = reason.push_stack(func_id); - zombie.insert(func_id, pushed_reason); + + all_zombie_uses_in_func.extend( + inst.result_id + .and_then(|result_id| self.get_zombie_by_id(result_id)) + .into_iter() + .chain(self.zombies_used_from_inst(inst)) + .map(|zombie| { + let origin = if inst.class.opcode == Op::FunctionCall + && inst.operands[0] == Operand::IdRef(zombie.id) + { + UseOrigin::CallCalleeOperand { + caller_func_id: func_id, + } + } else { + UseOrigin::IntraFuncOperandOrResultType { + parent_func_id: func_id, + } + }; + ZombieUse { + used_zombie_id: zombie.id, + use_file_id_line_col: file_id_line_col, + origin, + } + }), + ); + } + if !all_zombie_uses_in_func.is_empty() { + self.id_to_zombie_kind + .insert(func_id, ZombieKind::Uses(all_zombie_uses_in_func)); any = true; - break; } } + any } - any } -// If an entry point references a zombie'd value, then the entry point would normally get removed. -// That's an absolutely horrible experience to debug, though, so instead, create a nice error -// message containing the stack trace of how the entry point got to the zombie value. -fn report_error_zombies( - sess: &Session, - module: &Module, - zombie: &FxHashMap>, -) -> super::Result<()> { - let mut result = Ok(()); - let mut names = None; - for root in super::dce::collect_roots(module) { - if let Some(reason) = zombie.get(&root) { - let names = names.get_or_insert_with(|| get_names(module)); - let stack = reason - .stack - .iter() - .map(|&s| get_name(names, s).into_owned()); - let stack_note = once("Stack:".to_string()) - .chain(stack) - .collect::>() - .join("\n"); - result = Err(sess - .struct_span_err(reason.span, reason.reason) - .note(&stack_note) - .emit()); +struct ZombieReporter<'a> { + sess: &'a Session, + module: &'a Module, + zombies: &'a Zombies, + + id_to_name: Option>, + span_regen: SpanRegenerator<'a>, +} +impl<'a> ZombieReporter<'a> { + fn new(sess: &'a Session, module: &'a Module, zombies: &'a Zombies) -> Self { + Self { + sess, + module, + zombies, + + id_to_name: None, + span_regen: SpanRegenerator::new(sess.source_map(), module), } } - result + + // If an entry point references a zombie'd value, then the entry point would normally get removed. + // That's an absolutely horrible experience to debug, though, so instead, create a nice error + // message containing the stack trace of how the entry point got to the zombie value. + fn report_all(mut self) -> super::Result<()> { + let mut result = Ok(()); + // FIXME(eddyb) this loop means that every entry-point can potentially + // list out all the leaves, but that shouldn't be a huge issue. + for root_id in super::dce::collect_roots(self.module) { + if let Some(zombie) = self.zombies.get_zombie_by_id(root_id) { + for (_, mut err) in self.build_errors_keyed_by_leaf_id(zombie) { + result = Err(err.emit()); + } + } + } + result + } + + fn add_use_note_to_err( + &mut self, + err: &mut DiagnosticBuilder<'a, ErrorGuaranteed>, + span: Span, + zombie: Zombie<'_>, + zombie_use: &ZombieUse, + ) { + let mut id_to_name = |id, kind| { + self.id_to_name + .get_or_insert_with(|| get_names(self.module)) + .get(&id) + .map_or_else( + || format!("unnamed {kind} (%{id})"), + |&name| format!("`{name}`"), + ) + }; + let note = match zombie_use.origin { + UseOrigin::GlobalOperandOrResultType => { + format!("used by {}", id_to_name(zombie.id, "global")) + } + UseOrigin::IntraFuncOperandOrResultType { parent_func_id } => { + format!( + "used from within {}", + id_to_name(parent_func_id, "function") + ) + } + UseOrigin::CallCalleeOperand { caller_func_id } => { + format!("called by {}", id_to_name(caller_func_id, "function")) + } + }; + + let span = zombie_use + .use_file_id_line_col + .and_then(|(file_id, line, col)| { + self.span_regen.src_loc_from_op_line(file_id, line, col) + }) + .and_then(|src_loc| self.span_regen.src_loc_to_rustc(src_loc)) + .unwrap_or(span); + err.span_note(span, note); + } + + fn build_errors_keyed_by_leaf_id( + &mut self, + zombie: Zombie<'_>, + ) -> FxIndexMap> { + // FIXME(eddyb) this is a bit inefficient, compared to some kind of + // "small map", but this is the error path, and being correct is more + // important here - in particular, we don't want to ignore *any* leaves. + let mut errors_keyed_by_leaf_id = FxIndexMap::default(); + + let span = self + .span_regen + .src_loc_for_id(zombie.id) + .and_then(|src_loc| self.span_regen.src_loc_to_rustc(src_loc)) + .unwrap_or(DUMMY_SP); + match zombie.kind { + ZombieKind::Leaf => { + let reason = self.span_regen.zombie_for_id(zombie.id).unwrap().reason; + errors_keyed_by_leaf_id.insert(zombie.id, self.sess.struct_span_err(span, reason)); + } + ZombieKind::Uses(zombie_uses) => { + for zombie_use in zombie_uses { + let used_zombie = self + .zombies + .get_zombie_by_id(zombie_use.used_zombie_id) + .unwrap(); + for (leaf_id, err) in self.build_errors_keyed_by_leaf_id(used_zombie) { + use rustc_data_structures::fx::IndexEntry as Entry; + match errors_keyed_by_leaf_id.entry(leaf_id) { + Entry::Occupied(_) => err.cancel(), + Entry::Vacant(entry) => { + self.add_use_note_to_err( + entry.insert(err), + span, + zombie, + zombie_use, + ); + } + } + } + } + } + } + errors_keyed_by_leaf_id + } } -pub fn remove_zombies( +pub fn report_and_remove_zombies( sess: &Session, opts: &super::Options, module: &mut Module, ) -> super::Result<()> { - let zombies_owned = ZombieDecoration::decode_all(module) - .map(|(id, zombie)| { - let ZombieDecoration { reason, span } = zombie.deserialize(); - let span = span - .and_then(|span| span.to_rustc(sess.source_map())) - .unwrap_or(DUMMY_SP); - (id, (reason, span)) - }) - .collect::>(); - let mut zombies = zombies_owned - .iter() - .map(|(id, (reason, span))| (*id, ZombieInfo::new(reason, *span))) - .collect(); - ZombieDecoration::remove_all(module); + let mut zombies = Zombies { + id_to_zombie_kind: ZombieDecoration::decode_all(module) + .map(|(id, _)| (id, ZombieKind::Leaf)) + .collect(), + }; // Note: This is O(n^2). - while spread_zombie(module, &mut zombies) {} + while zombies.spread(module) {} - let result = report_error_zombies(sess, module, &zombies); + let result = ZombieReporter::new(sess, module, &zombies).report_all(); // FIXME(eddyb) use `log`/`tracing` instead. if opts.print_all_zombie { - for (&zomb, reason) in &zombies { - let orig = if zombies_owned.iter().any(|&(z, _)| z == zomb) { - "original" - } else { - "infected" - }; - println!("zombie'd {} because {} ({})", zomb, reason.reason, orig); + 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; + eprint!("zombie'd %{zombie_id} because {reason}"); + if !infection_chain.is_empty() { + eprint!(" (infected via {:?})", infection_chain); + } + eprintln!(); } } if opts.print_zombie { + let mut span_regen = SpanRegenerator::new(sess.source_map(), module); let names = get_names(module); for f in &module.functions { - if let Some(reason) = is_zombie(f.def.as_ref().unwrap(), &zombies) { + 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()); - println!("Function removed {:?} because {:?}", name, reason.reason); + let reason = span_regen.zombie_for_id(zombie_leaf_id).unwrap().reason; + eprintln!("function removed {name:?} because {reason:?}"); } } } - module - .capabilities - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .extensions - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .ext_inst_imports - .retain(|inst| is_zombie(inst, &zombies).is_none()); - if module - .memory_model - .as_ref() - .map_or(false, |inst| is_zombie(inst, &zombies).is_some()) + // FIXME(eddyb) this should be unnecessary, either something is unused, and + // it will get DCE'd *anyway*, or it caused an error. { - module.memory_model = None; + let keep = |inst: &Instruction| { + if let Some(result_id) = inst.result_id { + zombies.get_zombie_by_id(result_id).is_none() + } else { + zombies.zombies_used_from_inst(inst).next().is_none() + } + }; + 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())); } - module - .entry_points - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .execution_modes - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_string_source - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_names - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .debug_module_processed - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .annotations - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .types_global_values - .retain(|inst| is_zombie(inst, &zombies).is_none()); - module - .functions - .retain(|f| is_zombie(f.def.as_ref().unwrap(), &zombies).is_none()); result } diff --git a/crates/rustc_codegen_spirv/src/spirv_type.rs b/crates/rustc_codegen_spirv/src/spirv_type.rs index 5cce80991a..73cfdc662f 100644 --- a/crates/rustc_codegen_spirv/src/spirv_type.rs +++ b/crates/rustc_codegen_spirv/src/spirv_type.rs @@ -103,27 +103,28 @@ impl SpirvType<'_> { Self::Bool => cx.emit_global().type_bool_id(id), Self::Integer(width, signedness) => { let result = cx.emit_global().type_int_id(id, width, signedness as u32); + let u_or_i = if signedness { "i" } else { "u" }; match width { - 8 if !cx.builder.has_capability(Capability::Int8) => cx - .zombie_even_in_user_code(result, def_span, "u8 without OpCapability Int8"), - 16 if !cx.builder.has_capability(Capability::Int16) => cx - .zombie_even_in_user_code( - result, - def_span, - "u16 without OpCapability Int16", - ), - 64 if !cx.builder.has_capability(Capability::Int64) => cx - .zombie_even_in_user_code( - result, - def_span, - "u64 without OpCapability Int64", - ), - 8 | 16 | 32 | 64 => (), - 128 => cx.zombie_with_span(result, def_span, "u128"), - other => cx.zombie_with_span( + 8 if !cx.builder.has_capability(Capability::Int8) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}8` without `OpCapability Int8`"), + ), + 16 if !cx.builder.has_capability(Capability::Int16) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}16` without `OpCapability Int16`"), + ), + 64 if !cx.builder.has_capability(Capability::Int64) => cx.zombie_with_span( + result, + def_span, + &format!("`{u_or_i}64` without `OpCapability Int64`"), + ), + 8 | 16 | 32 | 64 => {} + w => cx.zombie_with_span( result, def_span, - &format!("Integer width {other} invalid for spir-v"), + &format!("`{u_or_i}{w}` unsupported in SPIR-V"), ), }; result @@ -131,17 +132,16 @@ impl SpirvType<'_> { Self::Float(width) => { let result = cx.emit_global().type_float_id(id, width); match width { - 64 if !cx.builder.has_capability(Capability::Float64) => cx - .zombie_even_in_user_code( - result, - def_span, - "f64 without OpCapability Float64", - ), + 64 if !cx.builder.has_capability(Capability::Float64) => cx.zombie_with_span( + result, + def_span, + "`f64` without `OpCapability Float64`", + ), 32 | 64 => (), other => cx.zombie_with_span( result, def_span, - &format!("Float width {other} invalid for spir-v"), + &format!("`f{other}` unsupported in SPIR-V"), ), }; result @@ -217,11 +217,8 @@ impl SpirvType<'_> { .type_pointer(id, StorageClass::Generic, pointee); // no pointers to functions if let SpirvType::Function { .. } = cx.lookup_type(pointee) { - cx.zombie_even_in_user_code( - result, - def_span, - "function pointer types are not allowed", - ); + // FIXME(eddyb) use the `SPV_INTEL_function_pointers` extension. + cx.zombie_with_span(result, def_span, "function pointer types are not allowed"); } result } @@ -293,11 +290,8 @@ impl SpirvType<'_> { .type_pointer(Some(id), StorageClass::Generic, pointee); // no pointers to functions if let SpirvType::Function { .. } = cx.lookup_type(pointee) { - cx.zombie_even_in_user_code( - result, - def_span, - "function pointer types are not allowed", - ); + // FIXME(eddyb) use the `SPV_INTEL_function_pointers` extension. + cx.zombie_with_span(result, def_span, "function pointer types are not allowed"); } result } diff --git a/docs/src/codegen-args.md b/docs/src/codegen-args.md index 704c733679..99c956e989 100644 --- a/docs/src/codegen-args.md +++ b/docs/src/codegen-args.md @@ -135,6 +135,12 @@ Disables compaction of SPIR-V IDs at the end of linking. Causes absolutely ginor emitted. Useful if you're println debugging IDs in the linker (although spirv-opt will compact them anyway, be careful). +### `--no-early-report-zombies` + +Delays reporting zombies (aka "deferred errors") even further, to allow more legalization. +Currently this also replaces the zombie reporting with a SPIR-T-based version +(which may become the default in the future). + ### `--no-structurize` Disables CFG structurization. Probably results in invalid modules. diff --git a/tests/ui/dis/asm_op_decorate.not_spirt.rs b/tests/ui/dis/asm_op_decorate.not_spirt.rs index 9b32e9871f..f8460b8f4d 100644 --- a/tests/ui/dis/asm_op_decorate.not_spirt.rs +++ b/tests/ui/dis/asm_op_decorate.not_spirt.rs @@ -6,6 +6,7 @@ // compile-flags: -C target-feature=+RuntimeDescriptorArray,+ext:SPV_EXT_descriptor_indexing // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/asm_op_decorate.spirt.rs b/tests/ui/dis/asm_op_decorate.spirt.rs index 973b80a609..be47ede266 100644 --- a/tests/ui/dis/asm_op_decorate.spirt.rs +++ b/tests/ui/dis/asm_op_decorate.spirt.rs @@ -6,6 +6,7 @@ // compile-flags: -C target-feature=+RuntimeDescriptorArray,+ext:SPV_EXT_descriptor_indexing // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/custom_entry_point.not_spirt.rs b/tests/ui/dis/custom_entry_point.not_spirt.rs index b2e8391214..8d1db083f4 100644 --- a/tests/ui/dis/custom_entry_point.not_spirt.rs +++ b/tests/ui/dis/custom_entry_point.not_spirt.rs @@ -5,6 +5,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/custom_entry_point.spirt.rs b/tests/ui/dis/custom_entry_point.spirt.rs index f5117322ca..6ef6a7dbe9 100644 --- a/tests/ui/dis/custom_entry_point.spirt.rs +++ b/tests/ui/dis/custom_entry_point.spirt.rs @@ -5,6 +5,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/generic-fn-op-name.not_spirt.rs b/tests/ui/dis/generic-fn-op-name.not_spirt.rs index 48ef785d67..a7629f5908 100644 --- a/tests/ui/dis/generic-fn-op-name.not_spirt.rs +++ b/tests/ui/dis/generic-fn-op-name.not_spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" #![feature(adt_const_params)] diff --git a/tests/ui/dis/generic-fn-op-name.spirt.rs b/tests/ui/dis/generic-fn-op-name.spirt.rs index ce29fb428b..4fbf96678e 100644 --- a/tests/ui/dis/generic-fn-op-name.spirt.rs +++ b/tests/ui/dis/generic-fn-op-name.spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" #![feature(adt_const_params)] diff --git a/tests/ui/dis/issue-723-output.not_spirt.rs b/tests/ui/dis/issue-723-output.not_spirt.rs index aab775a377..24dd50b82d 100644 --- a/tests/ui/dis/issue-723-output.not_spirt.rs +++ b/tests/ui/dis/issue-723-output.not_spirt.rs @@ -17,6 +17,7 @@ // build-pass // compile-flags: -C debuginfo=0 -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/issue-723-output.spirt.rs b/tests/ui/dis/issue-723-output.spirt.rs index 4a9a46b238..96e056b04b 100644 --- a/tests/ui/dis/issue-723-output.spirt.rs +++ b/tests/ui/dis/issue-723-output.spirt.rs @@ -17,6 +17,7 @@ // build-pass // compile-flags: -C debuginfo=0 -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs b/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs index ba25af924f..0b89f048dd 100644 --- a/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs +++ b/tests/ui/dis/non-writable-storage_buffer.not_spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr b/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr index d04f4bcd31..4cee425ab0 100644 --- a/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr +++ b/tests/ui/dis/non-writable-storage_buffer.not_spirt.stderr @@ -8,8 +8,8 @@ OpExtension "SPV_KHR_shader_clock" OpMemoryModel Logical Simple OpEntryPoint Fragment %1 "main" OpExecutionMode %1 OriginUpperLeft -%2 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.not_spirt.rs" -%3 = OpString "$OPSTRING_FILENAME/cell.rs" +%2 = OpString "$OPSTRING_FILENAME/cell.rs" +%3 = OpString "$OPSTRING_FILENAME/non-writable-storage_buffer.not_spirt.rs" OpName %4 "buf_imm" OpName %5 "buf_mut" OpName %6 "buf_interior_mut" diff --git a/tests/ui/dis/non-writable-storage_buffer.spirt.rs b/tests/ui/dis/non-writable-storage_buffer.spirt.rs index 5548788222..c9c0d083c0 100644 --- a/tests/ui/dis/non-writable-storage_buffer.spirt.rs +++ b/tests/ui/dis/non-writable-storage_buffer.spirt.rs @@ -7,6 +7,7 @@ // build-pass // compile-flags: -C llvm-args=--disassemble-globals // normalize-stderr-test "OpCapability VulkanMemoryModel\n" -> "" +// normalize-stderr-test "OpSource .*\n" -> "" // normalize-stderr-test "OpExtension .SPV_KHR_vulkan_memory_model.\n" -> "" // normalize-stderr-test "OpMemoryModel Logical Vulkan" -> "OpMemoryModel Logical Simple" diff --git a/tests/ui/dis/ptr_copy.normal.stderr b/tests/ui/dis/ptr_copy.normal.stderr index c9f66e2d67..c84157ee26 100644 --- a/tests/ui/dis/ptr_copy.normal.stderr +++ b/tests/ui/dis/ptr_copy.normal.stderr @@ -1,14 +1,29 @@ -error: Cannot memcpy dynamically sized data +error: cannot memcpy dynamically sized data --> $CORE_SRC/intrinsics.rs:2460:9 | 2460 | copy(src, dst, count) - | ^^^^^^^^^^^^^^^^^^^^^ + | ^ | - = note: Stack: - core::intrinsics::copy:: - ptr_copy::copy_via_raw_ptr - ptr_copy::main - main +note: used from within `core::intrinsics::copy::` + --> $CORE_SRC/intrinsics.rs:2447:21 + | +2447 | pub const unsafe fn copy(src: *const T, dst: *mut T, count: usize) { + | ^ +note: called by `ptr_copy::copy_via_raw_ptr` + --> $DIR/ptr_copy.rs:30:18 + | +30 | unsafe { core::ptr::copy(src, dst, 1) } + | ^ +note: called by `ptr_copy::main` + --> $DIR/ptr_copy.rs:35:5 + | +35 | copy_via_raw_ptr(&i, o); + | ^ +note: called by `main` + --> $DIR/ptr_copy.rs:33:1 + | +33 | #[spirv(fragment)] + | ^ error: aborting due to previous error diff --git a/tests/ui/lang/consts/nested-ref-in-composite.stderr b/tests/ui/lang/consts/nested-ref-in-composite.stderr index 0844de7a6b..d6205a866a 100644 --- a/tests/ui/lang/consts/nested-ref-in-composite.stderr +++ b/tests/ui/lang/consts/nested-ref-in-composite.stderr @@ -2,21 +2,35 @@ error: constant arrays/structs cannot contain pointers to other constants --> $DIR/nested-ref-in-composite.rs:20:17 | 20 | *pair_out = pair_deep_load(&(&123, &3.14)); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ | - = note: Stack: - nested_ref_in_composite::main_pair - main_pair +note: used from within `nested_ref_in_composite::main_pair` + --> $DIR/nested-ref-in-composite.rs:20:17 + | +20 | *pair_out = pair_deep_load(&(&123, &3.14)); + | ^ +note: called by `main_pair` + --> $DIR/nested-ref-in-composite.rs:18:1 + | +18 | #[spirv(fragment)] + | ^ error: constant arrays/structs cannot contain pointers to other constants --> $DIR/nested-ref-in-composite.rs:25:19 | 25 | *array3_out = array3_deep_load(&[&0, &1, &2]); - | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ + | ^ + | +note: used from within `nested_ref_in_composite::main_array3` + --> $DIR/nested-ref-in-composite.rs:25:19 + | +25 | *array3_out = array3_deep_load(&[&0, &1, &2]); + | ^ +note: called by `main_array3` + --> $DIR/nested-ref-in-composite.rs:23:1 | - = note: Stack: - nested_ref_in_composite::main_array3 - main_array3 +23 | #[spirv(fragment)] + | ^ error: aborting due to 2 previous errors diff --git a/tests/ui/lang/core/ptr/allocate_const_scalar.stderr b/tests/ui/lang/core/ptr/allocate_const_scalar.stderr index 05d7b4690e..c6ca98ddc3 100644 --- a/tests/ui/lang/core/ptr/allocate_const_scalar.stderr +++ b/tests/ui/lang/core/ptr/allocate_const_scalar.stderr @@ -1,8 +1,15 @@ error: pointer has non-null integer address - | - = note: Stack: - allocate_const_scalar::main - main + | +note: used from within `allocate_const_scalar::main` + --> $DIR/allocate_const_scalar.rs:15:20 + | +15 | let _pointer = POINTER; + | ^ +note: called by `main` + --> $DIR/allocate_const_scalar.rs:13:1 + | +13 | #[spirv(fragment)] + | ^ error: aborting due to previous error diff --git a/tests/ui/lang/core/ref/member_ref_arg-broken.rs b/tests/ui/lang/core/ref/member_ref_arg-broken.rs new file mode 100644 index 0000000000..d79e38e4b9 --- /dev/null +++ b/tests/ui/lang/core/ref/member_ref_arg-broken.rs @@ -0,0 +1,53 @@ +// FIXME(eddyb) this is like `member_ref_arg`, but testing the error messages +// in some broken cases - this test should eventually pass, but for now +// we care more that the error messages do not regress too much. + +// build-fail +// normalize-stderr-not_spirt "36\[%36\]" -> "$$ID[%$$ID]" +// normalize-stderr-spirt "38\[%38\]" -> "$$ID[%$$ID]" +// normalize-stderr-test "(note: module `.*)\.(not_spirt|spirt)`" -> "$1.{not_spirt,spirt}`" + +use spirv_std::spirv; + +struct S { + x: u32, + y: u32, +} + +// NOTE(eddyb) `#[inline(never)]` is for blocking inlining at the e.g. MIR level, +// whereas any Rust-GPU-specific legalization will intentially ignore it. + +#[inline(never)] +fn f(x: &u32) -> u32 { + *x +} + +#[inline(never)] +fn g(xy: (&u32, &u32)) -> (u32, u32) { + (*xy.0, *xy.1) +} + +#[inline(never)] +fn h(xyz: (&u32, &u32, &u32)) -> (u32, u32, u32) { + (*xyz.0, *xyz.1, *xyz.2) +} + +// FIXME(eddyb) the reason this doesn't work while the others do, is somewhat +// subtle - specifically, we currently have some passes (`destructure_composites` +// and `spirt_passes::reduce`) which can handle single-level `OpComposite*` +// instructions, but the extra nesting here stops them dead in their tracks +// (they're also not really the right solution for this problem, such composites +// should just never exist by-value, and `qptr` may very well get rid of them). +#[inline(never)] +fn h_newtyped(xyz: ((&u32, &u32, &u32),)) -> (u32, u32, u32) { + (*xyz.0 .0, *xyz.0 .1, *xyz.0 .2) +} + +#[spirv(fragment)] +pub fn main() { + let s = S { x: 2, y: 2 }; + f(&s.x); + g((&s.x, &s.y)); + h((&s.x, &s.y, &s.x)); + h_newtyped(((&s.x, &s.y, &s.x),)); +} diff --git a/tests/ui/lang/core/ref/member_ref_arg-broken.stderr b/tests/ui/lang/core/ref/member_ref_arg-broken.stderr new file mode 100644 index 0000000000..e2508b3e6d --- /dev/null +++ b/tests/ui/lang/core/ref/member_ref_arg-broken.stderr @@ -0,0 +1,11 @@ +warning: `#[inline(never)]` function `member_ref_arg_broken::h` needs to be inlined because it has illegal argument or return types + +warning: `#[inline(never)]` function `member_ref_arg_broken::h_newtyped` needs to be inlined because it has illegal argument or return types + +error: error:0:0 - OpLoad Pointer '$ID[%$ID]' is not a logical pointer. + | + = note: spirv-val failed + = note: module `$TEST_BUILD_DIR/lang/core/ref/member_ref_arg-broken.{not_spirt,spirt}` + +error: aborting due to previous error; 2 warnings emitted + diff --git a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs new file mode 100644 index 0000000000..cdb10e4fbd --- /dev/null +++ b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.rs @@ -0,0 +1,34 @@ +// FIXME(eddyb) this is like `zst_member_ref_arg`, but testing the error messages +// in some broken cases (see issue #1037) - this test should eventually pass, but +// for now we care more that the error messages do not regress too much. + +// build-fail + +use spirv_std::spirv; +struct A; +struct B; + +pub struct S { + x: A, + y: B, + + z: Z, + w: W, +} + +fn f(x: &B) {} + +#[spirv(fragment)] +pub fn main_scalar(#[spirv(push_constant)] s: &S) { + f(&s.y); +} + +#[spirv(fragment)] +pub fn main_scalar_pair(#[spirv(push_constant)] s: &S) { + f(&s.y); +} + +#[spirv(fragment)] +pub fn main_scalar_scalar_pair_nested(#[spirv(push_constant)] s: &S<(usize, usize)>) { + f(&s.y); +} diff --git a/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr new file mode 100644 index 0000000000..bd341e6aa3 --- /dev/null +++ b/tests/ui/lang/core/ref/zst_member_ref_arg-broken.stderr @@ -0,0 +1,59 @@ +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar_scalar_pair_nested` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^ +note: called by `main_scalar_scalar_pair_nested` + --> $DIR/zst_member_ref_arg-broken.rs:31:1 + | +31 | #[spirv(fragment)] + | ^ + +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar` + --> $DIR/zst_member_ref_arg-broken.rs:23:5 + | +23 | f(&s.y); + | ^ +note: called by `main_scalar` + --> $DIR/zst_member_ref_arg-broken.rs:21:1 + | +21 | #[spirv(fragment)] + | ^ + +error: cannot cast between pointer types + from `*struct (usize, usize) { u32, u32 }` + to `*struct B { }` + --> $DIR/zst_member_ref_arg-broken.rs:33:5 + | +33 | f(&s.y); + | ^ + | +note: used from within `zst_member_ref_arg_broken::main_scalar_pair` + --> $DIR/zst_member_ref_arg-broken.rs:28:5 + | +28 | f(&s.y); + | ^ +note: called by `main_scalar_pair` + --> $DIR/zst_member_ref_arg-broken.rs:26:1 + | +26 | #[spirv(fragment)] + | ^ + +error: aborting due to 3 previous errors +