Raytracing shader tests use gl_InstanceID but that's invalid in Vulkan

[dneto0]

The validator recently started checking the rule that InstanceID is not allowed in Vulkan shaders. (Thank you @sheredom )

But Tests/ spv.AnyHitShader.rahit, spv.ClosestHitShader.rchit, spv.IntersectShader.rint use gl_InstanceID. That is translated to an access of BuiltIn InstanceID, which is not permitted in Vulkan.

The gl_InstanceIndex is not defined for those shader types. It's unclear to me what is intended here. The SPV_NV_ray_tracing extension does not specifically enable InstanceID, so it appears that the tests are invalid, and glslang is incorrectly accepting gl_InstanceID for such shaders.

Cc: @chaoc @dgkoch

(Edited to fix VertexID -> InstanceID)

[dneto0]

This is causing failures in Shaderc bots because they always pull top-of-tree SPIRV-Tools

[dgkoch]

We're discussing what to do here. Our implementation is expecting InstanceId, but the SPV and VK specs say InstanceIndex.

[dneto0]

@dgkoch Thanks for the update. I'd be ok with whatever resolution makes sense for you. I just need the specs and tools to line up somehow. If it isn't a speedy resolution please consider blessing #1578 to temporarily disable the offending portions of the test.

[dgkoch]

@dneto0 unfortunately @alelenv is OOTO this week and we'd like to confer with him, but our inclination is to update the SPV and Vulkan specs to match the implementation (meaning that InstanceId instead of InstanceIndex is allowed in intersection, anyhit, and closest hit shaders).

Can we at least temporarily whitelist that in spirv-tools while we figure that out?

[johnkslang]

It is okay to track failures in the regression results. See https://github.com/KhronosGroup/glslang/pull/1578#issuecomment-438441961.

[dneto0]

@dgkoch

Can we at least temporarily whitelist that in spirv-tools while we figure that out?

Yeah. Please file an issue against SPIRV-Tools to disable the check for the 3 raytracing shader types. We can do that before spec changes land.

[dgkoch]

@dneto0 I filed https://github.com/KhronosGroup/SPIRV-Tools/issues/2046 for this.

[dgkoch]

We've confirmed that we intended to use InstanceId for the ray tracing shaders and have updated the SPIRV and GLSL specs to reflect that:

• https://github.com/KhronosGroup/SPIRV-Registry/pull/15
• https://github.com/KhronosGroup/GLSL/pull/41

Still need to fix the json grammar to reflect this.

[dgkoch]

Actually there don't seem to be any json grammar changes required - both InstanceId and InstanceIndex already depend on the Shader capability, and so there doesn't seem to be any need to add RayTracingNV as well. @johnkslang can you confirm that is how this is supposed to work?

[johnkslang]

@dgkoch, agreed. If we had this form of ray tracing outside the Shader capability, you'd want to list both. But, because RayTracingNV implicitly declares Shader, it's covered.

[dgkoch]

Ok - I don't there is anything remaining for this issue then.

There is https://github.com/KhronosGroup/SPIRV-Tools/issues/2051 to improve the validation, but it can be tracked separately.

I apparently don't have permissions to close the issue.

[johnkslang]

Closing as nothing to change for glslang.