MSL: Also handle signedness mismatches in fragment outputs.

[cdavis5e]

It turns out that the number of components is not the only place where Vulkan allows differences but Metal insists on strict equivalence. Vulkan also allows the fragment shader and the attachment to differ in signedness--for example, rendering to an R8G8B8A8_UINT image where the fragment shader declares its output to be an ivec4.

To fix this, we must adjust the [[color]] output in the interface block to match the render target. Use the shader output specification mechanism to do this; also move the fragment component count here to save some memory at run time.

[HansKristian-Work]

R8G8B8A8_UINT image where the fragment shader declares its output to be an ivec4

I don't think that is legal ... Can you point to spec language?

[cdavis5e]

R8G8B8A8_UINT image where the fragment shader declares its output to be an ivec4

I don't think that is legal ... Can you point to spec language?

The Fragment Output interface section says:

Output values written by a fragment shader must be declared with either OpTypeFloat or OpTypeInt, and a Width of 32. If storageInputOutput16 is supported, output values written by a fragment shader can be also declared with either OpTypeFloat or OpTypeInt and a Width of 16. Composites of these types are also permitted. [...] If the color attachment has an integer format, color values are assumed to be integers and converted to the bit-depth of the target. Any value that cannot be represented in the attachment’s format is undefined. For any other attachment format no conversion is performed. If the type of the values written by the fragment shader do not match the format of the corresponding color attachment, the resulting values are undefined for those components. [emphasis added]

That would indicate that you are correct. However, the CTS actually tests the case where the fragment shader's output and the color attachment's format are mismatched in sign, which suggests that this must be allowed somewhere in the spec... (see dEQP-VK.api.fragment_shader_output.different_signedness.*)

[cdavis5e]

$#@!ing MSVC...

[HansKristian-Work]

Does that CTS test what the output value is? The spec language says undefined value, which means it's valid to just write 0 to those in theory (but using mismatched types isn't technically illegal). CTS sometimes have bugs, and it's possible this is one of those cases.

[cdavis5e]

Does that CTS test what the output value is?

As a matter of fact, it does.

[HansKristian-Work]

File a CTS bug then. It's not a valid test with the current spec language.

[HansKristian-Work]

Based on this finding, I'm not sure where to go with this PR. If we're adding extra complexity and maintenance burden to support a case that only a bogus CTS test cares about, I'm a bit reluctant, but that's doesn't mean I'm blocking it outright. Awaiting a reply before this PR can progress.

[cdavis5e]

File a CTS bug then. It's not a valid test with the current spec language.

KhronosGroup/VK-GL-CTS#548

[aitor-lunarg]

So while the spec says it's undefined behavior, https://gerrit.khronos.org/c/vk-gl-cts/+/12319 forced all drivers to support this... My main concern is that because now all drivers have that undefined behavior defined, the workgroup could decide to actually make it defined I guess.

I've created an internal CTS issue for the public one to address this: https://gitlab.khronos.org/Tracker/vk-gl-cts/-/issues/6131

While KosmicKrisp works around this, it would be better if we didn't have to.