Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Invalid SPIR-V generation for outer product mul #5987

Open
Axel-Reactor opened this issue Jan 2, 2025 · 9 comments · May be fixed by #6044
Open

Invalid SPIR-V generation for outer product mul #5987

Axel-Reactor opened this issue Jan 2, 2025 · 9 comments · May be fixed by #6044
Assignees
@aleino-nv
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Milestone
Q1 2025 (Winter)

Comments

@Axel-Reactor
Copy link 

#version 460

struct TYPE_bindings_FragmentShaderOutputsExample {
    vector<float32_t, 4> m_color_0;
    vector<float32_t, 4> m_color_1;
};

layout (location = 0) in vector<float32_t, 2> BINDING_bindings_FragmentShaderInputsExample_texcoord;
layout (location = 0) out vector<float32_t, 4> BINDING_bindings_FragmentShaderOutputsExample_color_0;
layout (location = 1) out vector<float32_t, 4> BINDING_bindings_FragmentShaderOutputsExample_color_1;

[shader("fragment")] void main()
{
    matrix<float32_t, 4, 1> TEMP_3 = matrix<float32_t, 4, 1>(0);
    matrix<float32_t, 4, 1> r1_4 = TEMP_3;
    vector<float32_t, 4> TEMP_5 = vector<float32_t, 4>(0);
    vector<float32_t, 4> c1_6 = TEMP_5;
    matrix<float32_t, 1, 4> TEMP_7 = { c1_6[0], c1_6[1], c1_6[2], c1_6[3] };
    matrix<float32_t, 4, 4> TEMP_8 = mul(r1_4, TEMP_7);
    matrix<float32_t, 4, 4> m1_9 = TEMP_8;
    vector<float32_t, 4> TEMP_11 = vector<float32_t, 4>((m1_9[0][0]));
    vector<float32_t, 4> TEMP_12 = TEMP_11;
    vector<float32_t, 4> TEMP_13 = vector<float32_t, 4>(0);
    vector<float32_t, 4> TEMP_14 = TEMP_13;
    TYPE_bindings_FragmentShaderOutputsExample TEMP_15 = TYPE_bindings_FragmentShaderOutputsExample(TEMP_12, TEMP_14);
    TYPE_bindings_FragmentShaderOutputsExample TEMP_16 = TEMP_15;
    BINDING_bindings_FragmentShaderOutputsExample_color_0 = TEMP_16.m_color_0;
    BINDING_bindings_FragmentShaderOutputsExample_color_1 = TEMP_16.m_color_1;
    return;
}

slangc test.slang -target spirv -o test.spv
spirv-val test.spv

->

error: line 17: Matrix types can only be parameterized as having only 2, 3, or 4 columns.
  %mat1v4float = OpTypeMatrix %v4float 1

Specifically I believe this originates from this outer product mul (mat4x1 * mat1x4): matrix<float32_t, 4, 4> TEMP_8 = mul(r1_4, TEMP_7);
@Axel-Reactor Axel-Reactor changed the title Invalid SPIR-V generation Invalid SPIR-V generation for outer product mul Jan 2, 2025
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 2, 2025

If possible, please avoid using matrix types where row or col is 1. They should be >= 2.

@csyonghe
Copy link
Collaborator

csyonghe commented Jan 2, 2025

1-vector and 1-matrix types are not well defined and can lead to all sorts of issues in the type system. They are not supported by SPIRV, Metal, WGSL, GLSL and not officially supported by Slang at the moment.

@Axel-Reactor
Copy link
Author

I see. I would still like to see an error then instead of producing invalid SPIR-V.

@jtytgat
Copy link

jtytgat commented Jan 6, 2025

1-vector and 1-matrix types are not well defined and can lead to all sorts of issues in the type system. They are not supported by SPIRV, Metal, WGSL, GLSL and not officially supported by Slang at the moment.

True, but for those cases DXC will automatically switch to vector/scalar types in its SPIR-V output, cfr. https://github.com/microsoft/DirectXShaderCompiler/blob/main/docs/SPIR-V.rst#indexing-operator
Isn't that an option here?

@jtytgat jtytgat mentioned this issue Jan 6, 2025
Open
@csyonghe
Copy link
Collaborator

csyonghe commented Jan 6, 2025

@jtytgat it is, and we do already have some level of support of 1-vectors. But we don’t yet have 1-matrix support. There are more work around 1-matrices because we will then rewrite all the algebra operations and we did not have time to get to it. Before the support around 1-matrices are complete, users should avoid using them to prevent running into weird corner case issues. We appreciate patches to make 1-matrices functional.

@bmillsNV bmillsNV added this to the Q1 2025 (Winter) milestone Jan 7, 2025
@bmillsNV
Copy link
Collaborator

bmillsNV commented Jan 7, 2025

@aleino-nv can you help to add an error for this one?

@bmillsNV bmillsNV added the goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang label Jan 7, 2025
aleino-nv added a commit to aleino-nv/slang that referenced this issue Jan 9, 2025
@aleino-nv
Emit errors when seeing a matrix with row/col count of 1
c46e1aa 
Such matrices aren't well supported by our targets, and we currently don't transform such
types to supported ones.
Therefore, generate an error rather than outputting invalid target code.

This closes shader-slang#5987.
@aleino-nv aleino-nv linked a pull request Jan 9, 2025 that will close this issue
Open
@aleino-nv
Copy link
Collaborator

Here is a PR that generates error messages like the following, on the above code:

> slangc -o test.spv -target spirv -stage fragment -entry fragmentMain test.slang
test.slang(14): error 39999: matrix of type 'matrix<float,4,1>' has column or row count of 1
    matrix<float32_t, 4, 1> TEMP_3 = matrix<float32_t, 4, 1>(0);
                            ^~~~~~
test.slang(15): error 39999: matrix of type 'matrix<float,4,1>' has column or row count of 1
    matrix<float32_t, 4, 1> r1_4 = TEMP_3;
                            ^~~~
test.slang(18): error 39999: matrix of type 'matrix<float,1,4>' has column or row count of 1
    matrix<float32_t, 1, 4> TEMP_7 = { c1_6[0], c1_6[1], c1_6[2], c1_6[3] };
                            ^~~~~~

@aleino-nv
Copy link
Collaborator

...although that doesn't quite do it in all cases. I just found out that this can be circumvented like so:

#version 460

struct TYPE_bindings_FragmentShaderOutputsExample {
    vector<float32_t, 4> m_color_0;
    vector<float32_t, 4> m_color_1;
};

layout (location = 0) in vector<float32_t, 2> BINDING_bindings_FragmentShaderInputsExample_texcoord;
layout (location = 0) out vector<float32_t, 4> BINDING_bindings_FragmentShaderOutputsExample_color_0;
layout (location = 1) out vector<float32_t, 4> BINDING_bindings_FragmentShaderOutputsExample_color_1;

matrix<float32_t, 4, 1> fun()
{
    return matrix<float32_t, 4, 1>(0);
}

matrix<float32_t, 1, 4> fun2()
{
    return matrix<float32_t, 1, 4>(0);
}

[shader("fragment")] void fragmentMain()
{
    vector<float32_t, 4> TEMP_5 = vector<float32_t, 4>(0);
    vector<float32_t, 4> c1_6 = TEMP_5;
    matrix<float32_t, 4, 4> TEMP_8 = mul(fun(), fun2());
    matrix<float32_t, 4, 4> m1_9 = TEMP_8;
    vector<float32_t, 4> TEMP_11 = vector<float32_t, 4>((m1_9[0][0]));
    vector<float32_t, 4> TEMP_12 = TEMP_11;
    vector<float32_t, 4> TEMP_13 = vector<float32_t, 4>(0);
    vector<float32_t, 4> TEMP_14 = TEMP_13;
    TYPE_bindings_FragmentShaderOutputsExample TEMP_15 = TYPE_bindings_FragmentShaderOutputsExample(TEMP_12, TEMP_14);
    TYPE_bindings_FragmentShaderOutputsExample TEMP_16 = TEMP_15;
    BINDING_bindings_FragmentShaderOutputsExample_color_0 = TEMP_16.m_color_0;
    BINDING_bindings_FragmentShaderOutputsExample_color_1 = TEMP_16.m_color_1;
    return;
}

I'll try to find a better place for this check... Any usage of e.g. matrix<float32_t, N, 1> should be caught.

@aleino-nv
Copy link
Collaborator

aleino-nv commented Jan 9, 2025 •
edited
Loading

https://github.com/shader-slang/slang/blob/master/tests/bugs/gh-4395.slang is now generating errors.
@csyonghe Should this error not be reported if the target is HLSL, or what?

Or maybe we add an option to opt out of this error, and then pass that option in this particular test?
That's maybe better since it's nice to let the user know that their Slang code is not portable, by default, even if they happen to be compiling for HLSL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@aleino-nv aleino-nv

Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang
Projects
None yet
Milestone
Q1 2025 (Winter)
Development

Successfully merging a pull request may close this issue.

Emit errors when seeing a matrix with row/col count of 1 aleino-nv/slang
5 participants
@csyonghe @aleino-nv @jtytgat @Axel-Reactor @bmillsNV