[SPIR-V] Bindless Result type cannot be OpTypeImage

[manas-kulkarni]

We plan to use a specialization constant to figure out whether to use bindless textures or not based on the GPU limits. Currently facing an issue when we try to do this with a SpirV error

fatal error: generated SPIR-V is invalid: Result type cannot be OpTypeImage
  %83 = OpPhi %type_2d_image %76 %75 %79 %77

HLSL

Texture2D<float4> bindless[70000];
RWTexture2D<float4> rw;

[[vk::constant_id( 0 )]] const bool gBindlessTextureSupport = true;

#define DECL_BINDLESS_TEXTURE_RESOURCE(res_type, res_name, res_index) \
    res_type res_name;                                                \         
    res_type GetBindlessTex_##res_name()                              \
    {                                                                 \
        if ( gBindlessTextureSupport )                                \
        {                                                             \
            return bindless[res_index];                               \
        }                                                             \
        else                                                          \
        {                                                             \
            return res_name;                                          \
        }                                                             \
    }

DECL_BINDLESS_TEXTURE_RESOURCE(Texture2D<float4>, clutOutput, 0)
#define GET_BINDLESS_TEXTURE(res_name) GetBindlessTex_##res_name()

[numthreads(8, 8, 1)]
void main( uint3 dtid : SV_DispatchThreadID )
{
    rw[dtid.xy] = GET_BINDLESS_TEXTURE(clutOutput)[dtid.xy];    
}

We verified that the GLSL version works fine with glslangValidator so must be a bug in dxc. Tried with --legalize-hlsl but same error

[manas-kulkarni]

GLSL version

#version 450

#extension GL_EXT_samplerless_texture_functions : enable

layout(local_size_x = 8, local_size_y = 8, local_size_z = 1) in;

layout (constant_id = 0) const bool gBindlessTextureSupport = true;

layout(set = 1, binding = 0, rgba8) uniform writeonly image2D rw;
layout(set = 0, binding = 0) uniform texture2D bindless[70000];

#define DECL_BINDLESS_TEXTURE_RESOURCE(res_type, res_name, res_index) \
    layout(set = 0, binding = res_index) uniform res_type res_name;   \
    res_type GetBindlessTex_##res_name()                              \
    {                                                                 \
        if ( gBindlessTextureSupport )                                \
        {                                                             \
            return bindless[res_index];                               \
        }                                                             \
        else                                                          \
        {                                                             \
            return res_name;                                          \
        }                                                             \
    }

DECL_BINDLESS_TEXTURE_RESOURCE(texture2D, clutOutput, 1)
#define GET_BINDLESS_TEXTURE(res_name) GetBindlessTex_##res_name()

void main()
{
    imageStore(rw, ivec2(gl_GlobalInvocationID.xy), texelFetch(GET_BINDLESS_TEXTURE(clutOutput), ivec2(gl_GlobalInvocationID.xy), 0));
}

[kuhar]

Thanks for reporting, @manas-kulkarni. @jaebaek or @sudonatalie, could you take a look?

[manas-kulkarni]

Any update on this? Kind of a pressing issue for us so any update would be great

[ByumjinConffx]

Hi @kuhar, @jaebaek, @sudonatalie. I have a same issue.

[alan-baker]

The GLSL version passes due to some missing validation. The return type of a function cannot be an opaque type, but this is not currently validated.

The HLSL legalization has problems with this case. Right now it's basically hitting inlining and ssa conversion, which produces the invalid phi instruction. It would have to instead sink the textureFetch into both sides of the if statement.

As a workaround, you could change the spec id to just a static const value, but then you'd have to generate both SPIR-V modules and select the right one at pipeline creation time. That's not great, but it might get you going for now.

[manas-kulkarni]

Thanks for the update.

If we disable this validation on DXC side, will the generated SpirV work correctly?

Can we expect a fix in HLSL legalization or is this a non-fixable issue? We will start looking at other ways of dealing with this if it's not fixable.

We are trying to use spec constant to avoid generating two shader binaries.

[alan-baker]

I think whether or not the invalid SPIR-V works would be driver-dependent. You could test by disabling validation and/or optimizations in DXC, but there's no guarantee it would be widely supported or continue to work.

Another potential workaround you could try refactoring the macros so that you have one that declares just the resources and others that define the texelFetch:

#version 450

#extension GL_EXT_samplerless_texture_functions : enable

layout(local_size_x = 8, local_size_y = 8, local_size_z = 1) in;

layout (constant_id = 0) const bool gBindlessTextureSupport = true;

layout(set = 1, binding = 0, rgba8) uniform writeonly image2D rw;
layout(set = 0, binding = 0) uniform texture2D bindless[70000];

#define DECL_BINDLESS_TEXTURE_RESOURCE(res_type, res_name, res_index) \
    layout(set = 0, binding = res_index) uniform res_type res_name;

#define FETCH_RESOURCE(res_name, res_index) \
    vec4 GetBindlessTex_##res_name(ivec2 coords, int lod)                              \
    {                                                                 \
        if ( gBindlessTextureSupport )                                \
        {                                                             \
            vec4 x = texelFetch(bindless[res_index], coords, lod); \
            return x; \
        }                                                             \
        else                                                          \
        {                                                             \
            vec4 x = texelFetch(res_name, coords, lod); \
            return x; \
        }                                                             \
    }

DECL_BINDLESS_TEXTURE_RESOURCE(texture2D, clutOutput, 1)
FETCH_RESOURCE(clutOutput, 1)
#define GET_BINDLESS_TEXTURE(res_name, coords, lod) GetBindlessTex_##res_name(coords, lod)

void main()
{
    ivec2 coords = ivec2(gl_GlobalInvocationID.xy);
    int lod = 0;
    vec4 x = GET_BINDLESS_TEXTURE(clutOutput, coords, lod);
    imageStore(rw, ivec2(gl_GlobalInvocationID.xy), x);
}

This should work in both GLSL and HLSL.

[manas-kulkarni]

Thanks! We will try with validation disabled.

[manas-kulkarni]

We tried the SpirV on a S21 Mali-G78 and it throws the same validation error when creating the shader and crashes when creating the pipeline. So legalize hlsl option would be necessary for this case that sinks the texture op on both sides of the if statement

[alan-baker]

Have you tried the modified code I suggested above? Even if the improved optimizations are implemented it will take some time before they are available.

[s-perron]

As @alan-baker pointed out, the problem is that the optimizer cannot fold the branch that selects the resource because of the spec constant. A good work around is what he suggested.

We could do something in the optimizer. We have code in in this pass that can replicate code and move it into a switch or in this case an if-statement. We could probably refactor that code to make this case work as well. I don't know what the timeline will be for getting this done.

[manas-kulkarni]

@alan-baker The problem with that code is it doesn't fit all our use cases. Only works when you are doing GET_BINDLESS(x).Sample. But won't work when you are using return from GET_BINDLESS(x) and then passing that Texture as an argument to another function. Thanks for all the help!

@s-perron Would be great if this gets fixed. Otherwise, we have to rethink or keep two separate shader binaries as we currently do

[s-perron]

We will not be fixing this for reasons outlined above.