assignment to undeclared variable in generated metal code

[kevinmgh]

We're seeing an issue with hlsl -> dxc -> spirv -> metal where the resulting metal includes assignment to an undeclared variable. This is using the latest SPIRV Cross (https://github.com/KhronosGroup/SPIRV-Cross/releases/tag/MoltenVK-1.1.5). A simple example:

Texture2D texture1; 
Texture2D texture2; 
Texture2D texture3; 

Texture2D GetIndexedTexture ( uint index ) { if ( index == 1 ) { return texture1 ; } else if ( index == 2 ) { return texture2 ; } else { return texture3 ; } }

results in

        do
        {
            if (_42 == 1u)
            {
                _0 = texture1;
                break;
            }
            else
            {
                if (_42 == 2u)
                {
                    _0 = texture2;
                    break;
                }
                else
                {
                    _0 = texture3;
                    break;
                }
            }
        } while(false);

(where _0 was never declared).

example.hlsl contains this as well as a few commented alternate implementations of "GetIndexedTexture" all of which have this issue. exampleInput.hlsl.txt exampleOutput.metal.txt

[kevinmgh]

As an aside, I also tried rolling back to an earlier version of SPIRV cross which yielded a result that compiled but still doesn't look correct. See how the result only exists in the current scope (note: this is from a more complicated input example not provided, but this snippet was from essentially the same thing).

do
{
    if (_3987 == 1u)
    {
        texture2d_array<float> _32396;
        _32396 = texture1;
        break;
    }
    else
    {
        if (_3987 == 2u)
        {
            texture2d_array<float> _32397;
            _32397 = texture2;
            break;
        }
        else
        {
            texture2d_array<float> _32398;
            _32398 = texture3;
            break;
        }
        break; // unreachable workaround
    }
    break; // unreachable workaround
} while(false);

[kevinmgh]

The attached exampleinput.hlsl.txt is the full source code that would yield exampleOutput.metal.txt.

However, I apologize -- there was a more recent version of SPIRV Cross than our last pull. I reproduced the above with https://github.com/KhronosGroup/SPIRV-Cross/commit/0e2880ab990e79ce6cc8c79c219feda42d98b1e8

But it seems to have already been fixed with https://github.com/KhronosGroup/SPIRV-Cross/commit/5f7a6de5521b2ce56769f7fb7aa675c143ab8018 edit: we still have issues (slightly different, see below) with the latest version

[djwingrove]

In further testing, the new version creates msl that compiles but it is not valid code.

            do

            {

                if (_10858 == 1u)

                {

                    texture2d_array<float> _28020;

                    _28020 = texture1;

                    break;

                }

                else

                {

                    if (_10858 == 2u)

                    {

                        texture2d_array<float> _28021;

                        _28021 = texture2;

                        break;

                    }

                    else

                    {

                        texture2d_array<float> _28022;

                        _28022 = texture3;

                        break;

                    }

                    break; // unreachable workaround

                }

                break; // unreachable workaround

            } while(false

[kevinmgh]

Yeah, confirmed that we still have this issue -- the output metal code compiles when generated with 5f7a6de5521b2ce56769f7fb7aa675c143ab8018, but it does not do what the HLSL does, see @djwingrove's example. Attached is the full metal output from the same full HLSL input I attached earlier.

exampleOutput_5f7a6de5521b2ce56769f7fb7aa675c143ab8018.metal.txt

[kevinmgh]

slight addendum: the commented implementation in the provided example hlsl input that uses an array lookup does result in what looks like correctly-generated MSL code with https://github.com/KhronosGroup/SPIRV-Cross/commit/5f7a6de5521b2ce56769f7fb7aa675c143ab8018 (it was incorrect with https://github.com/KhronosGroup/SPIRV-Cross/commit/0e2880ab990e79ce6cc8c79c219feda42d98b1e8).

The other examples all still yield the compiling-but-incorrect output with https://github.com/KhronosGroup/SPIRV-Cross/commit/5f7a6de5521b2ce56769f7fb7aa675c143ab8018.

[HansKristian-Work]

Do note that texture objects cannot be used as lvalues like this.

[HansKristian-Work]

For example:

Texture2D texture1; 
Texture2D texture2; 
Texture2D texture3;
SamplerState S;

Texture2D GetIndexedTexture ( uint index ) { if ( index == 1 ) { return texture1 ; } else if ( index == 2 ) { return texture2 ; } else { return texture3 ; } }

float4 main(uint i : I, float2 uv : UV) : SV_Target
{
    return GetIndexedTexture(i).Sample(S, uv);
}

when compiled to DXIL:

/tmp/test.hlsl:6:51: error: local resource not guaranteed to map to unique global resource.
Texture2D GetIndexedTexture ( uint index ) { if ( index == 1 ) { return texture1 ; } else if ( index == 2 ) { return texture2 ; } else { return texture3 ; } }
                                                  ^
/tmp/test.hlsl:6:96: error: local resource not guaranteed to map to unique global resource.
Texture2D GetIndexedTexture ( uint index ) { if ( index == 1 ) { return texture1 ; } else if ( index == 2 ) { return texture2 ; } else { return texture3 ; } }
                                                                                               ^

[HansKristian-Work]

DXC also complains when compiling to SPIR-V:

fatal error: generated SPIR-V is invalid: Result type cannot be OpTypeImage
  %43 = OpPhi %type_2d_image %40 %39 %41 %38 %42 %34

note: please file a bug report on https://github.com/Microsoft/DirectXShaderCompiler/issues with source code if possible

[HansKristian-Work]

For the attached HLSL shader, this requires legalization to work, and it doesn't seem like DXC does the necessary legalization to make this valid SPIR-V. Using Phi with textures is simply not legal.

I suggest using an array of 3 textures instead, and selecting the index instead. That would be fine and just as efficient.