Skip to content

spirv-val: Fix Offset not checking if in struct#6701

Open
spencer-lunarg wants to merge 1 commit into
KhronosGroup:mainfrom
spencer-lunarg:spencer-lunarg-untyped-pointers-the-gift-that-will-give-for-years-offset-fix
Open

spirv-val: Fix Offset not checking if in struct#6701
spencer-lunarg wants to merge 1 commit into
KhronosGroup:mainfrom
spencer-lunarg:spencer-lunarg-untyped-pointers-the-gift-that-will-give-for-years-offset-fix

Conversation

@spencer-lunarg
Copy link
Copy Markdown
Contributor

closes #6696

seems we currently were also just not validating Offset as well


if (target->opcode() != spv::Op::OpDecorationGroup) {
if (IsMemberDecorationOnly(decoration)) {
// SPIR-V spec bug? (...example lost in time)
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... not sure where this is coming from, probably something when someone gets to #6594

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

... guess the smoke test found more

@alan-baker what do you want to do here, clearly something is wrong as the Offset is not following the

image

unless it was modified for all these various extension cases, but I guess don't understand what

layout(binding = 0, offset = 4) uniform atomic_uint countArr[4];

even means and where the memory for these offsets are all defined

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll want to double check this PR against CTS. I remember the lack of spec language and existing tests was frustrating.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

}

TEST_F(ValidateSpvEXTDescriptorHeap, BufferPointerEXTLayout) {
// TODO - Seems like VUID-StandaloneSpirv-Result-11346 is wrong because
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

OpMemberDecorate %U 0 Offset 0
OpDecorateId %_runtimearr_17 ArrayStrideIdEXT %18
OpDecorate %_ptr_UniformConstant Offset 0
; OpDecorate %_ptr_Uniform Offset 0
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Agree this shouldn't be there.

%_runtimearr_17 and %U do need explicit layouts (and have it). The pointers should not be laid out though.


if (target->opcode() != spv::Op::OpDecorationGroup) {
if (IsMemberDecorationOnly(decoration)) {
// SPIR-V spec bug? (...example lost in time)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

You'll want to double check this PR against CTS. I remember the lack of spec language and existing tests was frustrating.

@alan-baker
Copy link
Copy Markdown
Contributor

Looking at the dxc smoketest results, apparently this is needed for meshing shading too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

spirv-val: Missing OffsetIdEXT check on a struct

2 participants