[SPIR-V] Reapeated SV_DomainLocation semantics don't produce an error and can crash the compiler (works with the DXIL backend)

[Dredhog]

Title

[SPIR-V] Reapeated SV_DomainLocation semantics don't produce an error and can crash the compiler (works with the DXIL backend)

Functional impact

Declaring repeated SV_DomainLocation input semantics in the same Domain shader entry point does not produce a compiler error and crashes in some cases.

Minimal repro steps

1. Compile the following shader with dxc.exe -T ds_6_0 -E Domain -spirv repro_shader.txt
   repro_shader.txt

Expected result

The SPIR-V backend should produce error similar to that of the DXIL backend:

float4 Domain (const OutputPatch<HullOut,3> vi,
^
repro_shader.txt:4:1: error: Parameter with semantic SV_DomainLocation has overlapping semantic index at 0

Actual result

The SPIR-V backend crashes with an access violation exception:
Internal compiler error: access violation. Attempted to read from address 0x0000000000000010

Further technical details

The repro_shader.txt in the Minimal repro steps section crashes the compiler with 3 repeats, if repeating only twice it still compiles, which is still an issue as it should instead produce a compiler error (like the DXIL backend does)
Used DXC release v1.6.2104 (e09a454)

[Keenuts]

Seems like this reproduction code hits 2 issues:

• the semantic duplication
• the float broadcast to a vector which fails.

For the first issue, we don't crash today, but we allow it while DXIL doesn't.
The reason is: When we find 2 input variables referencing the same semantic, we consider the second as an alias, and will merge them.

Seems like this aliasing is the cause of the second issue: some codegen relies on the different variable type, but when we alias, we might end up changing the type of bary2, and cause the type issue on broadcast.

Seems like we should not allow aliasing (to match DXIL behavior). I'll see the cleanest way to handle that.

[Keenuts]

Looking a bit at this, I want do add the error.
But I'm reluctant as we did allowed aliasing input variables in the past to be compatible with FXC.

@tex3d: is it allowed or not in HLSL to use the same semantic for an input parameter twice to alias it?
If yes, is it allowed if the type of the input changes?
The HLSL spec doesn't seem to restrict it, but I do see errors listed https://learn.microsoft.com/en-us/windows/win32/direct3dhlsl/hlsl-errors-and-warnings which seems to go in the DXIL direction.

[Keenuts]

Or @llvm-beanz maybe?

• Shall input parameter aliasing be allowed?
• If yes, can its type change, as long as it's a flat-conversion like cast? (float3 : A and float2 : A)?

My opinion is no, we should not allow input alias.

[llvm-beanz]

I think the rules here as implemented in DXC are complicated and inconsistent. For example both the case in this bug and your example with user-defined semantics are invalid (see: Hull Shader & Vertex Shader).

We currently only enforce this in the DXIL validator rather than in Sema, which is why the error handling between DXIL and SPIR-V are hard to keep consistent. @bob80905 worked up a feature proposal for moving this into Sema (see: microsoft/hlsl-specs#156), but we decided that wasn't justified for DXC, but we would pursue this approach in Clang.

We do also seem to allow duplicate compute shader system values (see: SV_GroupIndex & SV_GroupID). I can't think of a legitimate use case for this, but changing it would likely need to be a language change because we'd almost certainly break users in some cases.

[Keenuts]

Right, btw, the validator doesn't seem to catch SV_Position & POSITION alias:
https://godbolt.org/z/aYn73joGz

Assuming this line of the documentation is right:

Note to Direct3D 9 developers: For Direct3D 9 targets, shader semantics must map to valid Direct3D 9 semantics. For backwards compatibility POSITION0 (and its variant names) is treated as SV_Position, COLOR is treated as SV_TARGET.

We do also seem to allow duplicate compute shader system values (see: SV_GroupIndex & SV_GroupID).

Seems like this is just an overlook: the shader has no input, those are grabbed from DXIL builtins, hence probably why this wasn't caught.

At least for the SPIR-V side, I'm in favor of disallow any aliasing. As long as we can say that's desirable from the HLSL language point of view.

[llvm-beanz]

Right, btw, the validator doesn't seem to catch SV_Position & POSITION alias: https://godbolt.org/z/aYn73joGz

Oh! This is actually kinda fascinating. It looks like if you use SV_Position, then POSITION gets treated as a user-defined semantic. That's probably also a bug.

Seems like this is just an overlook: the shader has no input, those are grabbed from DXIL builtins, hence probably why this wasn't caught.

Yep, and it is a product of relying on validation for these errors.

At least for the SPIR-V side, I'm in favor of disallow any aliasing. As long as we can say that's desirable from the HLSL language point of view.

I think that's fair, and seems consistent with the intent.

[Keenuts]

Right, btw, the validator doesn't seem to catch SV_Position & POSITION alias: https://godbolt.org/z/aYn73joGz

Oh! This is actually kinda fascinating. It looks like if you use SV_Position, then POSITION gets treated as a user-defined semantic. That's probably also a bug.

Seems like this is just an overlook: the shader has no input, those are grabbed from DXIL builtins, hence probably why this wasn't caught.

Yep, and it is a product of relying on validation for these errors.

At least for the SPIR-V side, I'm in favor of disallow any aliasing. As long as we can say that's desirable from the HLSL language point of view.

I think that's fair, and seems consistent with the intent.

@tex3d @pow2clk the other case for SV_Position