[SPIR-V] Empty groupshared arrays generate invalid SPIR-V

[RossNordby]

Using 0 as a constant value for a groupshared array causes a fatal error: generated SPIR-V is invalid: OpTypeArray Length <id> '11[%uint_0]' default value must be at least 1: found 0

Not exactly a big concern, but by request of the compiler output message, here's a reproduction:

RWByteAddressBuffer SomeBytes : register(u0);

groupshared int PoorlySizedArray[0];

[numthreads(1, 1, 1)]
void CSMain(uint3 threadId : SV_DispatchThreadID)
{ 
    SomeBytes.Store(threadId.x, PoorlySizedArray[threadId.x]);
}

Observed in 1.6.0.2994 and 1.6.0.3105 (1f6d1facb).

[jaebaek]

@RossNordby Thank you for reporting this issue. We will take a look.

[cassiebeckley]

@pow2clk is it valid HLSL for an array to have a size of zero? I found https://docs.microsoft.com/en-us/windows/win32/direct3dhlsl/dx-graphics-hlsl-variable-syntax#name_index_ which seems to say that the array size should be equal to zero, unless I'm misreading it. Is that a typo?

[pow2clk]

Hi Cassie!

It is valid, but it was introduced somewhat unceremoniously. Initially it was a regression from FXC, which forbade this, but ultimately, we chose to make it official since the restriction no longer made sense. See #2461.

I think you are referring to this part of the documentation?

Name[Index]

ASCII string that uniquely identifies a shader variable. To define an optional array, use index for the array size, which is a positive integer = 1.

I think perhaps you meant to say that the array size should be equal to one? That's true and I fear it's not so much a typo as outdated documentation, which is something we are working on. However setting it to one should still work for these purposes, but zeros are allowed now too.

[Keenuts]

Looked at it a bit today. Looking at https://learn.microsoft.com/en-us/windows/win32/direct3d12/resource-binding-in-hlsl, seems like this should be handled as an array of unknown size.

The exact example in this document could be handled the codegen today (need to declare it as an OpRuntimeArray and some fixes around, but almost works)

Texture2D<float4> tex1[0] : register(t0);

[numthreads(1, 1, 1)]
void main(uint3 threadId : SV_DispatchThreadID)
{}

Same for:

Buffer<float4> buff[0];

cbuffer buffer {
    int array[0]; // This one requires a fix in SPIRV-val.
}

But OpRuntimeArray has a few restrictions, one of them is the storage class must be either Uniform, StorageBuffer or UniformConstant. So IIRC, we wouldn't be able to translate this as it would require the Workgroup storage class.

Same for a local variable:

[numthreads(1, 1, 1)]
void main(uint3 threadId : SV_DispatchThreadID)
{
  int array[0];
}

So we might have to emit an error saying this specific case is not supported in SPIR-V.

[Keenuts]

Turns out, it seems groupshared int PoorlySizedArray[0]; should be illegal in HLSL. @tex3d

Could you confirm/deny those are legal in HLSL? My understanding was that if the array size is zero, then its considered as "an array which size is defined at runtime", but seems like it's a bit more than that:

A: illegal, local arrays cannot have a size=0

[numthreads(1, 1, 1)]
void main(uint3 threadId : SV_DispatchThreadID)
{
  int array[0];
}

B: legal, the array size is only known at runtime

cbuffer buffer {
    int array[0]; // This one requires a fix in SPIRV-val.
}

C: legal: the array size is only known at runtime

Texture2D<float4> tex1[0] : register(t0);

D: illegal?

groupshared int PoorlySizedArray[0];

[Keenuts]

@tex3d friendly ping

[Keenuts]

@tex3d friendly ping

[Keenuts]

@tex3d friendly ping

[Keenuts]

@tex3d friendly ping

[damyanp]

We're planning to clean up the language spec for this here: https://github.com/microsoft/hlsl-specs/issues/141

[tex3d]

Apologies for not addressing this before.

I don't think we should be supporting zero-length arrays at all. I think the way DXC supports this is an accident from failing to check and emit an error for this case.

We don't actually support runtime-sized arrays for groupshared memory or constant buffer arrays using a [0] array size, in spite of the fact that DXC will not emit an error for these cases today.

We support unbounded resource arrays, but the syntax DXC supports for that does not include any dimension value (like RWBuffer buffers[]).

Note that FXC supports [0] and maps it to an unbounded resource array, but DXC doesn't interpret this in the same way. For DXC, this results in a zero-length resource range instead (which seems like it should be invalid).

We could choose to emit an error when using [0] for resources for consistency with disallowing [0] sized arrays in all other cases, with the known issue that some code that compiled on FXC won't compile on DXC. This seems like the best option however, since that code doesn't work correctly on DXC today anyway. It's better to emit an error than quietly do something unintended.