PENGUINLIONG/spirq-rs

Access is not correctly calculated for some storage buffers

expenses opened this issue · 6 comments

Hi! Thanks for this fantastic crate, it's worked so well for my usecases. I'm running into a storage buffer problem with the attached shader where the descriptor type is StorageBuffer(ReadWrite) when it should be ReadOnly.

moon.spv.zip

I've looked at the visualization from https://www.khronos.org/spir/visualizer/ and inserted some dbg! statements which showed that get_desc_access is being called on id %16 and not %14, which is the thing actually being decorated by NonWritable:

spirq-rs/spirq/src/reflect.rs

Lines 1340 to 1345 in f77af02

// For SPIR-V versions <= 1.3 in which `BufferBlock` is not
// deprecated.
let var = if self.contains_deco(ty_id, None, Decoration::BufferBlock) {
let access = self
.get_desc_access(op.var_id)
.ok_or(Error::ACCESS_CONFLICT)?;

20230526_14h05m26s_grim

In fact, the same problem seems to happen even when I recompile it with the -fspv-target-env=vulkan1.3 hlsl flag which removed BufferBlock:

moon2.spv.zip

Thanks for your report. Will look into this.

It's kinda funny that your shader compiler applies the NonWritable decoration to the structure members instead of the declared variable. What was that? DXC?

Hi, hopefully this issue is addressed by #106. To ensure that this feature doesn't break in the future is it okay that I upload your moon.spv as a test case to this repo?

Hi, hopefully this issue is addressed by #106. To ensure that this feature doesn't break in the future is it okay that I upload your moon.spv as a test case to this repo?

Fantastic, yep that fixes it! You're absolutely welcome to add the shader as a test case. Here's the hlsl source as well (currently just a partially-implemented bindless pbr thing):

struct PushConstant {
    float4x4 combined_matrix;
    float3 camera_pos;
};

static const uint INVALID = 4294967295;

struct MaterialInfo {
    float4 base_color_factor;
    float3 emissive_factor;
    float metallic_factor;
    float roughness_factor;
    uint albedo_texture;
    uint normal_texture;
    uint emissive_texture;
};

[[vk::push_constant]]
PushConstant constant;

struct Varying {
    float4 builtin_position: SV_Position;
    float3 position: POSITION0;
    float2 uv: TEXCOORD0;
    float3 normal: NORMAL0;
    uint material_id: TEXCOORD1;
};

[[vk::binding(0)]] Texture2D<float3> tex[];
[[vk::binding(1)]] SamplerState samp;
[[vk::binding(2)]] StructuredBuffer<MaterialInfo> infos;

float length_squared(float3 vec) {
    return dot(vec, vec);
}

float3x3 compute_cotangent_frame(
    float3 normal,
    float3 position,
    float2 uv
) {
    float3 delta_pos_x = ddx(position);
    float3 delta_pos_y = ddy(position);
    float2 delta_uv_x = ddx(uv);
    float2 delta_uv_y = ddy(uv);

    float3 delta_pos_y_perp = cross(delta_pos_y, normal);
    float3 delta_pos_x_perp = cross(normal, delta_pos_x);

    float3 t = delta_pos_y_perp * delta_uv_x.x + delta_pos_x_perp * delta_uv_y.x;
    float3 b = delta_pos_y_perp * delta_uv_x.y + delta_pos_x_perp * delta_uv_y.y;

    float invmax = 1.0 / sqrt(max(length_squared(t), length_squared(b)));
    return transpose(float3x3(t * invmax, b * invmax, normal));
}
    

[shader("pixel")]
float4 PSMain(
    Varying varying
): SV_Target0 {
    MaterialInfo info = infos[varying.material_id];

    float3 normal = normalize(varying.normal);

    float3 view_vector = constant.camera_pos - varying.position;

    if (info.normal_texture != INVALID) {
        float3 tex_normal = tex[info.normal_texture].Sample(samp, varying.uv).xyz * 255.0 / 127.0 - 128.0 / 127.0;
        normal = normalize(mul(compute_cotangent_frame(normal, -view_vector, varying.uv), tex_normal));
    }

    float3 sun_dir = normalize(float3(1,1,1));
    float brightness = max(dot(normal, sun_dir), 0.0);

    float3 albedo = info.base_color_factor.rgb;

    if (info.albedo_texture != INVALID) {
        albedo *= tex[info.albedo_texture].Sample(samp, varying.uv).xyz;
    }

    float3 emissive = info.emissive_factor;

    if (info.emissive_texture != INVALID) {
        emissive *= tex[info.emissive_texture].Sample(samp, varying.uv).xyz;
    }

    return float4(albedo * brightness + emissive, 1.0);
}

You can compile it with dxc shader.hlsl -HV 2021 -T ps_6_6 -E PSMain -spirv -Fo shader.spv. dxc --version reports dxcompiler.dll: 1.7(dev;3759-8c9d92be).

Thank you! :)