Vulkan-ValidationLayers icon indicating copy to clipboard operation
Vulkan-ValidationLayers copied to clipboard

Check that `Dref` operations in shader match `compareEnable` on sampler

Open Rua opened this issue 3 years ago • 5 comments

In the spec section "Texel Input Validation Operations", two of the invalid/undefined cases are:

The SPIR-V instruction is one of the OpImage*Dref* instructions and the sampler compareEnable is VK_FALSE The SPIR-V instruction is not one of the OpImage*Dref* instructions and the sampler compareEnable is VK_TRUE

The validation layer doesn't currently check for this, and there is no VUID for these items, so maybe that is also something that needs correcting in the documentation. But since the spec does state that these situations lead to undefined behaviour, I feel like the layer should check for this in some way.

Rua avatar Dec 31 '21 10:12 Rua

@sfricke-samsung are these VUID-RuntimeSpirv-* candidates?

ncesario-lunarg avatar Jan 03 '22 18:01 ncesario-lunarg

But since the spec does state that these situations lead to undefined behaviour,

It says above in that section it says

In such cases the value of the texel returned is undefined.

So this means this is not "undefined behaviour" but rather that the value returned is undefined. So if there is an example you have hit that has caused side effects such as crashing, they I would purpose we create CTS test for it as my reading of the spec shows drivers/compilers should be able to handle these cases

I feel like the layer should check for this in some way

So I do agree that there are a lot of cases here that one would not catch without the help of a tool, but I would argue these might be better suited in the best practice layer then

sfricke-samsung avatar Jan 04 '22 06:01 sfricke-samsung

I would argue these might be better suited in the best practice layer then

Yes, if we do add anything, I would second that opinion.

ncesario-lunarg avatar Jan 04 '22 18:01 ncesario-lunarg

To come back to this, the DESCRIPTOR_REQ_VIEW_TYPE_* and DESCRIPTOR_REQ_*_SAMPLE constants in the source code are used to check for shader-bound resource matching. I noticed that the rules for these checks are listed in the "Texel Input Validation Operations" section. But these checks are currently included as validation checks, and refer to VUID-vkCmdDraw-None-02699, which simply states:

Descriptors in each bound descriptor set, specified via vkCmdBindDescriptorSets, must be valid if they are statically used by the VkPipeline bound to the pipeline bind point used by this command

No mention is made of what counts as "valid". It could mean that they must be valid resource object handles, or that they must also match the shader according to the validation operations section. For these two cases, the validation code takes the latter interpretation. But if not following these matching rules is not actually invalid usage, then these checks should probably also go in "best practices"?

Rua avatar Jan 20 '22 16:01 Rua

I didn't check all of the DESCRIPTOR_REQ_* logs, but it looks like at least some of them were added long ago to address "core" issues. That said, maybe it makes sense to use a different/unassigned VUID for logging those errors, but we'd have to look at those case by case. e.g., https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/1148 was added a long time ago and was originally logged as an unassigned VUID (kVUID_Core_DrawState_DescriptorSetNotUpdated), but eventually switched to 02699 (https://github.com/KhronosGroup/Vulkan-ValidationLayers/pull/2182).

ncesario-lunarg avatar Jan 21 '22 13:01 ncesario-lunarg