Implement mesh node output params #6482
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
To enable reuse of the mesh code generation for output parameters, mesh nodes now share the mesh shader function properties instead of having their own copies. This allows us to rely on the same code to detect errors and pass along parameter information. Since nodes don't store their info in the shader info union, there isn't a conflict. Similar sharing with mesh signature lowering is possible by calling the mesh signature lowering stages for mesh nodes and lightly altering those functions to accomodate for node shaders. Some changes were made to validation to allow for the new parameters and DXIL intrinsics in node shaders. Created a convenience method in function properties to identify mesh nodes as I was tired of typing out the conditionals to identify it. Adds tests for invalid output parameters, misuse of SetMeshOutputCounts, and a thorough end-to-end node record-to-mesh output test. Fixes microsoft#6475 Add a raft of tests for mesh node out param fails enable signature lowering more tests
pow2clk
added
the
experimental-mesh-nodes
Contributor
|
✅ With the latest revision this PR passed the Python code formatter. |
Contributor
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
added 3 commits
March 29, 2024 20:43
tex3d
reviewed
Apr 1, 2024
Contributor
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
Some general things (see comments for specifics):
- Gate mesh node metadata on
IsMeshNode()rather than things like validator version for HL IR or the field of a union member (likeShaderProps.MS.outputTopology). - Don't access
ShaderProps.MSunion member without being sure it's valid (IsMS() || IsMeshNode()). - Validation tests should probably be IR based, not HLSL based, since we'd likely want to prevent some of these cases in HLSL before reaching validation at some point.
- Test file paths could be improved.
tools/clang/test/CodeGenDXIL/hlsl/objects/NodeObjects/mesh-node-input-parameters.hlsl
Show resolved
Hide resolved
tools/clang/test/CodeGenDXIL/hlsl/objects/NodeObjects/mesh-node-input-parameters.hlsl
Show resolved
Hide resolved
tools/clang/test/CodeGenDXIL/hlsl/objects/NodeObjects/mesh-node-multi-setoutcounts.hlsl
Show resolved
Hide resolved
tools/clang/test/HLSLFileCheck/shader_targets/mesh/notArrayOutIndicesMesh.hlsl
Show resolved
Hide resolved
damyanp
approved these changes
Apr 1, 2024
tex3d
reviewed
Apr 1, 2024
tools/clang/test/HLSLFileCheck/shader_targets/mesh-node/notArrayOutIndicesNode.hlsl
Show resolved
Hide resolved
tex3d
approved these changes
Apr 1, 2024
Contributor
tex3d
left a comment
tex3d
left a comment
There was a problem hiding this comment.
I think this is basically fine to merge for now. Follow-ups can be done later.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
To enable reuse of the mesh code generation for output parameters, mesh nodes now share the mesh shader function properties instead of having their own copies. This allows us to rely on the same code to detect errors and pass along parameter information. Since nodes don't store their info in the shader info union, there isn't a conflict.
Similar sharing with mesh signature lowering is possible by calling the mesh signature lowering stages for mesh nodes and lightly altering those functions to accomodate for node shaders.
Some changes were made to validation to allow for the new parameters and DXIL intrinsics in node shaders.
Created a convenience method in function properties to identify mesh nodes as I was tired of typing out the conditionals to identify it.
Adds tests for invalid output parameters, misuse of SetMeshOutputCounts, and a thorough end-to-end node record-to-mesh output test.
Fixes #6475