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

Clspv crashes compiling code writing to structs with varying size fields. #1166

Closed
BukeBeyond opened this issue Jul 26, 2023 · 3 comments · Fixed by #1167
Closed

Clspv crashes compiling code writing to structs with varying size fields. #1166

BukeBeyond opened this issue Jul 26, 2023 · 3 comments · Fixed by #1167
Assignees
@rjodinchr

Comments

@BukeBeyond
Copy link 

struct S { long i1; int i2; int i3; }; // Fail: i64i32*** stack smashing detected ***
// struct S { int i1; long i2; int i3; }; // OK: First field has the minimum size of all fields
// struct S { int i1; char i2; char i3; }; // Fail
// struct S { char Fix; int i1; char i2; char i3; }; // OK: byte size first field fixes all

kernel void Kernel(global struct S* s)
{
    s->i1 = 0;
    s->i2 = 0;
    s->i3 = 0;
}

godbolt.org/z/533he8K96
@BukeBeyond
Copy link
Author

I should add, Clang 17 pre-compiles these fine, as in #1142

The bug is in Clspv Vulkan conversion even when given the pre-compiled ll file.

@rjodinchr rjodinchr self-assigned this Jul 27, 2023
rjodinchr added a commit to rjodinchr/clspv that referenced this issue Jul 27, 2023
@rjodinchr
fix SmallerTypeNotAliasing when a structure is involved
68f0389 
If the bigger type is a struct, we need to make sure that all its
fields are bigger or equal to the type selected to return. Otherwise,
return the smaller type amongst the field of the structure.

This fix ends up generating vector of 1 element. Fix getSPIRVConstant
and getSPIRVType to avoid generating vector of 1 element as it is not
allowed in SPIRV-V.

Fix google#1166
@rjodinchr rjodinchr mentioned this issue Jul 27, 2023
Merged
@rjodinchr
Copy link
Collaborator

Thank you for the bug report.
The pull request mentioned above should fix the issue.

rjodinchr added a commit to rjodinchr/clspv that referenced this issue Jul 27, 2023
@rjodinchr
fix SmallerTypeNotAliasing when a structure is involved
cf515fc 
If the bigger type is a struct, we need to make sure that all its
fields are bigger or equal to the type selected to return. Otherwise,
return the smaller type amongst the field of the structure.

This fix ends up generating vector of 1 element. Fix getSPIRVConstant
and getSPIRVType to avoid generating vector of 1 element as it is not
allowed in SPIRV-V.

Fix google#1166
alan-baker pushed a commit that referenced this issue Jul 27, 2023
@rjodinchr
fix SmallerTypeNotAliasing when a structure is involved (#1167)
3002cf2 
If the bigger type is a struct, we need to make sure that all its
fields are bigger or equal to the type selected to return. Otherwise,
return the smaller type amongst the field of the structure.

This fix ends up generating vector of 1 element. Fix getSPIRVConstant
and getSPIRVType to avoid generating vector of 1 element as it is not
allowed in SPIRV-V.

Fix #1166
@BukeBeyond
Copy link
Author

BukeBeyond commented Jul 31, 2023 •
edited
Loading

With previous GPU technologies, direct access to complex structures in memory never worked right. So we had to implement intricate serialization to replicate objects.

But here it is, you proved this can work, after fixing this bug!

So good to see Clspv back on track, leading the pack.

Thank you Mr. Jodin and Mr. Baker!

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

@rjodinchr rjodinchr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

fix SmallerTypeNotAliasing when a structure is involved rjodinchr/clspv
2 participants
@BukeBeyond @rjodinchr