Skip to content

Commit 573aa75

Browse files
jimblandyjimblandy
committed
[naga spv-out] Document and refactor write_runtime_array_length.
Document and refactor `naga::back::spv::BlockContext::write_runtime_array_length`. Don't try to handle finding the length of a particular element of a `binding_array<array<T>>`. The SPIR-V backend doesn't wrap that type correctly anyway; gfx-rs#6333 changes the validator to forbid such types. Instead, assume that the elements of a `binding_array<T>` are always structs whose final members may be a runtime-sized array. Pull out consistency checks after the analysis of the array expression, so that we always carry out all the checks regardless of what path we took to produce the information.
1 parent 974b3c3 commit 573aa75

File tree

1 file changed

+117
-55
lines changed

1 file changed

+117
-55
lines changed

‎naga/src/back/spv/index.rs

Lines changed: 117 additions & 55 deletions
Original file line numberDiff line numberDiff line change
@@ -38,98 +38,160 @@ impl<'w> BlockContext<'w> {
3838
///
3939
/// Given `array`, an expression referring a runtime-sized array, return the
4040
/// instruction id for the array's length.
41+
///
42+
/// Runtime-sized arrays may only appear in the values of global
43+
/// variables, which must have one of the following Naga types:
44+
/// 1) A runtime-sized array.
45+
/// 2) A struct whose last member is a runtime-sized array.
46+
/// 3) A binding array of 2).
47+
///
48+
/// Thus, the expression `array` has the form of:
49+
/// - An optional [`AccessIndex`], for case 2), applied to...
50+
/// - An optional [`Access`] or [`AccessIndex`], for case 3), applied to...
51+
/// - A [`GlobalVariable`].
52+
///
53+
/// The SPIR-V generated takes into account wrapped globals; see
54+
/// [`global_needs_wrapper`].
55+
///
56+
/// [`GlobalVariable`]: crate::Expression::GlobalVariable
57+
/// [`AccessIndex`]: crate::Expression::AccessIndex
58+
/// [`Access`]: crate::Expression::Access
59+
/// [`base`]: crate::Expression::Access::base
4160
pub(super) fn write_runtime_array_length(
4261
&mut self,
4362
array: Handle<crate::Expression>,
4463
block: &mut Block,
4564
) -> Result<Word, Error> {
46-
// Naga IR permits runtime-sized arrays as global variables, or as the
47-
// final member of a struct that is a global variable, or one of these
48-
// inside a buffer that is itself an element in a buffer bindings array.
49-
// SPIR-V requires that runtime-sized arrays are wrapped in structs.
50-
// See `helpers::global_needs_wrapper` and its uses.
51-
let (opt_array_index_id, global_handle, opt_last_member_index) = match self
52-
.ir_function
53-
.expressions[array]
54-
{
65+
// The index into the binding array, if any.
66+
let binding_array_index_id: Option<Word>;
67+
68+
// The handle to the Naga IR global we're referring to.
69+
let global_handle: Handle<crate::GlobalVariable>;
70+
71+
// At the Naga type level, if the runtime-sized array is the final member of a
72+
// struct, this is that member's index.
73+
//
74+
// This does not cover wrappers: if this backend wrapped the Naga global's
75+
// type in a synthetic SPIR-V struct (see `global_needs_wrapper`), this is
76+
// `None`.
77+
let opt_last_member_index: Option<u32>;
78+
79+
// Inspect `array` and decide whether we have a binding array and/or an
80+
// enclosing struct.
81+
match self.ir_function.expressions[array] {
5582
crate::Expression::AccessIndex { base, index } => {
5683
match self.ir_function.expressions[base] {
57-
// The global variable is an array of buffer bindings of structs,
58-
// we are accessing one of them with a static index,
59-
// and the last member of it.
6084
crate::Expression::AccessIndex {
6185
base: base_outer,
6286
index: index_outer,
6387
} => match self.ir_function.expressions[base_outer] {
88+
// An `AccessIndex` of an `AccessIndex` must be a
89+
// binding array holding structs whose last members are
90+
// runtime-sized arrays.
6491
crate::Expression::GlobalVariable(handle) => {
6592
let index_id = self.get_index_constant(index_outer);
66-
(Some(index_id), handle, Some(index))
93+
binding_array_index_id = Some(index_id);
94+
global_handle = handle;
95+
opt_last_member_index = Some(index);
96+
}
97+
_ => {
98+
return Err(Error::Validation(
99+
"array length expression: AccessIndex(AccessIndex(Global))",
100+
))
67101
}
68-
_ => return Err(Error::Validation("array length expression case-1a")),
69102
},
70-
// The global variable is an array of buffer bindings of structs,
71-
// we are accessing one of them with a dynamic index,
72-
// and the last member of it.
73103
crate::Expression::Access {
74104
base: base_outer,
75105
index: index_outer,
76106
} => match self.ir_function.expressions[base_outer] {
107+
// Similarly, an `AccessIndex` of an `Access` must be a
108+
// binding array holding structs whose last members are
109+
// runtime-sized arrays.
77110
crate::Expression::GlobalVariable(handle) => {
78111
let index_id = self.cached[index_outer];
79-
(Some(index_id), handle, Some(index))
112+
binding_array_index_id = Some(index_id);
113+
global_handle = handle;
114+
opt_last_member_index = Some(index);
115+
}
116+
_ => {
117+
return Err(Error::Validation(
118+
"array length expression: AccessIndex(Access(Global))",
119+
))
80120
}
81-
_ => return Err(Error::Validation("array length expression case-1b")),
82121
},
83-
// The global variable is a buffer, and we are accessing the last member.
84122
crate::Expression::GlobalVariable(handle) => {
85-
let global = &self.ir_module.global_variables[handle];
86-
match self.ir_module.types[global.ty].inner {
87-
// The global variable is an array of buffer bindings of run-time arrays.
88-
crate::TypeInner::BindingArray { .. } => (Some(index), handle, None),
89-
// The global variable is a struct, and we are accessing the last member
90-
_ => (None, handle, Some(index)),
91-
}
123+
// An outer `AccessIndex` applied directly to a
124+
// `GlobalVariable`. Since binding arrays can only contain
125+
// structs, this must be referring to the last member of a
126+
// struct that is a runtime-sized array.
127+
binding_array_index_id = None;
128+
global_handle = handle;
129+
opt_last_member_index = Some(index);
92130
}
93-
_ => return Err(Error::Validation("array length expression case-1c")),
94-
}
95-
}
96-
// The global variable is an array of buffer bindings of arrays.
97-
crate::Expression::Access { base, index } => match self.ir_function.expressions[base] {
98-
crate::Expression::GlobalVariable(handle) => {
99-
let index_id = self.cached[index];
100-
let global = &self.ir_module.global_variables[handle];
101-
match self.ir_module.types[global.ty].inner {
102-
crate::TypeInner::BindingArray { .. } => (Some(index_id), handle, None),
103-
_ => return Err(Error::Validation("array length expression case-2a")),
131+
_ => {
132+
return Err(Error::Validation(
133+
"array length expression: AccessIndex(<unexpected>)",
134+
))
104135
}
105136
}
106-
_ => return Err(Error::Validation("array length expression case-2b")),
107-
},
108-
// The global variable is a run-time array.
137+
}
109138
crate::Expression::GlobalVariable(handle) => {
110-
let global = &self.ir_module.global_variables[handle];
111-
if !global_needs_wrapper(self.ir_module, global) {
112-
return Err(Error::Validation("array length expression case-3"));
113-
}
114-
(None, handle, None)
139+
// A direct reference to a global variable. This must hold the
140+
// runtime-sized array directly.
141+
binding_array_index_id = None;
142+
global_handle = handle;
143+
opt_last_member_index = None;
115144
}
116145
_ => return Err(Error::Validation("array length expression case-4")),
117146
};
118147

148+
// The verifier should have checked this, but make sure the inspection above
149+
// agrees with the type about whether a binding array is involved.
150+
//
151+
// Eventually we do want to support `binding_array<array<T>>`. This check
152+
// ensures that whoever relaxes the validator will get an error message from
153+
// us, not just bogus SPIR-V.
154+
let global = &self.ir_module.global_variables[global_handle];
155+
match (
156+
&self.ir_module.types[global.ty].inner,
157+
binding_array_index_id,
158+
) {
159+
(&crate::TypeInner::BindingArray { .. }, Some(_)) => {}
160+
(_, None) => {}
161+
_ => {
162+
return Err(Error::Validation(
163+
"array length expression: bad binding array inference",
164+
))
165+
}
166+
}
167+
168+
// SPIR-V allows runtime-sized arrays to appear only as the last member of a
169+
// struct. Determine this member's index.
119170
let gvar = self.writer.global_variables[global_handle].clone();
120171
let global = &self.ir_module.global_variables[global_handle];
121-
let (last_member_index, gvar_id) = match opt_last_member_index {
122-
Some(index) => (index, gvar.access_id),
123-
None => {
124-
if !global_needs_wrapper(self.ir_module, global) {
125-
return Err(Error::Validation(
126-
"pointer to a global that is not a wrapped array",
127-
));
128-
}
172+
let needs_wrapper = global_needs_wrapper(self.ir_module, global);
173+
let (last_member_index, gvar_id) = match (opt_last_member_index, needs_wrapper) {
174+
(Some(index), false) => {
175+
// At the Naga type level, the runtime-sized array appears as the
176+
// final member of a struct, whose index is `index`. We didn't need to
177+
// wrap this, since the Naga type meets SPIR-V's requirements already.
178+
(index, gvar.access_id)
179+
}
180+
(None, true) => {
181+
// At the Naga type level, the runtime-sized array does not appear
182+
// within a struct. We wrapped this in an OpTypeStruct with nothing
183+
// else in it, so the index is zero. OpArrayLength wants the pointer
184+
// to the wrapper struct, so use `gvar.var_id`.
129185
(0, gvar.var_id)
130186
}
187+
_ => {
188+
return Err(Error::Validation(
189+
"array length expression: bad SPIR-V wrapper struct inference",
190+
));
191+
}
131192
};
132-
let structure_id = match opt_array_index_id {
193+
194+
let structure_id = match binding_array_index_id {
133195
// We are indexing inside a binding array, generate the access op.
134196
Some(index_id) => {
135197
let element_type_id = match self.ir_module.types[global.ty].inner {

0 commit comments

Comments
 (0)