From ea168baf569448b52fbe07fd145dd566de35a439 Mon Sep 17 00:00:00 2001 From: Jim Blandy Date: Thu, 19 Aug 2021 23:00:36 -0700 Subject: [PATCH] [spv-out] Clean up capability handling. Remove `forbidden_caps`. Accumulate capabilities actually used separately from the permitted capabilities, so that the latter can be retained across Writer resets, while the former is cleared between modules. --- src/back/spv/block.rs | 3 +- src/back/spv/mod.rs | 28 ++++++--- src/back/spv/recyclable.rs | 7 +++ src/back/spv/writer.rs | 124 ++++++++++++++++++++++++------------- 4 files changed, 112 insertions(+), 50 deletions(-) diff --git a/src/back/spv/block.rs b/src/back/spv/block.rs index 19450c02f7..f4923aa6f8 100644 --- a/src/back/spv/block.rs +++ b/src/back/spv/block.rs @@ -1111,7 +1111,8 @@ impl<'w> BlockContext<'w> { } }; - self.writer.check(&[spirv::Capability::ImageQuery])?; + self.writer + .require_any("image queries", &[spirv::Capability::ImageQuery])?; match query { Iq::Size { level } => { diff --git a/src/back/spv/mod.rs b/src/back/spv/mod.rs index 4c2c06e6da..bd52a03776 100644 --- a/src/back/spv/mod.rs +++ b/src/back/spv/mod.rs @@ -55,8 +55,8 @@ const BITS_PER_BYTE: crate::Bytes = 8; pub enum Error { #[error("target SPIRV-{0}.{1} is not supported")] UnsupportedVersion(u8, u8), - #[error("one of the required capabilities {0:?} is missing")] - MissingCapabilities(Vec), + #[error("using {0} requires at least one of the capabilities {1:?}, but none are available")] + MissingCapabilities(&'static str, Vec), #[error("unimplemented {0}")] FeatureNotImplemented(&'static str), #[error("module is not validated properly: {0}")] @@ -385,8 +385,18 @@ pub struct Writer { physical_layout: PhysicalLayout, logical_layout: LogicalLayout, id_gen: IdGenerator, - capabilities: crate::FastHashSet, - forbidden_caps: Option<&'static [Capability]>, + + /// The set of capabilities modules are permitted to use. + /// + /// This is initialized from `Options::capabilities`. + capabilities_available: Option>, + + /// The set of capabilities used by this module. + /// + /// If `capabilities_available` is `Some`, then this is always a subset of + /// that. + capabilities_used: crate::FastHashSet, + debugs: Vec, annotations: Vec, flags: WriterFlags, @@ -422,12 +432,16 @@ bitflags::bitflags! { pub struct Options { /// (Major, Minor) target version of the SPIR-V. pub lang_version: (u8, u8), + /// Configuration flags for the writer. pub flags: WriterFlags, - /// Set of SPIR-V allowed capabilities, if provided. - // Note: there is a major bug currently associated with deriving the capabilities. - // We are calling `required_capabilities`, but the semantics of this is broken. + + /// If given, the set of capabilities modules are allowed to use. Code that + /// requires capabilities beyond these is rejected with an error. + /// + /// If this is `None`, all capabilities are permitted. pub capabilities: Option>, + /// How should the generated code handle array, vector, or matrix indices /// that are out of range? pub index_bounds_check_policy: IndexBoundsCheckPolicy, diff --git a/src/back/spv/recyclable.rs b/src/back/spv/recyclable.rs index c1b7fdcc66..e8813497f3 100644 --- a/src/back/spv/recyclable.rs +++ b/src/back/spv/recyclable.rs @@ -42,3 +42,10 @@ impl Recyclable for std::collections::HashMap { self } } + +impl Recyclable for std::collections::HashSet { + fn recycle(mut self) -> Self { + self.clear(); + self + } +} diff --git a/src/back/spv/writer.rs b/src/back/spv/writer.rs index a53dc9cf6e..6a0c634cd1 100644 --- a/src/back/spv/writer.rs +++ b/src/back/spv/writer.rs @@ -50,15 +50,8 @@ impl Writer { } let raw_version = ((major as u32) << 16) | ((minor as u32) << 8); - let (capabilities, forbidden_caps) = match options.capabilities { - Some(ref caps) => (caps.clone(), None), - None => { - let mut caps = crate::FastHashSet::default(); - caps.insert(spirv::Capability::Shader); - let forbidden: &[_] = &[spirv::Capability::Kernel]; - (caps, Some(forbidden)) - } - }; + let mut capabilities_used = crate::FastHashSet::default(); + capabilities_used.insert(spirv::Capability::Shader); let mut id_gen = IdGenerator::default(); let gl450_ext_inst_id = id_gen.next(); @@ -68,8 +61,8 @@ impl Writer { physical_layout: PhysicalLayout::new(raw_version), logical_layout: LogicalLayout::default(), id_gen, - capabilities, - forbidden_caps, + capabilities_available: options.capabilities.clone(), + capabilities_used, debugs: vec![], annotations: vec![], flags: options.flags, @@ -110,8 +103,7 @@ impl Writer { // Copied from the old Writer: flags: self.flags, index_bounds_check_policy: self.index_bounds_check_policy, - capabilities: take(&mut self.capabilities), - forbidden_caps: take(&mut self.forbidden_caps), + capabilities_available: take(&mut self.capabilities_available), // Initialized afresh: id_gen, @@ -119,6 +111,7 @@ impl Writer { gl450_ext_inst_id, // Recycled: + capabilities_used: take(&mut self.capabilities_used).recycle(), physical_layout: self.physical_layout.clone().recycle(), logical_layout: take(&mut self.logical_layout).recycle(), debugs: take(&mut self.debugs).recycle(), @@ -134,27 +127,49 @@ impl Writer { }; *self = fresh; + + self.capabilities_used.insert(spirv::Capability::Shader); } - //TODO: revive and rewrite this, see: - // https://github.com/gfx-rs/rspirv/issues/198 - // https://github.com/gfx-rs/rspirv/pull/185#issuecomment-900796025 - pub(super) fn check(&mut self, capabilities: &[spirv::Capability]) -> Result<(), Error> { - if capabilities.is_empty() - || capabilities - .iter() - .any(|cap| self.capabilities.contains(cap)) - { - return Ok(()); - } - if let Some(forbidden) = self.forbidden_caps { - // take the first allowed capability, blindly - if let Some(&cap) = capabilities.iter().find(|cap| !forbidden.contains(cap)) { - self.capabilities.insert(cap); - return Ok(()); + /// Indicate that the code requires any one of the listed capabilities. + /// + /// If nothing in `capabilities` appears in the available capabilities + /// specified in the [`Options`] from which this `Writer` was created, + /// return an error. The `what` string is used in the error message to + /// explain what provoked the requirement. (If no available capabilites were + /// given, assume everything is available.) + /// + /// The first acceptable capability will be added to this `Writer`'s + /// [`capabilities_used`] table, and an `OpCapability` emitted for it in the + /// result. For this reason, more specific capabilities should be listed + /// before more general. + /// + /// [`capabilities_used`]: Writer::capabilities_used + pub(super) fn require_any( + &mut self, + what: &'static str, + capabilities: &[spirv::Capability], + ) -> Result<(), Error> { + match *capabilities { + [] => Ok(()), + [first, ..] => { + // Find the first acceptable capability, or return an error if + // there is none. + let selected = match self.capabilities_available { + None => first, + Some(ref available) => { + match capabilities.iter().find(|cap| available.contains(cap)) { + Some(&cap) => cap, + None => { + return Err(Error::MissingCapabilities(what, capabilities.to_vec())) + } + } + } + }; + self.capabilities_used.insert(selected); + Ok(()) } } - Err(Error::MissingCapabilities(capabilities.to_vec())) } pub(super) fn get_type_id(&mut self, lookup_ty: LookupType) -> Word { @@ -556,13 +571,13 @@ impl Writer { _ => None, }; if let Some(cap) = cap { - self.capabilities.insert(cap); + self.capabilities_used.insert(cap); } Instruction::type_int(id, bits, signedness) } Sk::Float => { if bits == 64 { - self.capabilities.insert(spirv::Capability::Float64); + self.capabilities_used.insert(spirv::Capability::Float64); } Instruction::type_float(id, bits) } @@ -705,18 +720,30 @@ impl Writer { let kind = match class { crate::ImageClass::Sampled { kind, multi: _ } => { if dim == crate::ImageDimension::D1 { - self.check(&[spirv::Capability::Sampled1D])?; + self.require_any("1D sampled images", &[spirv::Capability::Sampled1D])?; } kind } crate::ImageClass::Depth { multi: _ } => crate::ScalarKind::Float, crate::ImageClass::Storage { format, .. } => { - let required_caps: &[_] = match dim { - crate::ImageDimension::D1 => &[spirv::Capability::Image1D], - crate::ImageDimension::Cube => &[spirv::Capability::ImageCubeArray], - _ => &[], + match dim { + crate::ImageDimension::D1 => { + self.require_any( + "1D storage images", + &[spirv::Capability::Image1D], + )?; + } + crate::ImageDimension::Cube => { + if arrayed { + self.require_any( + "arrayed cube images", + &[spirv::Capability::ImageCubeArray], + )?; + } + } + _ => {} }; - self.check(required_caps)?; + format.into() } }; @@ -727,7 +754,6 @@ impl Writer { pointer_class: None, }; let dim = map_dim(dim); - //self.check(dim.required_capabilities())?; let type_id = self.get_type_id(LookupType::Local(local_type)); Instruction::type_image(id, type_id, dim, arrayed, class) } @@ -1013,6 +1039,10 @@ impl Writer { self.decorate(id, Decoration::Centroid, &[]); } Some(crate::Sampling::Sample) => { + self.require_any( + "per-sample interpolation", + &[spirv::Capability::SampleRateShading], + )?; self.decorate(id, Decoration::Sample, &[]); } } @@ -1039,10 +1069,20 @@ impl Writer { Bi::FragDepth => BuiltIn::FragDepth, Bi::FrontFacing => BuiltIn::FrontFacing, Bi::PrimitiveIndex => { - self.capabilities.insert(spirv::Capability::Geometry); + self.require_any( + "`primitive_index` built-in", + &[spirv::Capability::Geometry], + )?; BuiltIn::PrimitiveId } - Bi::SampleIndex => BuiltIn::SampleId, + Bi::SampleIndex => { + self.require_any( + "`sample_index` built-in", + &[spirv::Capability::SampleRateShading], + )?; + + BuiltIn::SampleId + } Bi::SampleMask => BuiltIn::SampleMask, // compute Bi::GlobalInvocationId => BuiltIn::GlobalInvocationId, @@ -1226,7 +1266,7 @@ impl Writer { ep_instruction.to_words(&mut self.logical_layout.entry_points); } - for capability in self.capabilities.iter() { + for capability in self.capabilities_used.iter() { Instruction::capability(*capability).to_words(&mut self.logical_layout.capabilities); } if ir_module.entry_points.is_empty() {