microsoft / DirectXShaderCompiler

This repo hosts the source for the DirectX Shader Compiler which is based on LLVM/Clang.
Other
3.12k stars 699 forks source link

Dynamic resources validation errors: All metadata must be used by dxil.!55 = !{i32 1} #4888

Open kruseborn opened 1 year ago

kruseborn commented 1 year ago

Compiling this code with -T ps_6_6 -E PSMain outputs: error: All metadata must be used by dxil.!55 = !{i32 1}

Is this code invalid or is it a compiler bug in DXC?

https://godbolt.org/z/4fMjoW4Gv

struct PSInput
{
    float4 position : SV_Position;
    float4 color    : COLOR0;
};

cbuffer Ids {
    int id1;
    int id2;
};

static const Texture2D<float4> textures[2] = {
    Texture2D<float4>(ResourceDescriptorHeap[id1]),
    Texture2D<float4>(ResourceDescriptorHeap[id2])
};

SamplerState ss: register(s0);

float4 PSMain(PSInput input) : SV_Target0
{
    float4 sample = textures[NonUniformResourceIndex(int(input.color.x))].Sample(ss, float2(0, 1));
    return sample;
}
mathforlife83 commented 1 year ago

I am also hitting the same error message, it does compile without the NonUniformResourceIndex function, but then AMD crashes when trying to create a pipeline, in amdxc64.dll. Probably because of badly generated DXIL?

Keenuts commented 1 year ago

Hi!

@llvm-beanz : TL;DR: seems legal, also crashes with SPIR-V (assert). Can you confirm it's legal and we should fix it?

Thanks for the report! Far from an HLSL expert, but seems like this should be legal:

So it seems like we could have a static const array storing resources looked up like that. Playing a bit with the code, I can compile this, reproducing the code:

cbuffer Ids {
    int id1;
    int id2;
};

static const Texture2D<float4> textures[2] = {
    Texture2D<float4>(ResourceDescriptorHeap[id1]),
    Texture2D<float4>(ResourceDescriptorHeap[id2])
};

RWStructuredBuffer<float4> output;
SamplerState ss: register(s0);

[numthreads(64, 1, 1)]
void main(uint id : SV_DispatchThreadID) {
  output[0] = textures[NonUniformResourceIndex(id)].Sample(ss, float2(0, 1));
}

And this, generating DXIL:

cbuffer Ids {
    int id1;
    int id2;
};

RWStructuredBuffer<float4> output;
SamplerState ss: register(s0);

[numthreads(64, 1, 1)]
void main(uint id : SV_DispatchThreadID) {
  Texture2D<float4> tex = ResourceDescriptorHeap[(int)NonUniformResourceIndex(id == 0 ? id1 : id2)];
  output[0] =  tex.Sample(ss, float2(0, 1));
}

(command: ./build/bin/dxc ./repro.hlsl -T cs_6_6 -E main -Od -Vd)

Adding the -spirv command yields a crash

DirectXShaderCompiler/include/llvm/Support/Casting.h:96: static bool llvm::isa_impl_cl<clang::Expr, const clang::Stmt *>::doit(const From *) [To = clang::Expr, From = const clang::Stmt *]: Assertion `Val && "isa<> used on a null pointer"' failed.
Aborted
kruseborn commented 1 year ago

@Keenuts Any chance of getting a fix for this one, it would help us delete a lot of code :)

llvm-beanz commented 1 year ago

@tex3d, Can you confirm that the HLSL is valid?

tex3d commented 1 year ago

First, a side note: the spec language around descriptor and data volatility don't really impact whether this scenario's HLSL code is legal.

The HLSL code here could be made legal by the compiler, but a couple issues hold it back from being able to do that today. In general, to make this legal, the compiler must transform your code into a form that looks like this (from the HLSL perspective):

static const uint textureIndexes[2] = {id1, id2};

// and later in the function:
    Texture2D<float4> tex = ResourceDescriptorHeap[
            NonUniformResourceIndex(
                textureIndexes[int(input.color.x)]
            )
        ];
    float4 sample = tex.Sample(ss, float2(0, 1));

There are two patterns in the original code that need to be identified and generate diagnostics for now, until we can successfully handle each of them.

  1. NonUniformResourceIndex can only be used on the immediate index of a global (bound) resource array, or the immediate index of the ResourceDescriptorHeap or SampleDescriptorHeap built-in arrays. Currently, if this intrinsic is used elsewhere, such as another array index, or on a value assigned to an intermediate variable that is then used to index the resource array, it will be silently lost.
  2. Temporary arrays of resource objects (doesn't matter whether static const, or local) can only work by transforming them into arrays of indices into a single global (bound) resource array or one of the *DescriptorHeap arrays. This work isn't done yet for the *DescriptorHeap arrays.

This issue can be used to track the need to add diagnostics for these cases that direct the user to write code the compiler can handle for now.