Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[spv-out] Decorate binding array image/sample load result as NonUniform #2422

Merged
merged 4 commits into from
Aug 4, 2023
Merged

[spv-out] Decorate binding array image/sample load result as NonUniform #2422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
76d0c23
validate binding_array variable address space
teoxoy Aug 4, 2023
a6ee0d4
clarify binding array of handles comment
teoxoy Aug 4, 2023
263105d
[spv-out] decorate the result of the `OpLoad` with `NonUniform` (not …
teoxoy Aug 4, 2023
84acba0
[spv-out] rename `get_image_id` to `get_handle_id`
teoxoy Aug 4, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 29 additions & 8 deletions src/back/spv/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,7 @@ impl<'w> BlockContext<'w> {
crate::TypeInner::Vector { .. } => {
self.write_vector_access(expr_handle, base, index, block)?
}
// Only binding arrays in the Handle address space will take this path (due to `is_intermediate`)
crate::TypeInner::BindingArray {
base: binding_type, ..
} => {
Expand Down Expand Up @@ -308,6 +309,14 @@ impl<'w> BlockContext<'w> {
None,
));

// Subsequent image operations require the image/sampler to be decorated as NonUniform
// if the image/sampler binding array was accessed with a non-uniform index
// see VUID-RuntimeSpirv-NonUniform-06274
if self.fun_info[index].uniformity.non_uniform_result.is_some() {
self.writer
.decorate_non_uniform_binding_array_access(load_id)?;
}

load_id
}
ref other => {
Expand Down Expand Up @@ -347,6 +356,7 @@ impl<'w> BlockContext<'w> {
));
id
}
// Only binding arrays in the Handle address space will take this path (due to `is_intermediate`)
crate::TypeInner::BindingArray {
base: binding_type, ..
} => {
Expand Down Expand Up @@ -1426,8 +1436,8 @@ impl<'w> BlockContext<'w> {
) -> Result<ExpressionPointer, Error> {
let result_lookup_ty = match self.fun_info[expr_handle].ty {
TypeResolution::Handle(ty_handle) => match return_type_override {
// We use the return type override as a special case for binding arrays as the OpAccessChain
// needs to return a pointer, but indexing into a binding array just gives you the type of
// We use the return type override as a special case for handle binding arrays as the OpAccessChain
// needs to return a pointer, but indexing into a handle binding array just gives you the type of
// the binding in the IR.
Some(ty) => ty,
None => LookupType::Handle(ty_handle),
Expand All @@ -1454,12 +1464,20 @@ impl<'w> BlockContext<'w> {
if let crate::Expression::GlobalVariable(var_handle) =
self.ir_function.expressions[base]
{
let gvar = &self.ir_module.global_variables[var_handle];
if let crate::TypeInner::BindingArray { .. } =
self.ir_module.types[gvar.ty].inner
{
is_non_uniform_binding_array |=
self.fun_info[index].uniformity.non_uniform_result.is_some();
let gvar: &crate::GlobalVariable =
&self.ir_module.global_variables[var_handle];
match gvar.space {
crate::AddressSpace::Storage { .. } | crate::AddressSpace::Uniform => {
if let crate::TypeInner::BindingArray { .. } =
self.ir_module.types[gvar.ty].inner
{
is_non_uniform_binding_array = self.fun_info[index]
.uniformity
.non_uniform_result
.is_some();
}
}
_ => {}
}
}

Expand Down Expand Up @@ -1543,6 +1561,9 @@ impl<'w> BlockContext<'w> {
};
(pointer_id, expr_pointer)
};
// Subsequent load, store and atomic operations require the pointer to be decorated as NonUniform
// if the buffer binding array was accessed with a non-uniform index
// see VUID-RuntimeSpirv-NonUniform-06274
if is_non_uniform_binding_array {
self.writer
.decorate_non_uniform_binding_array_access(pointer_id)?;
Expand Down
12 changes: 6 additions & 6 deletions src/back/spv/image.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -379,7 +379,7 @@ impl<'w> BlockContext<'w> {
})
}

pub(super) fn get_image_id(&mut self, expr_handle: Handle<crate::Expression>) -> Word {
pub(super) fn get_handle_id(&mut self, expr_handle: Handle<crate::Expression>) -> Word {
let id = match self.ir_function.expressions[expr_handle] {
crate::Expression::GlobalVariable(handle) => {
self.writer.global_variables[handle.index()].handle_id
Expand Down Expand Up @@ -745,7 +745,7 @@ impl<'w> BlockContext<'w> {
sample: Option<Handle<crate::Expression>>,
block: &mut Block,
) -> Result<Word, Error> {
let image_id = self.get_image_id(image);
let image_id = self.get_handle_id(image);
let image_type = self.fun_info[image].ty.inner_with(&self.ir_module.types);
let image_class = match *image_type {
crate::TypeInner::Image { class, .. } => class,
Expand Down Expand Up @@ -830,7 +830,7 @@ impl<'w> BlockContext<'w> {
) -> Result<Word, Error> {
use super::instructions::SampleLod;
// image
let image_id = self.get_image_id(image);
let image_id = self.get_handle_id(image);
let image_type = self.fun_info[image].ty.handle().unwrap();
// SPIR-V doesn't know about our `Depth` class, and it returns
// `vec4<f32>`, so we need to grab the first component out of it.
Expand All @@ -857,7 +857,7 @@ impl<'w> BlockContext<'w> {
let sampled_image_type_id =
self.get_type_id(LookupType::Local(LocalType::SampledImage { image_type_id }));

let sampler_id = self.get_image_id(sampler);
let sampler_id = self.get_handle_id(sampler);
let coordinates_id = self
.write_image_coordinates(coordinate, array_index, block)?
.value_id;
Expand Down Expand Up @@ -1013,7 +1013,7 @@ impl<'w> BlockContext<'w> {
) -> Result<Word, Error> {
use crate::{ImageClass as Ic, ImageDimension as Id, ImageQuery as Iq};

let image_id = self.get_image_id(image);
let image_id = self.get_handle_id(image);
let image_type = self.fun_info[image].ty.handle().unwrap();
let (dim, arrayed, class) = match self.ir_module.types[image_type].inner {
crate::TypeInner::Image {
Expand Down Expand Up @@ -1164,7 +1164,7 @@ impl<'w> BlockContext<'w> {
value: Handle<crate::Expression>,
block: &mut Block,
) -> Result<(), Error> {
let image_id = self.get_image_id(image);
let image_id = self.get_handle_id(image);
let coordinates = self.write_image_coordinates(coordinate, array_index, block)?;
let value_id = self.cached[value];

Expand Down
2 changes: 1 addition & 1 deletion src/back/spv/ray.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ impl<'w> BlockContext<'w> {
} => {
//Note: composite extract indices and types must match `generate_ray_desc_type`
let desc_id = self.cached[descriptor];
let acc_struct_id = self.get_image_id(acceleration_structure);
let acc_struct_id = self.get_handle_id(acceleration_structure);
let width = 4;

let flag_type_id = self.get_type_id(LookupType::Local(LocalType::Value {
Expand Down
7 changes: 6 additions & 1 deletion src/valid/interface.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -409,7 +409,12 @@ impl super::Validator {
// A binding array is (mostly) supposed to behave the same as a
// series of individually bound resources, so we can (mostly)
// validate a `binding_array<T>` as if it were just a plain `T`.
crate::TypeInner::BindingArray { base, .. } => base,
crate::TypeInner::BindingArray { base, .. } => match var.space {
crate::AddressSpace::Storage { .. }
| crate::AddressSpace::Uniform
| crate::AddressSpace::Handle => base,
_ => return Err(GlobalVariableError::InvalidUsage(var.space)),
},
_ => var.ty,
};
let type_info = &self.types[inner_ty.index()];
Expand Down
44 changes: 22 additions & 22 deletions tests/out/spv/binding-arrays.spvasm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,28 +35,28 @@ OpMemberDecorate %47 0 Offset 0
OpDecorate %63 Location 0
OpDecorate %63 Flat
OpDecorate %66 Location 0
OpDecorate %95 NonUniform
OpDecorate %118 NonUniform
OpDecorate %120 NonUniform
OpDecorate %145 NonUniform
OpDecorate %147 NonUniform
OpDecorate %183 NonUniform
OpDecorate %211 NonUniform
OpDecorate %227 NonUniform
OpDecorate %243 NonUniform
OpDecorate %264 NonUniform
OpDecorate %266 NonUniform
OpDecorate %288 NonUniform
OpDecorate %290 NonUniform
OpDecorate %312 NonUniform
OpDecorate %314 NonUniform
OpDecorate %336 NonUniform
OpDecorate %338 NonUniform
OpDecorate %360 NonUniform
OpDecorate %362 NonUniform
OpDecorate %384 NonUniform
OpDecorate %386 NonUniform
OpDecorate %409 NonUniform
OpDecorate %96 NonUniform
OpDecorate %119 NonUniform
OpDecorate %121 NonUniform
OpDecorate %146 NonUniform
OpDecorate %148 NonUniform
OpDecorate %184 NonUniform
OpDecorate %212 NonUniform
OpDecorate %228 NonUniform
OpDecorate %244 NonUniform
OpDecorate %265 NonUniform
OpDecorate %267 NonUniform
OpDecorate %289 NonUniform
OpDecorate %291 NonUniform
OpDecorate %313 NonUniform
OpDecorate %315 NonUniform
OpDecorate %337 NonUniform
OpDecorate %339 NonUniform
OpDecorate %361 NonUniform
OpDecorate %363 NonUniform
OpDecorate %385 NonUniform
OpDecorate %387 NonUniform
OpDecorate %410 NonUniform
%2 = OpTypeVoid
%3 = OpTypeInt 32 0
%4 = OpTypeStruct %3
Expand Down