KhronosGroup / SPIRV-Tools

Apache License 2.0
1.03k stars 541 forks source link

how best to add validation for Kernel SPIR-V #5597

Open bashbaug opened 4 months ago

bashbaug commented 4 months ago

I'd like to start adding some basic validation checks that are missing for Kernel SPIR-V and I am looking for guidance how best to do this. If it is helpful to provide a specific example, I would like to check that all built-in variables are in the Input storage class and that they all have the expected type, see: https://registry.khronos.org/OpenCL/specs/3.0-unified/html/OpenCL_Env.html#_built_in_variables

One of the first issues I'm finding is that a lot of this type of validation is only enabled for Vulkan environments. For example:

https://github.com/KhronosGroup/SPIRV-Tools/blob/9bd44d028e8ca257dde9c24d5f67adabee3275ab/source/val/validate_builtins.cpp#L4280-L4293

Would it be best to keep the current BuiltIn-specific rules as-is, call them only for Vulkan, and define other rules for Kernel SPIR-V? Or, would it be best to call the same BuiltIn-specific rules for both Vulkan and Kernel SPIR-V and to update them to handle both types of SPIR-V?

Thanks in advance for any advice you can provide!

alan-baker commented 4 months ago

Reusing the infrastructure is preferable for both builtins and general validation. I could see adding a generic environment based filter before validating any particular builtin.

For general validation the checks should be based on the appropriate requirement (e.g. either HasCapability(Kernel) or spvEnvIsOpenCL(env)).