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

OpCopyObject lets you sidestep OpSampledImage validation #5830

Open
spencer-lunarg opened this issue Sep 29, 2024 · 1 comment
Open

OpCopyObject lets you sidestep OpSampledImage validation #5830

spencer-lunarg opened this issue Sep 29, 2024 · 1 comment

Comments

@spencer-lunarg
Copy link
Contributor

There is a spirv-val error

All OpSampledImage instructions must be in the same block in which their Result are consumed

but now learning from @alan-baker in KhronosGroup/SPIRV-Guide#27 we should not be able to side-step with OpCopyObject

For GPU-AV in the Vulkan Validation Layers, I have times I need to go

%result = OpLoad %1 %2
  
 // <----- Inject an if check to make sure this access to something unrelated doesn't blow up when executed
%bad_accsss = OpAccessChain
%bad_load = OpLoad %bad_accsss

 
%5 = OpSampledImage %3 %4 %result 

and my "fix" was using OpCopyObject to pass %result so that spirv-val didn't complain 🤷

  1. I got from @dneto0 at the F2F this was not intended
  2. While here, any suggestions how something like GPU-AV should work around this case?
@alan-baker
Copy link
Contributor

I'd suggest we update the validation (and perhaps the spec) in ValidateSampledImage to traverse OpCopyObject. Just turn it into a DFS, but only push more instructions on OpCopyObject. It also appears that we aren't checking the similar rule for loads of images or samplers. So maybe the validator should refactor the code. The correct uses of OpSampledImage should ideally be checked on instruction operands already. So it might be the case the final check can removed.

As for GPU-AV, you could try either sinking or duplicating the load to just before the OpSampledImage. This is something inlining might be running into already. So maybe it would make sense to address it there and use a function in GPU-AV.

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

No branches or pull requests

2 participants