KhronosGroup / SPIRV-Tools

Apache License 2.0
1.08k stars 555 forks source link

OpCopyObject lets you sidestep OpSampledImage validation #5830

Open spencer-lunarg opened 3 weeks ago

spencer-lunarg commented 3 weeks ago

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 https://github.com/KhronosGroup/SPIRV-Guide/issues/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 :shrug:

  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 commented 3 weeks ago

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.