gfx-rs / gfx

[maintenance mode] A low-overhead Vulkan-like GPU API for Rust.
http://gfx-rs.github.io/
Apache License 2.0
5.35k stars 547 forks source link

Shader fails to compile with D3D12 backend #2559

Open CryZe opened 5 years ago

CryZe commented 5 years ago

Short info header:

The following shader works just fine with the Vulkan backend, but fails to compile with the D3D12 backend. The compilation to SPIR-V works, but the actual pipeline creation fails.

#version 450

layout(push_constant) uniform Data {
    float scale;
    vec4 color_tl;
    vec4 color_tr;
    vec4 color_bl;
    vec4 color_br;
} data;

layout(location = 0) in vec2 position;
layout(location = 1) in vec2 texcoord;

layout(location = 0) out vec4 color;
layout(location = 1) out vec2 outTexcoord;

void main() {
    vec4 left = mix(data.color_tl, data.color_bl, texcoord.y);
    vec4 right = mix(data.color_tr, data.color_br, texcoord.y);
    color = mix(left, right, texcoord.x);

    gl_Position = vec4(data.scale * position, 0.0, 1.0);
    outTexcoord = texcoord;
}

Error: error X3004: undeclared identifier 'data_color_bl'

kvark commented 5 years ago

Sounds like a spirv-cross failure, which we need to either update as a dependency, or report upstream.

CryZe commented 5 years ago

I manually compiled and ran the latest spirv-cross and it seems to compile it correctly (unless I'm missing something in the HLSL, I didn't test it):

cbuffer Data
{
    float data_scale : packoffset(c0);
    float4 data_color_tl : packoffset(c1);
    float4 data_color_tr : packoffset(c2);
    float4 data_color_bl : packoffset(c3);
    float4 data_color_br : packoffset(c4);
};

uniform float4 gl_HalfPixel;

static float4 gl_Position;
static float2 texcoord;
static float4 color;
static float2 position;
static float2 outTexcoord;

struct SPIRV_Cross_Input
{
    float2 position : TEXCOORD0;
    float2 texcoord : TEXCOORD1;
};

struct SPIRV_Cross_Output
{
    float4 color : TEXCOORD0;
    float2 outTexcoord : TEXCOORD1;
    float4 gl_Position : POSITION;
};

void vert_main()
{
    float4 left = lerp(data_color_tl, data_color_bl, texcoord.y.xxxx);
    float4 right = lerp(data_color_tr, data_color_br, texcoord.y.xxxx);
    color = lerp(left, right, texcoord.x.xxxx);
    gl_Position = float4(position * data_scale, 0.0f, 1.0f);
    outTexcoord = texcoord;
    gl_Position.x = gl_Position.x - gl_HalfPixel.x * gl_Position.w;
    gl_Position.y = gl_Position.y + gl_HalfPixel.y * gl_Position.w;
}

SPIRV_Cross_Output main(SPIRV_Cross_Input stage_input)
{
    texcoord = stage_input.texcoord;
    position = stage_input.position;
    vert_main();
    SPIRV_Cross_Output stage_output;
    stage_output.gl_Position = gl_Position;
    stage_output.color = color;
    stage_output.outTexcoord = outTexcoord;
    return stage_output;
}
kvark commented 5 years ago

Interestingly, the data is push constants in one case and a constant buffer in another. I guess spirv-cross options are different in your run?

CryZe commented 5 years ago

Yeah I forgot to set any options, but they barely changed anything. Here is the HLSL code that gfx generates:

[2019-01-08T18:11:59Z DEBUG gfx_backend_dx12::device] SPIRV-Cross generated shader:

cbuffer SPIRV_CROSS_RootConstant_data : register(b0, space0)
{
    float data_scale : packoffset(c0);
    float4 data_color_tl : packoffset(c1);
};
static float4 gl_Position;
static float2 texcoord;
static float4 color;
static float2 position;
static float2 outTexcoord;
struct SPIRV_Cross_Input
{
    float2 position : TEXCOORD0;
    float2 texcoord : TEXCOORD1;
};
struct SPIRV_Cross_Output
{
    float4 color : TEXCOORD0;
    float2 outTexcoord : TEXCOORD1;
    float4 gl_Position : SV_Position;
};
void vert_main()
{
    float4 left = lerp(data_color_tl, data_color_bl, texcoord.y.xxxx);
    float4 right = lerp(data_color_tr, data_color_br, texcoord.y.xxxx);
    color = lerp(left, right, texcoord.x.xxxx);
    gl_Position = float4(position * data_scale, 0.0f, 1.0f);
    outTexcoord = texcoord;
    gl_Position.y = -gl_Position.y;
}
SPIRV_Cross_Output main(SPIRV_Cross_Input stage_input)
{
    texcoord = stage_input.texcoord;
    position = stage_input.position;
    vert_main();
    SPIRV_Cross_Output stage_output;
    stage_output.gl_Position = gl_Position;
    stage_output.color = color;
    stage_output.outTexcoord = outTexcoord;
    return stage_output;
}
kvark commented 5 years ago

Thank you! looks like we are missing quite a few push constants cc @msiglreith

CryZe commented 5 years ago

The root constant layout calculation seems to be wrong:

        let root_constant_layout = layout
            .root_constants
            .iter()
            .filter_map(|constant| {
                if constant.stages.contains(stage_flag) {
                    Some(hlsl::RootConstant {
                        start: constant.range.start * 4,
                        end: constant.range.end * 4,
                        binding: constant.range.start,
                        space: 0,
                    })
                } else {
                    None
                }
            }).collect();

This calculates that there is one constant of the range 0..32, which when bumped to 0..128 via a debugger yields the correct cbuffer in the HLSL code.

CryZe commented 5 years ago

Oh wow, so apparently that means that it takes the layout from the pipeline layout, where I have 0..8 specified. So in a way, this is not a bug, but my own mistake, but I feel like this is a very easy thing to miss. Maybe the panic should include a message saying that the pipeline layout's push constant size needs to be correct.

This can be closed otherwise.

kvark commented 5 years ago

Yes, a panic would certainly be welcome here (especially since it's not performance critical code), but at the end of the day we aren't guaranteeing correctness if it doesn't panic, so this isn't really a bug we have to fix. De-prioritizing, leaving open to add a panic (do you want to file a PR?).