Skip to content

Commit c2e54b0

Browse files
jimblandyclaude
andcommitted
spv-out: don't dynamically index PushConstant vectors with OpAccessChain
Per VUID-RuntimeSpirv-None-04745, all accesses into a PushConstant-storage-class variable that are arrays must use dynamically uniform indices; this restriction also applies to vectors. Naga was emitting `OpAccessChain` with per-invocation (non-uniform) indices directly into push-constant vectors, which is undefined behavior under that rule -- observed on RADV as every invocation silently reading element 0 of the vector. Add a special case to `write_checked_load`, alongside the existing matCx2 ones, that loads the whole vector and uses `OpVectorExtractDynamic` instead, which isn't subject to the restriction. Fixes gfx-rs#8612. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
1 parent ef79a92 commit c2e54b0

5 files changed

Lines changed: 237 additions & 1 deletion

naga/src/back/spv/block.rs

Lines changed: 85 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -429,6 +429,86 @@ impl BlockContext<'_> {
429429
block
430430
}
431431

432+
/// If `pointer` refers to a scalar reached by a dynamic (non-constant)
433+
/// index into a vector in the [`Immediate`] (push-constant) address
434+
/// space, write code to access the value, returning the ID of the
435+
/// result. Else return `None`.
436+
///
437+
/// `VUID-RuntimeSpirv-None-04745` requires that all accesses into a
438+
/// variable with a `PushConstant` storage class that are arrays use
439+
/// dynamically uniform indices; this restriction also applies to vectors.
440+
/// Naga IR, however, has no such restriction on indexing vectors in the
441+
/// `Immediate` address space. So Naga must not emit `OpAccessChain` with
442+
/// such an index directly into a `PushConstant` vector.
443+
///
444+
/// Instead, this loads the whole vector -- a plain `OpLoad`, which isn't
445+
/// subject to the restriction above -- and then extracts the desired
446+
/// component from that loaded value with `OpVectorExtractDynamic`, exactly
447+
/// as [`Self::write_vector_access()`] already does for by-value vectors.
448+
///
449+
/// [`Immediate`]: crate::AddressSpace::Immediate
450+
fn maybe_write_immediate_vector_dynamic_access(
451+
&mut self,
452+
pointer: Handle<crate::Expression>,
453+
block: &mut Block,
454+
) -> Result<Option<Word>, Error> {
455+
// We're only interested in a scalar reached by indexing into a
456+
// vector with a computed (not compile-time-constant) index.
457+
let crate::Expression::Access {
458+
base: vector_pointer,
459+
index,
460+
} = self.ir_function.expressions[pointer]
461+
else {
462+
return Ok(None);
463+
};
464+
465+
// If the index is actually a compile-time constant, the plain
466+
// access-chain path is fine: constants are as uniform as can be.
467+
if let GuardedIndex::Known(_) =
468+
GuardedIndex::from_expression(index, &self.ir_function.expressions, self.ir_module)
469+
{
470+
return Ok(None);
471+
}
472+
473+
// Ensure `vector_pointer` is a pointer to a vector in the Immediate
474+
// (push-constant) address space. Use helper functions, to handle both
475+
// `TypeInner::Pointer` and `TypeInner::ValuePointer`.
476+
let vector_pointer_ty = self.fun_info[vector_pointer]
477+
.ty
478+
.inner_with(&self.ir_module.types);
479+
if vector_pointer_ty.pointer_space() != Some(crate::AddressSpace::Immediate) {
480+
return Ok(None);
481+
}
482+
let Some(vector_base_ty) = vector_pointer_ty.pointer_base_type() else {
483+
return Ok(None);
484+
};
485+
let crate::TypeInner::Vector { size, scalar } =
486+
*vector_base_ty.inner_with(&self.ir_module.types)
487+
else {
488+
return Ok(None);
489+
};
490+
491+
let vector_type_id = self.get_numeric_type_id(NumericType::Vector { size, scalar });
492+
let component_type_id = self.get_numeric_type_id(NumericType::Scalar(scalar));
493+
494+
let vector_load_id = self.write_checked_load(
495+
vector_pointer,
496+
block,
497+
AccessTypeAdjustment::None,
498+
vector_type_id,
499+
)?;
500+
501+
let result_id = self.write_vector_access(
502+
component_type_id,
503+
vector_pointer,
504+
Some(vector_load_id),
505+
GuardedIndex::Expression(index),
506+
block,
507+
)?;
508+
509+
Ok(Some(result_id))
510+
}
511+
432512
/// If `pointer` refers to an access chain that contains a dynamic indexing
433513
/// of a two-row matrix in the [`Uniform`] address space, write code to
434514
/// access the value returning the ID of the result. Else return None.
@@ -2773,7 +2853,11 @@ impl BlockContext<'_> {
27732853
access_type_adjustment: AccessTypeAdjustment,
27742854
result_type_id: Word,
27752855
) -> Result<Word, Error> {
2776-
if let Some(result_id) = self.maybe_write_uniform_matcx2_dynamic_access(pointer, block)? {
2856+
if let Some(result_id) = self.maybe_write_immediate_vector_dynamic_access(pointer, block)? {
2857+
Ok(result_id)
2858+
} else if let Some(result_id) =
2859+
self.maybe_write_uniform_matcx2_dynamic_access(pointer, block)?
2860+
{
27772861
Ok(result_id)
27782862
} else if let Some(result_id) =
27792863
self.maybe_write_load_uniform_matcx2_struct_member(pointer, block)?
Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
capabilities = "IMMEDIATES"
2+
targets = "SPIRV"
3+
4+
[bounds_check_policies]
5+
index = "Restrict"
Lines changed: 43 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,43 @@
1+
// Regression test for <https://github.com/gfx-rs/wgpu/issues/8612>.
2+
//
3+
// The Vulkan environment spec (`VUID-RuntimeSpirv-None-04745`) requires
4+
// that accesses into a `PushConstant`-storage-class variable that are
5+
// arrays use dynamically uniform indices; this restriction also applies
6+
// to vectors. So naga must not emit `OpAccessChain` with a non-constant
7+
// index directly into a vector living in the `immediate` (push-constant)
8+
// address space -- it must instead load the whole vector and use
9+
// `OpVectorExtractDynamic`.
10+
//
11+
// Naming (mirrors `mat_cx2.wgsl`):
12+
// V = vector field, M = matrix field, C = constant index, trailing
13+
// C/V = constant/variable (dynamic) index into the vector/column.
14+
15+
struct Immediates {
16+
v: vec4<f32>,
17+
m: mat4x4<f32>,
18+
}
19+
20+
var<immediate> im: Immediates;
21+
22+
@group(0) @binding(0)
23+
var<storage, read_write> out: array<f32>;
24+
25+
@compute @workgroup_size(1)
26+
fn main(@builtin(local_invocation_index) idx: u32) {
27+
// Dynamically indexing a vector field directly. `im.v` is a
28+
// `TypeInner::Pointer` to a vector (it has a concrete entry in the
29+
// module's type arena, being a struct field).
30+
let v_c = im.v[0];
31+
let v_v = im.v[idx];
32+
33+
// Dynamically indexing a component of a matrix column. `im.m[0]` is a
34+
// `TypeInner::ValuePointer` to a vector (it doesn't have its own type
35+
// arena entry), which is a distinct code path from `v_v` above.
36+
let m_cc = im.m[0][0];
37+
let m_cv = im.m[0][idx];
38+
39+
out[0] = v_c;
40+
out[1] = v_v;
41+
out[2] = m_cc;
42+
out[3] = m_cv;
43+
}
Lines changed: 77 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,77 @@
1+
; SPIR-V
2+
; Version: 1.1
3+
; Generator: rspirv
4+
; Bound: 50
5+
OpCapability Shader
6+
OpExtension "SPV_KHR_storage_buffer_storage_class"
7+
%1 = OpExtInstImport "GLSL.std.450"
8+
OpMemoryModel Logical GLSL450
9+
OpEntryPoint GLCompute %19 "main" %16
10+
OpExecutionMode %19 LocalSize 1 1 1
11+
OpMemberDecorate %6 0 Offset 0
12+
OpMemberDecorate %6 1 Offset 16
13+
OpMemberDecorate %6 1 ColMajor
14+
OpMemberDecorate %6 1 MatrixStride 16
15+
OpDecorate %7 ArrayStride 4
16+
OpDecorate %10 Block
17+
OpMemberDecorate %10 0 Offset 0
18+
OpDecorate %12 DescriptorSet 0
19+
OpDecorate %12 Binding 0
20+
OpDecorate %13 Block
21+
OpMemberDecorate %13 0 Offset 0
22+
OpDecorate %16 BuiltIn LocalInvocationIndex
23+
%2 = OpTypeVoid
24+
%3 = OpTypeFloat 32
25+
%4 = OpTypeVector %3 4
26+
%5 = OpTypeMatrix %4 4
27+
%6 = OpTypeStruct %4 %5
28+
%7 = OpTypeRuntimeArray %3
29+
%8 = OpTypeInt 32 0
30+
%10 = OpTypeStruct %6
31+
%11 = OpTypePointer PushConstant %10
32+
%9 = OpVariable %11 PushConstant
33+
%13 = OpTypeStruct %7
34+
%14 = OpTypePointer StorageBuffer %13
35+
%12 = OpVariable %14 StorageBuffer
36+
%17 = OpTypePointer Input %8
37+
%16 = OpVariable %17 Input
38+
%20 = OpTypeFunction %2
39+
%21 = OpTypePointer PushConstant %6
40+
%22 = OpConstant %8 0
41+
%24 = OpTypePointer StorageBuffer %7
42+
%27 = OpTypePointer PushConstant %4
43+
%28 = OpTypePointer PushConstant %3
44+
%33 = OpConstant %8 3
45+
%36 = OpTypePointer PushConstant %5
46+
%37 = OpConstant %8 1
47+
%44 = OpTypePointer StorageBuffer %3
48+
%47 = OpConstant %8 2
49+
%19 = OpFunction %2 None %20
50+
%15 = OpLabel
51+
%18 = OpLoad %8 %16
52+
%23 = OpAccessChain %21 %9 %22
53+
%25 = OpAccessChain %24 %12 %22
54+
OpBranch %26
55+
%26 = OpLabel
56+
%29 = OpAccessChain %28 %23 %22 %22
57+
%30 = OpLoad %3 %29
58+
%31 = OpAccessChain %27 %23 %22
59+
%32 = OpLoad %4 %31
60+
%34 = OpExtInst %8 %1 UMin %18 %33
61+
%35 = OpVectorExtractDynamic %3 %32 %34
62+
%38 = OpAccessChain %28 %23 %37 %22 %22
63+
%39 = OpLoad %3 %38
64+
%40 = OpAccessChain %27 %23 %37 %22
65+
%41 = OpLoad %4 %40
66+
%42 = OpExtInst %8 %1 UMin %18 %33
67+
%43 = OpVectorExtractDynamic %3 %41 %42
68+
%45 = OpAccessChain %44 %25 %22
69+
OpStore %45 %30
70+
%46 = OpAccessChain %44 %25 %37
71+
OpStore %46 %35
72+
%48 = OpAccessChain %44 %25 %47
73+
OpStore %48 %39
74+
%49 = OpAccessChain %44 %25 %33
75+
OpStore %49 %43
76+
OpReturn
77+
OpFunctionEnd
Lines changed: 27 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,27 @@
1+
#version 460
2+
layout(local_size_x = 1, local_size_y = 1, local_size_z = 1) in;
3+
4+
struct _6
5+
{
6+
vec4 _m0;
7+
mat4 _m1;
8+
};
9+
10+
layout(set = 0, binding = 0, std430) buffer _13_12
11+
{
12+
float _m0[];
13+
} _12;
14+
15+
layout(push_constant, std430) uniform _10_9
16+
{
17+
_6 _m0;
18+
} _9;
19+
20+
void main()
21+
{
22+
_12._m0[0u] = _9._m0._m0.x;
23+
_12._m0[1u] = _9._m0._m0[min(gl_LocalInvocationIndex, 3u)];
24+
_12._m0[2u] = _9._m0._m1[0u].x;
25+
_12._m0[3u] = _9._m0._m1[0u][min(gl_LocalInvocationIndex, 3u)];
26+
}
27+

0 commit comments

Comments
 (0)