Skip to content

Fix OpStore of BDA to align to 8 #4123

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

Open
spencer-lunarg wants to merge 1 commit into
base: main
Choose a base branch
from
+102 −8
Open

Fix OpStore of BDA to align to 8 #4123

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
24 changes: 17 additions & 7 deletions SPIRV/SpvPostProcess.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
2 changes: 1 addition & 1 deletion Test/baseResults/spv.bufferhandle22.frag.out
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Copy link
Contributor Author

@spencer-lunarg spencer-lunarg Dec 11, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is

layout(buffer_reference) buffer MyBuffer;

struct S6 { MyBuffer x; };

layout(set = 0, binding = 1) restrict readonly buffer SSBO {
	S6 s6;
} InBuffer;

layout(buffer_reference) buffer MyBuffer {
	S6 s6;
};

void main() {
	MyBuffer payload;
	payload.s6 = InBuffer.s6;
}

which is storing a BDA pointer, but it was defaulting back to 16 before, so this is correct

Copy link

@maxime-modulopi maxime-modulopi Dec 12, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should still be 16 I believe. The default value for buffer_reference_align when unspecified is 16 and the first member of the implicit buffer struct can be proved to have the same alignment as the struct itself.
Other members may have smaller alignments depending on their offset in the struct (specified by the selected layout rule) though.

Copy link

@maxime-modulopi maxime-modulopi Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems like the correct formula to compute the highest guaranteed alignment for each member of a struct is gcd(struct_alignment, member_offset_in_the_struct) where gcd is the greatest common divisor.

Copy link
Contributor

@arcady-lunarg arcady-lunarg Dec 12, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alignments have to be powers of two so you don't need gcd, you can just use max.

Copy link

@maxime-modulopi maxime-modulopi Dec 13, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean max(struct_alignment, member_offset_in_the_struct)?
This does not work:

layout(buffer_reference, scalar) buffer Data
{
	uint64_t data0; // offset 0
	uint64_t data1; // offset 8
	uint64_t data2; // offset 16
	uint64_t data3; // offset 24
};

Correct:
gcd(16, 0) = 16
gcd(16, 8) = 8
gcd(16, 16) = 16
gcd(16, 24) = 8

Incorrect:
max(16, 0) = 16
max(16, 8) = 16
max(16, 16) = 16
max(16, 24) = 24

Copy link
Contributor Author

@spencer-lunarg spencer-lunarg Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, looked back into this, the issue is actually not the logic I added, it is doing things correctly IMO

The "real" issue here is this GLSL is generating code to copy the pointer, not the struct, but only because this test is --target-env vulkan1.1

If you set it to --target-env vulkan1.2 or higher, it will generate the correct SPIR-V such that the alignment is 8 as you would think

https://godbolt.org/z/d7n86soYd

So I agree there is an issue, but only for 1.1 or lower GLSL code path and its because it produces code that is copying a pointer, which should be aligned 8... trying to say "this PR is doing the logic correctly and there is nothing in the scope here to fix, but can raise a new issue to track the issue"

Copy link

@maxime-modulopi maxime-modulopi Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The pointer being 8 bytes, it indeed requires an alignment of at least 8 bytes.
However, OpLoad instructions should always use the largest alignment possible that can be compile-time-proved, for performance reasons (see Nvidia's Advanced API Performance: Descriptors blog).
From my understanding, this is the whole point of the existence of buffer_reference_align instead of auto-computing the alignment from the largest member.

In this example, MyBuffer does not set buffer_reference_align, which means the default value of 16 is used, as explained by GL_EXT_buffer_reference:

Each buffer reference type has an alignment that can be specified via
the "buffer_reference_align" layout qualifier. This must be a power of
two and be greater than or equal to the largest scalar/component type
in the block. If the layout qualifier is not specified, it defaults to
16 bytes.

Since the first member, s6, is at offset 0, we can prove that it has the same guaranteed alignment as the implicit struct (16) and thus should have loads/stores aligned to 16 bytes.

Copy link
Contributor Author

@spencer-lunarg spencer-lunarg Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thus should have loads/stores aligned to 16 bytes.

I agree 100% with you that it should be 16

I disagree there is anything to change in this PR, this PR is exposing a bug that is there already, which is --target-env vulkan1.1 is not producing correct BDA spirv, regardless of the Aligned value

Copy link

@maxime-modulopi maxime-modulopi Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will open another issue about buffer_reference_align being ignored for loads/stores.

Copy link
Contributor Author

@spencer-lunarg spencer-lunarg Dec 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@maxime-modulopi thanks, clearly there are still more things to get right, but happy to get it fixed little by little and issues with small code to reproduce is always helpful!

283: 6(ptr) Load 32(payload)
286: 285(ptr) AccessChain 52(InBuffer) 284
287: 49(S7) Load 286
Expand Down
62 changes: 62 additions & 0 deletions Test/baseResults/spv.bufferhandle26.frag.out
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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
21 changes: 21 additions & 0 deletions Test/spv.bufferhandle26.frag
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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;
}
1 change: 1 addition & 0 deletions gtests/Spv.FromFile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down