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

Embed core module in wasm build. #5569

Merged
merged 4 commits into from
Nov 15, 2024
Merged

Conversation

csyonghe
Copy link
Collaborator

This change allows us to embed prebuilt core module in WebAssembly.

The main issue being fixed here is that some of our AST nodes contain fields of type Index or UInt, whose sizes are architecture dependent. This means that the serialized module produced from x64 build system won't correct load by the web assembly module, which is 32-bit. The fix is to get rid of these arch-dependent int types in AST nodes.

To prevent future regression, the slang-cpp-extract application is updated to report an error if it sees an AST node field is of such types.

@csyonghe csyonghe requested a review from a team as a code owner November 15, 2024 06:56
@csyonghe csyonghe added the pr: non-breaking PRs without breaking changes label Nov 15, 2024
@csyonghe
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

"UInt",
"Index",
"Count",
"UIndex",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How will this code get updated if the names these types change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is not a good way to guard against that unfortunately. But we shouldn't be changing the names of these types.

"UIndex",
"UCount",
"PtrInt",
"intptr_t"};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also uintptr_t

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

aleino-nv
aleino-nv previously approved these changes Nov 15, 2024
Copy link
Collaborator

@aleino-nv aleino-nv left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me

@csyonghe csyonghe merged commit 05903f7 into shader-slang:master Nov 15, 2024
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants