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

Legalize the Entry-point for WGSL #5498

Merged
merged 5 commits into from
Nov 6, 2024
Merged

Legalize the Entry-point for WGSL #5498

merged 5 commits into from
Nov 6, 2024

Conversation

jkwak-work
Copy link
Collaborator

@jkwak-work jkwak-work commented Nov 5, 2024 •
edited
Loading

Closes #5486

The return type of the entry-point needs to be legalized when targeting WGSL.
This commit flattens the nested-structs of the return type and the input parameters of the entry-point.

Most of code is copied from the legalization code for Metal. The following functions are exactly same to the implementation for Metal or almost same.

  • flattenInputParameters() : 136 lines
  • reportUnsupportedSystemAttribute() : 7 lines
  • ensureResultStructHasUserSemantic() : 46 lines
  • struct MapStructToFlatStruct : 176 lines
  • flattenNestedStructs() : 95 lines
  • maybeFlattenNestedStructs() : 42 lines
  • _replaceAllReturnInst() : 19 lines
  • _returnNonOverlappingAttributeIndex() : 16 lines
  • _replaceAttributeOfLayout() : 23 lines
  • tryConvertValue() : 41 lines
  • legalizeSystemValueParameters() : 11 lines

They need to be refactored to reduce the duplication later.

@jkwak-work
Legalize the Entry-point for WGSL
60b8744 
The return type of the entry-point needs to be legalized when targeting
WGSL.
This commit flattens the nested-structs of the return type and the input
parameters of the entry-point.

Most of code is copied from the legalization code for Metal. The
following functions are exactly same to the implementation for Metal or
almost same.
- flattenInputParameters() : 136 lines
- reportUnsupportedSystemAttribute() : 7 lines
- ensureResultStructHasUserSemantic() : 46 lines
- struct MapStructToFlatStruct : 176 lines
- flattenNestedStructs() : 95 lines
- maybeFlattenNestedStructs() : 42 lines
- _replaceAllReturnInst() : 19 lines
- _returnNonOverlappingAttributeIndex() : 16 lines
- _replaceAttributeOfLayout() : 23 lines
- tryConvertValue() : 41 lines
- legalizeSystemValueParameters() : 11 lines

They need to be refactored to reduce the duplication later.
@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Nov 5, 2024
@jkwak-work jkwak-work self-assigned this Nov 5, 2024
@jkwak-work jkwak-work requested a review from a team as a code owner November 5, 2024 22:18
@jkwak-work
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

slangbot commented Nov 5, 2024

🌈 Formatted, please merge the changes from this PR

@slangbot slangbot mentioned this pull request Nov 5, 2024
Merged
@jkwak-work
Copy link
Collaborator Author

For the review, source/slang/slang-ir-wgsl-legalize.cpp is better to be compared to source/slang/slang-ir-metal-legalize.cpp.

csyonghe
csyonghe previously approved these changes Nov 5, 2024
View reviewed changes
Copy link
Collaborator

@csyonghe csyonghe 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. We definitely need to refactor this to reuse as much code as possible between the two backends.

@jkwak-work
Copy link
Collaborator Author

It seems like tests/compute/assoctype-lookup.slang is failing because of this change.
I am looking into it now.

@csyonghe
Copy link
Collaborator

csyonghe commented Nov 5, 2024

Can we also turn on all the relevant tests we have for metal to also cover wgsl?

@jkwak-work
Fix the failing test
e71829b 
The test case, `tests/compute/assoctype-lookup.slang`, had a bug that
the compute shader was trying to use the varying input/output with the
user defined semantics.

This commit removes the user defined semantics, because the compute
shaders cannot use the user defined semantics.
@jkwak-work
Copy link
Collaborator Author

Can we also turn on all the relevant tests we have for metal to also cover wgsl?

I see. That's a good idea.
Let me do that.

@jkwak-work
Copy link
Collaborator Author

I appears that this PR is missing a few use cases.
The following tests should be enabled but I cannot enabled them with the PR.

  • tests/compute/compile-time-loop.slang
  • tests/compute/constexpr.slang
  • tests/compute/discard-stmt.slang
  • tests/compute/texture-sampling.slang
  • tests/metal/atomic-intrinsics.slang
  • tests/metal/nested-struct-fragment-input.slang
  • tests/metal/nested-struct-fragment-output.slang
  • tests/metal/nested-struct-multi-entry-point-vertex.slang
  • tests/metal/no-struct-vertex-output.slang
  • tests/metal/stage-in-2.slang
  • tests/metal/sv_target-complex-1.slang
  • tests/metal/sv_target-complex-2.slang

I will make another separate PR for resolving the problem and enabling them.

@jkwak-work jkwak-work merged commit 79056cd into shader-slang:master Nov 6, 2024
14 checks passed
@jkwak-work jkwak-work deleted the feature/wgsl_support_basic_varying_input branch November 6, 2024 00:31
This was referenced Nov 6, 2024
Closed
Closed
@aleino-nv aleino-nv mentioned this pull request Nov 18, 2024
Merged
@fairywreath fairywreath mentioned this pull request Dec 24, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@csyonghe csyonghe csyonghe left review comments

Assignees

@jkwak-work jkwak-work

Labels
pr: non-breaking PRs without breaking changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[WGSL] Binding keywords are missing for a vertex shader entry-point
3 participants
@jkwak-work @slangbot @csyonghe