From 7cd236fd9bd8fecd24e5a882a03f178fd9b6e1b1 Mon Sep 17 00:00:00 2001 From: spencer-lunarg Date: Thu, 11 Dec 2025 14:20:49 -0500 Subject: [PATCH] Fix OpStore of BDA to align to 8 Tracks if OpStore is storing a buffer_reference pointer and if so, sets the alignment to be 8 like it does currently for loads --- SPIRV/SpvPostProcess.cpp | 24 +++++--- Test/baseResults/spv.bufferhandle22.frag.out | 2 +- Test/baseResults/spv.bufferhandle26.frag.out | 62 ++++++++++++++++++++ Test/spv.bufferhandle26.frag | 21 +++++++ gtests/Spv.FromFile.cpp | 1 + 5 files changed, 102 insertions(+), 8 deletions(-) create mode 100644 Test/baseResults/spv.bufferhandle26.frag.out create mode 100644 Test/spv.bufferhandle26.frag diff --git a/SPIRV/SpvPostProcess.cpp b/SPIRV/SpvPostProcess.cpp index 068e33e82e..6eb1a116e4 100644 --- a/SPIRV/SpvPostProcess.cpp +++ b/SPIRV/SpvPostProcess.cpp @@ -353,16 +353,26 @@ void Builder::postProcess(Instruction& inst) // Merge new and old (mis)alignment alignment |= inst.getImmediateOperand(alignmentIdx); - if (!is_store) { - Instruction* inst_type = module.getInstruction(inst.getTypeId()); + Instruction* inst_type = nullptr; + if (is_store) { + // For a store, we are storing through a pointer, so what this ends up looking like when storing a BDA pointer is we have a "OpTypePointer PhysicalStorageBuffer" pointing at the actual "OpTypePointer PhysicalStorageBuffer" value storing + // https://godbolt.org/z/58ff4P3sG + inst_type = module.getInstruction(accessChain->getTypeId()); if (inst_type->getOpCode() == Op::OpTypePointer && inst_type->getImmediateOperand(0) == StorageClass::PhysicalStorageBuffer) { - // This means we are loading a pointer which means need to ensure it is at least 8-byte aligned - // See https://github.com/KhronosGroup/glslang/issues/4084 - // In case the alignment is currently 4, need to ensure it is 8 before grabbing the LSB - alignment |= 8; - alignment &= 8; + inst_type = module.getInstruction(inst_type->getIdOperand(1)); } + } else { + inst_type = module.getInstruction(inst.getTypeId()); + } + + if (inst_type->getOpCode() == Op::OpTypePointer && + inst_type->getImmediateOperand(0) == StorageClass::PhysicalStorageBuffer) { + // This means we are loading a pointer which means need to ensure it is at least 8-byte aligned + // See https://github.com/KhronosGroup/glslang/issues/4084 + // In case the alignment is currently 4, need to ensure it is 8 before grabbing the LSB + alignment |= 8; + alignment &= 8; } // Pick the LSB diff --git a/Test/baseResults/spv.bufferhandle22.frag.out b/Test/baseResults/spv.bufferhandle22.frag.out index 18fecf4307..39351a1caa 100644 --- a/Test/baseResults/spv.bufferhandle22.frag.out +++ b/Test/baseResults/spv.bufferhandle22.frag.out @@ -470,7 +470,7 @@ spv.bufferhandle22.frag 279: 278(ptr) AccessChain 273 274 280: 6(ptr) CompositeExtract 277 0 282: 281(ptr) AccessChain 279 35 - Store 282 280 Aligned 16 + Store 282 280 Aligned 8 283: 6(ptr) Load 32(payload) 286: 285(ptr) AccessChain 52(InBuffer) 284 287: 49(S7) Load 286 diff --git a/Test/baseResults/spv.bufferhandle26.frag.out b/Test/baseResults/spv.bufferhandle26.frag.out new file mode 100644 index 0000000000..d0c50c6101 --- /dev/null +++ b/Test/baseResults/spv.bufferhandle26.frag.out @@ -0,0 +1,62 @@ +spv.bufferhandle26.frag +// Module Version 10000 +// Generated by (magic number): 8000b +// Id's are bound by 27 + + Capability Shader + Capability PhysicalStorageBufferAddressesEXT + Extension "SPV_KHR_physical_storage_buffer" + 1: ExtInstImport "GLSL.std.450" + MemoryModel PhysicalStorageBuffer64EXT GLSL450 + EntryPoint Fragment 4 "main" + ExecutionMode 4 OriginUpperLeft + Source GLSL 460 + SourceExtension "GL_EXT_buffer_reference" + SourceExtension "GL_EXT_scalar_block_layout" + Name 4 "main" + Name 7 "constants" + MemberName 7(constants) 0 "u_param" + Name 9 "ParametersBuffer" + MemberName 9(ParametersBuffer) 0 "src" + MemberName 9(ParametersBuffer) 1 "dst" + Name 12 "DataBuffer" + MemberName 12(DataBuffer) 0 "data" + Name 14 "" + Decorate 7(constants) Block + MemberDecorate 7(constants) 0 Offset 0 + Decorate 9(ParametersBuffer) Block + MemberDecorate 9(ParametersBuffer) 0 Offset 0 + MemberDecorate 9(ParametersBuffer) 1 Offset 8 + Decorate 12(DataBuffer) Block + MemberDecorate 12(DataBuffer) 0 NonWritable + MemberDecorate 12(DataBuffer) 0 Offset 0 + 2: TypeVoid + 3: TypeFunction 2 + TypeForwardPointer 6 PhysicalStorageBufferEXT + 7(constants): TypeStruct 6 + TypeForwardPointer 8 PhysicalStorageBufferEXT +9(ParametersBuffer): TypeStruct 8 8 + 10: TypeFloat 32 + 11: TypeVector 10(float) 4 + 12(DataBuffer): TypeStruct 11(fvec4) + 8: TypePointer PhysicalStorageBufferEXT 12(DataBuffer) + 6: TypePointer PhysicalStorageBufferEXT 9(ParametersBuffer) + 13: TypePointer PushConstant 7(constants) + 14: 13(ptr) Variable PushConstant + 15: TypeInt 32 1 + 16: 15(int) Constant 0 + 17: TypePointer PushConstant 6(ptr) + 20: 15(int) Constant 1 + 23: TypePointer PhysicalStorageBufferEXT 8(ptr) + 4(main): 2 Function None 3 + 5: Label + 18: 17(ptr) AccessChain 14 16 + 19: 6(ptr) Load 18 + 21: 17(ptr) AccessChain 14 16 + 22: 6(ptr) Load 21 + 24: 23(ptr) AccessChain 22 16 + 25: 8(ptr) Load 24 Aligned 8 + 26: 23(ptr) AccessChain 19 20 + Store 26 25 Aligned 8 + Return + FunctionEnd diff --git a/Test/spv.bufferhandle26.frag b/Test/spv.bufferhandle26.frag new file mode 100644 index 0000000000..e2adefd6c7 --- /dev/null +++ b/Test/spv.bufferhandle26.frag @@ -0,0 +1,21 @@ +#version 460 core + +#extension GL_EXT_buffer_reference: require +#extension GL_EXT_scalar_block_layout: require + +layout (buffer_reference, buffer_reference_align = 4, scalar) readonly buffer DataBuffer { + vec4 data; +}; + +layout (buffer_reference, buffer_reference_align = 8, scalar) buffer ParametersBuffer { + DataBuffer src; + DataBuffer dst; +}; + +layout(push_constant) uniform constants { + ParametersBuffer u_param; +}; + +void main() { + u_param.dst = u_param.src; +} \ No newline at end of file diff --git a/gtests/Spv.FromFile.cpp b/gtests/Spv.FromFile.cpp index 508f3f013e..b357ca831f 100644 --- a/gtests/Spv.FromFile.cpp +++ b/gtests/Spv.FromFile.cpp @@ -393,6 +393,7 @@ INSTANTIATE_TEST_SUITE_P( "spv.bufferhandle23.frag", "spv.bufferhandle24.frag", "spv.bufferhandle25.frag", + "spv.bufferhandle26.frag", "spv.bufferhandleRuntimeArray.frag", "spv.bufferhandleUvec2.frag", "spv.bufferhandle_Error.frag",