Skip to content

Commit

Permalink
Derive storage bindings via naga::StorageAccess instead of `naga::G…
Browse files Browse the repository at this point in the history 
…lobalUse` (#3985)

Removes the feature check for read-only and read-write storage textures as it's later checked by `create_bind_group_layout`.

https://github.com/gfx-rs/wgpu/blob/335f40f85a492a5d9a69eda6835a30173f2e6b03/wgpu-core/src/device/resource.rs#L1505-L1518

Also removes the `GlobalUse` checks from `check_binding_use` as naga is already checking those.

https://github.com/gfx-rs/naga/blob/535701f6b2f38f7e0088701ee4cf6d8024f74a21/src/valid/interface.rs#L639-L669
  • Loading branch information
@teoxoy
teoxoy authored Jul 31, 2023
1 parent 28334ac commit 48078a8
Show file tree
Hide file tree
Showing 3 changed files with 61 additions and 118 deletions.
4 changes: 4 additions & 0 deletions CHANGELOG.md
View file
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,10 @@ Bottom level categories:

### Bug Fixes

#### General

- Derive storage bindings via `naga::StorageAccess` instead of `naga::GlobalUse`. By @teoxoy in [#3985](https://github.com/gfx-rs/wgpu/pull/3985).

#### Vulkan
- Fix enabling `wgpu::Features::PARTIALLY_BOUND_BINDING_ARRAY` not being actually enabled in vulkan backend. By @39ali in[#3772](https://github.com/gfx-rs/wgpu/pull/3772).

Expand Down
3 changes: 1 addition & 2 deletions wgpu-core/src/device/resource.rs
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1285,8 +1285,7 @@ impl<A: HalApi> Device<A> {
inner: Box::new(inner),
})
})?;
let interface =
validation::Interface::new(&module, &info, self.features, self.limits.clone());
let interface = validation::Interface::new(&module, &info, self.limits.clone());
let hal_shader = hal::ShaderInput::Naga(hal::NagaShader { module, info });

let hal_desc = hal::ShaderModuleDescriptor {
Expand Down
172 changes: 56 additions & 116 deletions wgpu-core/src/validation.rs
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use crate::{binding_model::BindEntryMap, FastHashMap, FastHashSet};
use naga::valid::GlobalUse;
use std::{collections::hash_map::Entry, fmt};
use thiserror::Error;
use wgt::{BindGroupLayoutEntry, BindingType};
Expand Down Expand Up @@ -112,7 +111,7 @@ struct SpecializationConstant {
struct EntryPoint {
inputs: Vec<Varying>,
outputs: Vec<Varying>,
resources: Vec<(naga::Handle<Resource>, GlobalUse)>,
resources: Vec<naga::Handle<Resource>>,
#[allow(unused)]
spec_constants: Vec<SpecializationConstant>,
sampling_pairs: FastHashSet<(naga::Handle<Resource>, naga::Handle<Resource>)>,
Expand All @@ -121,7 +120,6 @@ struct EntryPoint {

#[derive(Debug)]
pub struct Interface {
features: wgt::Features,
limits: wgt::Limits,
resources: naga::Arena<Resource>,
entry_points: FastHashMap<(naga::ShaderStage, String), EntryPoint>,
Expand Down Expand Up @@ -174,11 +172,6 @@ pub enum BindingError {
Missing,
#[error("Visibility flags don't include the shader stage")]
Invisible,
#[error("The shader requires the load/store access flags {required:?} but only {allowed:?} is allowed")]
WrongUsage {
required: GlobalUse,
allowed: GlobalUse,
},
#[error("Type on the shader side does not match the pipeline binding")]
WrongType,
#[error("Storage class {binding:?} doesn't match the shader {shader:?}")]
Expand Down Expand Up @@ -206,9 +199,9 @@ pub enum BindingError {
#[error("Texture format {0:?} is not supported for storage use")]
BadStorageFormat(wgt::TextureFormat),
#[error(
"Storage texture usage {0:?} doesn't have a matching supported `StorageTextureAccess`"
"Storage texture with access {0:?} doesn't have a matching supported `StorageTextureAccess`"
)]
UnsupportedTextureStorageAccess(GlobalUse),
UnsupportedTextureStorageAccess(naga::StorageAccess),
}

#[derive(Clone, Debug, Error)]
Expand Down Expand Up @@ -379,34 +372,23 @@ fn map_storage_format_from_naga(format: naga::StorageFormat) -> wgt::TextureForm
}

impl Resource {
fn check_binding_use(
&self,
entry: &BindGroupLayoutEntry,
shader_usage: GlobalUse,
) -> Result<(), BindingError> {
let allowed_usage = match self.ty {
fn check_binding_use(&self, entry: &BindGroupLayoutEntry) -> Result<(), BindingError> {
match self.ty {
ResourceType::Buffer { size } => {
let (allowed_usage, min_size) = match entry.ty {
let min_size = match entry.ty {
BindingType::Buffer {
ty,
has_dynamic_offset: _,
min_binding_size,
} => {
let (class, global_use) = match ty {
wgt::BufferBindingType::Uniform => {
(naga::AddressSpace::Uniform, GlobalUse::READ)
}
let class = match ty {
wgt::BufferBindingType::Uniform => naga::AddressSpace::Uniform,
wgt::BufferBindingType::Storage { read_only } => {
let mut global_use = GlobalUse::READ | GlobalUse::QUERY;
global_use.set(GlobalUse::WRITE, !read_only);
let mut naga_access = naga::StorageAccess::LOAD;
naga_access.set(naga::StorageAccess::STORE, !read_only);
(
naga::AddressSpace::Storage {
access: naga_access,
},
global_use,
)
naga::AddressSpace::Storage {
access: naga_access,
}
}
};
if self.class != class {
Expand All @@ -415,7 +397,7 @@ impl Resource {
shader: self.class,
});
}
(global_use, min_binding_size)
min_binding_size
}
_ => return Err(BindingError::WrongType),
};
Expand All @@ -425,13 +407,10 @@ impl Resource {
}
_ => (),
}
allowed_usage
}
ResourceType::Sampler { comparison } => match entry.ty {
BindingType::Sampler(ty) => {
if (ty == wgt::SamplerBindingType::Comparison) == comparison {
GlobalUse::READ
} else {
if (ty == wgt::SamplerBindingType::Comparison) != comparison {
return Err(BindingError::WrongSamplerComparison);
}
}
Expand Down Expand Up @@ -480,56 +459,42 @@ impl Resource {
}
}
}
let (expected_class, usage) = match entry.ty {
let expected_class = match entry.ty {
BindingType::Texture {
sample_type,
view_dimension: _,
multisampled: multi,
} => {
let class = match sample_type {
wgt::TextureSampleType::Float { .. } => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Float,
multi,
},
wgt::TextureSampleType::Sint => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Sint,
multi,
},
wgt::TextureSampleType::Uint => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Uint,
multi,
},
wgt::TextureSampleType::Depth => naga::ImageClass::Depth { multi },
};
(class, GlobalUse::READ | GlobalUse::QUERY)
}
} => match sample_type {
wgt::TextureSampleType::Float { .. } => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Float,
multi,
},
wgt::TextureSampleType::Sint => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Sint,
multi,
},
wgt::TextureSampleType::Uint => naga::ImageClass::Sampled {
kind: naga::ScalarKind::Uint,
multi,
},
wgt::TextureSampleType::Depth => naga::ImageClass::Depth { multi },
},
BindingType::StorageTexture {
access,
format,
view_dimension: _,
} => {
let naga_format = map_storage_format_to_naga(format)
.ok_or(BindingError::BadStorageFormat(format))?;
let (naga_access, usage) = match access {
wgt::StorageTextureAccess::ReadOnly => (
naga::StorageAccess::LOAD,
GlobalUse::READ | GlobalUse::QUERY,
),
wgt::StorageTextureAccess::WriteOnly => (
naga::StorageAccess::STORE,
GlobalUse::WRITE | GlobalUse::QUERY,
),
wgt::StorageTextureAccess::ReadWrite => {
(naga::StorageAccess::all(), GlobalUse::all())
}
let naga_access = match access {
wgt::StorageTextureAccess::ReadOnly => naga::StorageAccess::LOAD,
wgt::StorageTextureAccess::WriteOnly => naga::StorageAccess::STORE,
wgt::StorageTextureAccess::ReadWrite => naga::StorageAccess::all(),
};
(
naga::ImageClass::Storage {
format: naga_format,
access: naga_access,
},
usage,
)
naga::ImageClass::Storage {
format: naga_format,
access: naga_access,
}
}
_ => return Err(BindingError::WrongType),
};
Expand All @@ -539,31 +504,19 @@ impl Resource {
shader: class,
});
}
usage
}
};

if allowed_usage.contains(shader_usage) {
Ok(())
} else {
Err(BindingError::WrongUsage {
required: shader_usage,
allowed: allowed_usage,
})
}
Ok(())
}

fn derive_binding_type(
&self,
shader_usage: GlobalUse,
features: wgt::Features,
) -> Result<BindingType, BindingError> {
fn derive_binding_type(&self) -> Result<BindingType, BindingError> {
Ok(match self.ty {
ResourceType::Buffer { size } => BindingType::Buffer {
ty: match self.class {
naga::AddressSpace::Uniform => wgt::BufferBindingType::Uniform,
naga::AddressSpace::Storage { .. } => wgt::BufferBindingType::Storage {
read_only: !shader_usage.contains(GlobalUse::WRITE),
naga::AddressSpace::Storage { access } => wgt::BufferBindingType::Storage {
read_only: access == naga::StorageAccess::LOAD,
},
_ => return Err(BindingError::WrongType),
},
Expand Down Expand Up @@ -606,19 +559,15 @@ impl Resource {
view_dimension,
multisampled: multi,
},
naga::ImageClass::Storage { format, .. } => BindingType::StorageTexture {
access: if !shader_usage.contains(GlobalUse::READ) {
wgt::StorageTextureAccess::WriteOnly
} else if !features
.contains(wgt::Features::TEXTURE_ADAPTER_SPECIFIC_FORMAT_FEATURES)
{
return Err(BindingError::UnsupportedTextureStorageAccess(
shader_usage,
));
} else if shader_usage.contains(GlobalUse::WRITE) {
wgt::StorageTextureAccess::ReadWrite
} else {
wgt::StorageTextureAccess::ReadOnly
naga::ImageClass::Storage { format, access } => BindingType::StorageTexture {
access: {
const LOAD_STORE: naga::StorageAccess = naga::StorageAccess::all();
match access {
naga::StorageAccess::LOAD => wgt::StorageTextureAccess::ReadOnly,
naga::StorageAccess::STORE => wgt::StorageTextureAccess::WriteOnly,
LOAD_STORE => wgt::StorageTextureAccess::ReadWrite,
_ => unreachable!(),
}
},
view_dimension,
format: {
Expand Down Expand Up @@ -880,12 +829,7 @@ impl Interface {
list.push(varying);
}

pub fn new(
module: &naga::Module,
info: &naga::valid::ModuleInfo,
features: wgt::Features,
limits: wgt::Limits,
) -> Self {
pub fn new(module: &naga::Module, info: &naga::valid::ModuleInfo, limits: wgt::Limits) -> Self {
let mut resources = naga::Arena::new();
let mut resource_mapping = FastHashMap::default();
for (var_handle, var) in module.global_variables.iter() {
Expand Down Expand Up @@ -949,11 +893,8 @@ impl Interface {

for (var_handle, var) in module.global_variables.iter() {
let usage = info[var_handle];
if usage.is_empty() {
continue;
}
if var.binding.is_some() {
ep.resources.push((resource_mapping[&var_handle], usage));
if !usage.is_empty() && var.binding.is_some() {
ep.resources.push(resource_mapping[&var_handle]);
}
}

Expand All @@ -968,7 +909,6 @@ impl Interface {
}

Self {
features,
limits,
resources,
entry_points,
Expand Down Expand Up @@ -1000,7 +940,7 @@ impl Interface {
.ok_or(StageError::MissingEntryPoint(pair.1))?;

// check resources visibility
for &(handle, usage) in entry_point.resources.iter() {
for &handle in entry_point.resources.iter() {
let res = &self.resources[handle];
let result = match given_layouts {
Some(layouts) => {
Expand All @@ -1026,13 +966,13 @@ impl Interface {
Err(BindingError::Invisible)
}
})
.and_then(|entry| res.check_binding_use(entry, usage))
.and_then(|entry| res.check_binding_use(entry))
}
None => derived_layouts
.get_mut(res.bind.group as usize)
.ok_or(BindingError::Missing)
.and_then(|set| {
let ty = res.derive_binding_type(usage, self.features)?;
let ty = res.derive_binding_type()?;
match set.entry(res.bind.binding) {
Entry::Occupied(e) if e.get().ty != ty => {
return Err(BindingError::InconsistentlyDerivedType)
Expand Down

0 comments on commit 48078a8

Please sign in to comment.