Allow Offset in addition to ConstOffset for texture operations

[rg3igalia]

As part of an upcoming Vulkan maintenance extension I'm developing CTS tests for, we want to allow using Offset in addition to ConstOffset for image operations. This was already possible for image gather operations, but not for others, and is mainly enforced by VUID-StandaloneSpirv-Offset-04663. There are other VUIDs affected by this but that's the main one. On the SPIR-V side there's no such restriction, so SPIR-V doesn't need any special capability nor a new extension to support this behavior.

This issue is related to the following glslang issue: KhronosGroup/glslang#3765

CTS uses the validator from SPIRV-Tools by default in Debug mode, and the new tests were failing validation due to VUID-StandaloneSpirv-Offset-04663. I got the validator to stop emitting this validation error by commenting out a code block like this:

diff --git a/source/val/validate_image.cpp b/source/val/validate_image.cpp
index 04100dd7..8968e31b 100644
--- a/source/val/validate_image.cpp
+++ b/source/val/validate_image.cpp
@@ -454,6 +454,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _,
              << " components, but given " << offset_size;
     }
 
+#if 0
     if (!_.options()->before_hlsl_legalization &&
         spvIsVulkanEnv(_.context()->target_env)) {
       if (opcode != spv::Op::OpImageGather &&
@@ -466,6 +467,7 @@ spv_result_t ValidateImageOperands(ValidationState_t& _,
                   "OpImage*Gather operations";
       }
     }
+#endif
   }
 
   if (mask & uint32_t(spv::ImageOperandsMask::ConstOffsets)) {

In that code block, I see that if I enable the before_hlsl_legalization option I could also skip that check. CTS allows us to pass shader build options when creating shader programs. Currently there's no code to wire anything to a call to spvValidatorOptionsSetBeforeHlslLegalization, but it's easy to do so (we already have other stuff wired to other validator options). My doubt is if that's the right solution: if I enable such an option when compiling shaders for the new tests, am I also allowing other "undesirable" things in the validator? If, on Vulkan implementations that have the new maintenance extension feature enabled, this is going to be legal, should we simply remove the check from the validator for all shaders? What's the right approach here?

/cc @spencer-lunarg since the the layers also use the SPIR-V validator (right?) and this could affect a validation layers release based on the spec that will include the maintenance extension.

[s-perron]

Alan will have the final word, but the right fix should be to change the vulkan environment check to reflect the change in the spec. I don't know how the spec is changing, so I can't suggest the actual change.

[rg3igalia]

I'll CC you and Alan in the internal Khronos spec MR so you have more context.

[spencer-lunarg]

So looked into this now, we are simply moving what use to be a Vulkan env standalone validation rule to a runtime validation rule around a new feature bit.

I would personally purpose to just move this check (VUID 04663) from a spirv-val and move the check to Validation Layers (since it is already in a spvIsVulkanEnv(_.context()->target_env) wrapper anyway)

We could even do the transition now (can make the PR quickly if we agree for both repos), and when the maintenance extension is release, we update the Validation Layers to check against the new feature bit

[dneto0]

I would personally purpose to just move this check (VUID 04663) from a spirv-val and move the check to Validation Layers (since it is already in a spvIsVulkanEnv(_.context()->target_env) wrapper anyway)

The maintenance extension is a change to the validation rules for Vulkan.
I would model it as a feature bit in ValidationState_t::Feature See

SPIRV-Tools/source/val/validation_state.h

Line 67 in 0243356

struct Feature {

Normally those would be turned on by a change in the environment; but there's no new Vulkan version so instead turn it on via a new method spvValidatorOptionsSet.... (see the example like spvValidatorOptionsSetRelaxBlockLayout)

cc: @alan-baker

[spencer-lunarg]

Just as some info, in the validation layers https://github.com/KhronosGroup/Vulkan-ValidationLayers/blob/main/layers/utils/shader_utils.cpp#L131 we take the runtime Vulkan features and pass in make calls like SetRelaxBlockLayout, so we are very capable to adding this flow

My concern is most people I assume just want to run spirv-val --target-env ... and having to keep track of an increasing number of random flags that just allow/disallow a check seems like something that is better served in a runtime check than a command line tool