Skip to content
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.

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

spirv-val: Disallow stores according to VUID 06924 #5368

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

svenvh
Copy link
Member

@svenvh svenvh commented Aug 9, 2023

Ensure that the validator rejects stores to objects of types
OpTypeImage, OpTypeSampler, OpTypeSampledImage,
OpTypeAccelerationStructureKHR, and arrays of these types, according
to VUID-StandaloneSpirv-OpTypeImage-06924.

Guard the check behind the before_hlsl_legalization option, as
sometimes we may have temporaries or local variables that are expected
to get optimized away.

Fixes #4796

@svenvh
Copy link
Member Author

svenvh commented Aug 9, 2023

If I understand the CI failures correctly, HLSL-to-SPIR-V translation by glslang and shaderc seems to result in some violations of this VUID, as HLSL allows the construct. Any suggestions what to do here?

@alan-baker
Copy link
Contributor

What does glslang do in the GLSL path when you have a local acceleration structure variable?

@svenvh
Copy link
Member Author

svenvh commented Aug 10, 2023

GLSL allows acceleration structures (and images/samplers) only as uniforms or function parameters, and glslang diagnoses incorrect uses:

ERROR: 0:10: 'accelerationStructureNV' : accelerationStructureNV can only be used in uniform variables or function parameters

@alan-baker
Copy link
Contributor

Does DXC handle this? I expect this is a case where glslang is missing some legalization optimizations.

@spencer-lunarg
Copy link
Contributor

Does this check cases where there is a OpImageWrite through an OpLoad or and Atomic instructions?

@svenvh
Copy link
Member Author

svenvh commented Sep 25, 2023

Does this check cases where there is a OpImageWrite through an OpLoad or and Atomic instructions?

OpImageWrite is checked elsewhere (in ValidateImageWrite) as far as I can tell.

@svenvh
Copy link
Member Author

svenvh commented Apr 19, 2024

To progress on this, I've excluded validation of stores to objects of types OpTypeImage and OpTypeSampler.

@svenvh svenvh marked this pull request as ready for review April 19, 2024 13:25
@svenvh
Copy link
Member Author

svenvh commented Sep 20, 2024

Rebased onto latest main. @spencer-lunarg or @alan-baker would you be happy to give this a look again?

Copy link
Contributor

@alan-baker alan-baker left a comment

Choose a reason for hiding this comment

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

Just some updates to the message for now.

source/val/validate_memory.cpp Show resolved Hide resolved
@alan-baker
Copy link
Contributor

@s-perron Coming from a DXC perspective, should this check be gated by a pre legalization guard?

@s-perron
Copy link
Collaborator

I'm not sure. The OpTypeImage and OpTypeSampler should be guarded. We do not currently generate OpTypeSampledImage in a way that would require it, but I think that could happen in the near future.

I would say yes it we should check the flag. In general, the compiler may add temporaries or the user may have local variables that are expect to get optimized away.

This would be to solution to the issue with OpTypeImage and OpTypeSampler mentioned above. The output from the glslang HLSL->SPIRV should be validated with the pre-legalization option. If the checks are guarded by that option, then they should pass.

@svenvh
Copy link
Member Author

svenvh commented Sep 20, 2024

This would be to solution to the issue with OpTypeImage and OpTypeSampler mentioned above. The output from the glslang HLSL->SPIRV should be validated with the pre-legalization option. If the checks are guarded by that option, then they should pass.

@s-perron thanks for the suggestion, I'd much rather land the complete validation including OpTypeImage and OpTypeSampler. Is it options()->before_hlsl_legalization that you're referring to?

Ensure that the validator rejects stores to objects of types
`OpTypeImage`, `OpTypeSampler`, `OpTypeSampledImage`,
`OpTypeAccelerationStructureKHR`, and arrays of these types, according
to `VUID-StandaloneSpirv-OpTypeImage-06924`.

Guard the check behind the before_hlsl_legalization option, as
sometimes we may have temporaries or local variables that are expected
to get optimized away.

Fixes KhronosGroup#4796

Signed-off-by: Sven van Haastregt <sven.vanhaastregt@arm.com>
Change-Id: Ie035c01c5f94e7bdfc16b5c6c85705f302b7bda3
@svenvh
Copy link
Member Author

svenvh commented Sep 20, 2024

I've reinstated the full checks, including OpTypeImage and OpTypeSampler, now all guarded behind before_hlsl_legalization.

@alan-baker alan-baker merged commit 7ba72f1 into KhronosGroup:main Sep 23, 2024
24 checks passed
@svenvh svenvh deleted the vuid-06924 branch September 23, 2024 12:41
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.

No validation against OpStore being used to store to an object with OpTypeImage.
4 participants