Skip to content

Commit 546afb3

Browse files
committed
Fix block layout to use std430 rules for StorageBuffer
The block layout validation was incorrectly applying std140 extended alignment (16-byte rounding for arrays/structs) to all storage classes. Std140 rules only apply to Uniform + Block decoration. StorageBuffer, PushConstant, PhysicalStorageBuffer, Workgroup with Block, and Uniform with BufferBlock all use std430 rules which have no 16-byte rounding. This matches the C++ SPIRV-Tools behavior in validate_decorations.cpp where blockRules (std140) is only true for Uniform + Block, and bufferRules (std430) is used for all other combinations.
1 parent ff64f65 commit 546afb3

2 files changed

Lines changed: 83 additions & 13 deletions

File tree

rust/spirv-tools-core/src/validation/rules/block_layout.rs

Lines changed: 27 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -48,6 +48,15 @@ impl ValidationRule for BlockLayoutRule {
4848
continue;
4949
}
5050

51+
// Std140 extended alignment (16-byte rounding for arrays/structs) only
52+
// applies to Uniform + Block decoration. All other combinations
53+
// (StorageBuffer + Block, BufferBlock + Uniform, PushConstant + Block, etc.)
54+
// use std430 rules which have no such rounding. This matches the C++
55+
// SPIRV-Tools behavior in validate_decorations.cpp (blockRules vs bufferRules).
56+
let is_uniform_block = block_info.decoration == BlockDecoration::Block
57+
&& block_info.storage_classes.contains(&StorageClass::Uniform);
58+
let extended_alignment = is_uniform_block && !relax_layout;
59+
5160
let Some(struct_inst) = ctx.definitions.get(&struct_id) else {
5261
continue;
5362
};
@@ -79,13 +88,12 @@ impl ValidationRule for BlockLayoutRule {
7988
let Some(member_inst) = ctx.definitions.get(&member_result_id) else {
8089
continue;
8190
};
82-
// Pass relax_layout to skip std140's 16-byte extended alignment rule
83-
// With relaxed or scalar layout, arrays/structs align to their natural alignment
8491
let Some(alignment) = type_alignment(
8592
member_type_id,
8693
ctx.definitions,
8794
&mut HashSet::new(),
8895
relax_layout,
96+
extended_alignment,
8997
) else {
9098
continue;
9199
};
@@ -507,21 +515,29 @@ fn type_alignment(
507515
definitions: &HashMap<ResultId, rspirv::dr::Instruction>,
508516
visiting: &mut HashSet<TypeId>,
509517
scalar_layout: bool,
518+
extended_alignment: bool,
510519
) -> Option<u32> {
511-
type_alignment_extended(ty, definitions, visiting, scalar_layout, false)
520+
type_alignment_extended(ty, definitions, visiting, scalar_layout, false, extended_alignment)
512521
}
513522

514523
/// Calculates type alignment with optional extended alignment for std140 rules.
515524
///
516-
/// In std140:
525+
/// In std140 (Uniform + Block):
517526
/// - Arrays and structs have their base alignment rounded up to 16 bytes
518527
/// - This is called "extended alignment"
528+
///
529+
/// In std430 (StorageBuffer + Block, BufferBlock + Uniform, PushConstant + Block, etc.):
530+
/// - No 16-byte rounding is applied
531+
///
532+
/// The `extended_alignment` parameter controls whether the 16-byte rounding applies.
533+
/// It should be `true` only for std140 (Uniform + Block) when not using relaxed/scalar layout.
519534
fn type_alignment_extended(
520535
ty: TypeId,
521536
definitions: &HashMap<ResultId, rspirv::dr::Instruction>,
522537
visiting: &mut HashSet<TypeId>,
523538
scalar_layout: bool,
524539
use_extended: bool,
540+
extended_alignment: bool,
525541
) -> Option<u32> {
526542
if !visiting.insert(ty) {
527543
return None;
@@ -536,7 +552,7 @@ fn type_alignment_extended(
536552
let (elem, count) = vector_info(inst);
537553
let (elem, count) = (elem?, count?);
538554
let elem_align =
539-
type_alignment_extended(elem, definitions, visiting, scalar_layout, false)?;
555+
type_alignment_extended(elem, definitions, visiting, scalar_layout, false, extended_alignment)?;
540556
if scalar_layout {
541557
Some(elem_align)
542558
} else {
@@ -549,17 +565,16 @@ fn type_alignment_extended(
549565
// Matrix alignment follows its column vector alignment.
550566
let (column, _) = matrix_info(inst);
551567
let column = column?;
552-
type_alignment_extended(column, definitions, visiting, scalar_layout, use_extended)
568+
type_alignment_extended(column, definitions, visiting, scalar_layout, use_extended, extended_alignment)
553569
}
554570
Op::TypeArray | Op::TypeRuntimeArray => {
555571
let elem = inst.operands.first().and_then(|op| match op {
556572
rspirv::dr::Operand::IdRef(id) => TypeId::try_from(*id).ok(),
557573
_ => None,
558574
})?;
559-
// In std140, array element alignment is rounded up to 16 bytes (extended alignment)
560575
let base_align =
561-
type_alignment_extended(elem, definitions, visiting, scalar_layout, true)?;
562-
if use_extended && !scalar_layout {
576+
type_alignment_extended(elem, definitions, visiting, scalar_layout, true, extended_alignment)?;
577+
if use_extended && extended_alignment && !scalar_layout {
563578
Some(round_up(base_align, 16))
564579
} else {
565580
Some(base_align)
@@ -573,11 +588,10 @@ fn type_alignment_extended(
573588
_ => return None,
574589
};
575590
let align =
576-
type_alignment_extended(ty, definitions, visiting, scalar_layout, true)?;
591+
type_alignment_extended(ty, definitions, visiting, scalar_layout, true, extended_alignment)?;
577592
max_align = max_align.max(align);
578593
}
579-
// In std140, struct alignment is rounded up to 16 bytes (extended alignment)
580-
if use_extended && !scalar_layout {
594+
if use_extended && extended_alignment && !scalar_layout {
581595
Some(round_up(max_align, 16))
582596
} else {
583597
Some(max_align)
@@ -597,7 +611,7 @@ fn vector_scalar_alignment(
597611
rspirv::dr::Operand::IdRef(id) => TypeId::try_from(*id).ok(),
598612
_ => None,
599613
})?;
600-
type_alignment(elem, definitions, &mut HashSet::new(), true)
614+
type_alignment(elem, definitions, &mut HashSet::new(), true, false)
601615
}
602616

603617
fn vector_info(inst: &rspirv::dr::Instruction) -> (Option<TypeId>, Option<u32>) {

rust/spirv-tools-core/src/validation/tests/mod.rs

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -14427,6 +14427,62 @@ fn array_stride_must_align_to_element() {
1442714427
assert!(matches!(err, ValidationError::InvalidBlockLayout { .. }));
1442814428
}
1442914429
#[test]
14430+
fn storage_buffer_array_stride_uses_std430_no_extended_alignment() {
14431+
// StorageBuffer + Block uses std430 rules: arrays do NOT get 16-byte
14432+
// extended alignment. An array of uint[2] with stride 8 should be valid
14433+
// because the element alignment is 4, and 8 % 4 == 0.
14434+
let text = [
14435+
"OpCapability Shader",
14436+
"OpMemoryModel Logical GLSL450",
14437+
"OpDecorate %struct Block",
14438+
"OpDecorate %var DescriptorSet 0",
14439+
"OpDecorate %var Binding 0",
14440+
"OpMemberDecorate %struct 0 Offset 0",
14441+
"OpDecorate %inner_arr ArrayStride 4",
14442+
"OpDecorate %outer_arr ArrayStride 8",
14443+
"%int = OpTypeInt 32 0",
14444+
"%two = OpConstant %int 2",
14445+
"%inner_arr = OpTypeArray %int %two",
14446+
"%outer_arr = OpTypeArray %inner_arr %two",
14447+
"%struct = OpTypeStruct %outer_arr",
14448+
"%ptr = OpTypePointer StorageBuffer %struct",
14449+
"%var = OpVariable %ptr StorageBuffer",
14450+
]
14451+
.join("\n");
14452+
text.as_str()
14453+
.validate(TargetEnv::Universal1_6)
14454+
.expect("StorageBuffer uses std430 rules where array stride 8 is valid");
14455+
}
14456+
#[test]
14457+
fn uniform_block_array_stride_requires_std140_extended_alignment() {
14458+
// Uniform + Block uses std140 rules: arrays get 16-byte extended alignment.
14459+
// An array of uint[2] with stride 8 should be rejected because std140
14460+
// rounds the array alignment up to 16, and 8 % 16 != 0.
14461+
let text = [
14462+
"OpCapability Shader",
14463+
"OpMemoryModel Logical GLSL450",
14464+
"OpDecorate %struct Block",
14465+
"OpDecorate %var DescriptorSet 0",
14466+
"OpDecorate %var Binding 0",
14467+
"OpMemberDecorate %struct 0 Offset 0",
14468+
"OpDecorate %inner_arr ArrayStride 4",
14469+
"OpDecorate %outer_arr ArrayStride 8",
14470+
"%int = OpTypeInt 32 0",
14471+
"%two = OpConstant %int 2",
14472+
"%inner_arr = OpTypeArray %int %two",
14473+
"%outer_arr = OpTypeArray %inner_arr %two",
14474+
"%struct = OpTypeStruct %outer_arr",
14475+
"%ptr = OpTypePointer Uniform %struct",
14476+
"%var = OpVariable %ptr Uniform",
14477+
]
14478+
.join("\n");
14479+
let err = text
14480+
.as_str()
14481+
.validate(TargetEnv::Universal1_6)
14482+
.expect_err("Uniform + Block uses std140 where array stride 8 is not aligned to 16");
14483+
assert!(matches!(err, ValidationError::InvalidBlockLayout { .. }));
14484+
}
14485+
#[test]
1443014486
fn vector_straddle_rejected_under_relax() {
1443114487
let text = [
1443214488
"OpCapability Shader",

0 commit comments

Comments
 (0)