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.

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

Problems with matrix layout #3752

Closed
Ipotrick opened this issue Mar 12, 2024 · 5 comments · Fixed by #3753
Closed

Problems with matrix layout #3752

Ipotrick opened this issue Mar 12, 2024 · 5 comments · Fixed by #3753
Assignees
Labels
goal:client support Feature or fix needed for a current slang user.

Comments

@Ipotrick
Copy link

Ipotrick commented Mar 12, 2024

Setting slang::SessionDesc::defaultMatrixLayoutMode to SLANG_MATRIX_LAYOUT_COLUMN_MAJOR, seems to have no effect when compiling with the following settings:

pc platform: x86-64msvc windows11
slang::TargetDesc::format: SlangCompileTarget::SLANG_SPIRV
slang::TargetDesc::profile: GLSL_460
slang::TargetDesc::flags: SLANG_TARGET_FLAG_GENERATE_SPIRV_DIRECTLY
slang::SessionDesc::defaultMatrixLayoutMode: SLANG_MATRIX_LAYOUT_COLUMN_MAJOR

Also, i found that specific typedef qualifier syntax causes an internal compiler error:

 SLANG [test] D:/repos/Timberdoodle/src/rendering/rasterize_visbuffer/draw_visbuffer.slang(63): internal error 99999: unexpected condition encountered in Slang compiler: unknown type modifier in semantic checking
typedef column_major float3x4 tester;
        ^~~~~~~~~~~~

I would like this syntax ( typedef qualifier type alias ) to work.

@csyonghe csyonghe self-assigned this Mar 12, 2024
@csyonghe csyonghe added the goal:client support Feature or fix needed for a current slang user. label Mar 12, 2024
@csyonghe csyonghe added this to the Q1 2024 (Winter) milestone Mar 12, 2024
@Ipotrick
Copy link
Author

Correction!
This is actually a buffer pointer bug!
When reading a float3x4 matrix from a buffer pointer by directly dereferencing it, the majorness is respected!
But when reading from the pointer array style it reads it as row major!
Example and repro:

struct Push
{
    float3x4* ptr;
};

[[vk::push_constant]] Push push;
[shader("compute")]
[numthreads(1, 1, 1)]
void main(uint3 dtid : SV_DispatchThreadID)
{    
    // This matrix is in memry column major. Slang respects this here and load it properly!
    float3x4 correctly_read_matrix = *push.ptr;
    printf("(%f,%f,%f,%f)\n(%f,%f,%f,%f)\n",
        correctly_read_matrix[0][0], correctly_read_matrix[0][1], correctly_read_matrix[0][2], correctly_read_matrix[0][3],
        correctly_read_matrix[1][0], correctly_read_matrix[1][1], correctly_read_matrix[1][2], correctly_read_matrix[1][3]
    );
    printf("(%f,%f,%f,%f)\n\n",
        correctly_read_matrix[2][0], correctly_read_matrix[2][1], correctly_read_matrix[2][2], correctly_read_matrix[2][3]
    );
    // With this syntax however, Slang ignores the column major setting and loads it as it it was row major!
    float3x4 broken_matrix = push.ptr[0];
    printf("(%f,%f,%f,%f)\n(%f,%f,%f,%f)\n",
        broken_matrix[0][0], broken_matrix[0][1], broken_matrix[0][2], broken_matrix[0][3],
        broken_matrix[1][0], broken_matrix[1][1], broken_matrix[1][2], broken_matrix[1][3]
    );
    printf("(%f,%f,%f,%f)\n\n",
        broken_matrix[2][0], broken_matrix[2][1], broken_matrix[2][2], broken_matrix[2][3]
    );
}

I wrote the following matrix to a buffer:

buffer_host_ptr =  f32mat3x4{
        // rc = row column
        {11, 21, 31},   // col 1
        {12, 22, 32},   // col 2
        {13, 23, 33},   // col 3
        {14, 24, 34},   // col 4
    };

I get the following output from the minimal repro:

// correct matrix:
(11.000000,12.000000,13.000000,14.000000)
(21.000000,22.000000,23.000000,24.000000)
(31.000000,32.000000,33.000000,34.000000)
// broken matrix:
(11.000000,21.000000,31.000000,12.000000)
(22.000000,32.000000,13.000000,23.000000)
(33.000000,14.000000,24.000000,34.000000)

@csyonghe
Copy link
Collaborator

I looked into it and verified that sessionDesc.defaultMatrixLayout is indeed ineffective and fixed that.
Also fixed the typedef issue.

I think the buffer pointer thing is a different issue.

@Ipotrick
Copy link
Author

Btw, thank you for all the quick fixes. This is very appreciated. I had much worse experience with other shading languages fixing issues.

@csyonghe
Copy link
Collaborator

OK, the PR should fix this issue.

@csyonghe
Copy link
Collaborator

And thank you for your patience with the compiler so far. The buffer pointer thing and the API around setting compiler options are relatively new and there are still bugs, but we are committed to address any issues quickly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:client support Feature or fix needed for a current slang user.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants