KhronosGroup / SPIRV-Cross

SPIRV-Cross is a practical tool and library for performing reflection on SPIR-V and disassembling SPIR-V back to high level languages.
Apache License 2.0
1.96k stars 549 forks source link

Invalid MSL when multiple SPIR-V variables are decorated with the same `BuiltIn` #2313

Closed AmesingFlank closed 1 month ago

AmesingFlank commented 2 months ago

Consider this SPIRV file, this contains multiple SPIRV variables marked with the InstanceId built-in decoration:

OpDecorate %gl_InstanceID BuiltIn InstanceId
OpDecorate %gl_InstanceID_0 BuiltIn InstanceId
OpDecorate %gl_InstanceID_1 BuiltIn InstanceId
...
%gl_InstanceID = OpVariable %_ptr_Input_int Input
%gl_InstanceID_0 = OpVariable %_ptr_Input_int Input
%gl_InstanceID_1 = OpVariable %_ptr_Input_int Input

When this shader is passed to spirv cross, an invalid MSL shader is generated:

...
vertex main0_out main0(main0_in in [[stage_in]], device CameraBlockPointers& cameraBlockPointers [[buffer(0)]], device ModelBlockPointers& modelBlockPointers [[buffer(1)]], uint gl_InstanceID [[instance_id]], uint gl_InstanceID [[instance_id]], uint gl_InstanceID [[instance_id]])
{
 ...
}

notice the redefinition of the gl_InstanceID variable

HansKristian-Work commented 2 months ago

This is illegal SPIR-V I believe. In Vulkan, InstanceId cannot be used, need to use InstanceIndex. The module is not valid OpenGL SPIR-V either, so I doubt that was the intent.

Also, I think duplicate decoration declarations like that are illegal. For user-defined variables for example, declaring two variables with same location is banned:

Any two inputs listed as operands on the same OpEntryPoint must not be assigned the same Location slot and Component word, either explicitly or implicitly. Any two outputs listed as operands on the same OpEntryPoint must not be assigned the same Location slot and Component word, either explicitly or implicitly.

spirv-val does not seem to validate this at the moment. I've suggested a spec clarification to add similar rules for builtins. I don't believe that was intended to work.

HansKristian-Work commented 1 month ago

This was clarified in 1.3.285 spec update to be banned.