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

[WIP] Allow duplication of OpTypeNodePayloadArrayAMDX #6470

Draft
wants to merge 10 commits into
base: master
Choose a base branch
from

Conversation

jkwak-work
Copy link
Collaborator

Closes #6391

This commit is mainly to allow OpTypeNodePayloadArrayAMDX to be duplicated in SPIRV when using WorkGraphs.

AMD spec requires that there should be dedicated type declaration with OpTypeNodePayloadArrayAMDX for each "node". And each of them should be decorated with PayloadNodeNameAMDX.
https://docs.vulkan.org/features/latest/features/proposals/VK_AMDX_shader_enqueue.html

This behavior is very different from how "types" are declared. Normally types are declared only once and they are shared across the whole program. But for the work-graphs, each "RecordType" belongs to each node and they need to be declared for each node.

Consider the following HLSL example where a producer triggers two consumer nodes. Each node will be differentiated by "NodeID" attribute.

[Shader("node")]
[NodeLaunch("broadcasting")]
[NumThreads(4,1,1)]
void myFancyNode(...,
    [NodeID("myNode1")] NodeOutput<MY_RECORD> myRecords1,
    [NodeID("myNode2")] NodeOutput<MY_RECORD> myRecords2,
    ...)
{
    ThreadNodeOutputRecords<MY_RECORD> myRecord1 = myRecords1.GetThreadNodeOutputRecords(1);
    myRecord1.myData = 1;
    myRecord1.OutputCompelete();

    ThreadNodeOutputRecords<MY_RECORD> myRecord2 = myRecords2.GetThreadNodeOutputRecords(1);
    myRecord2.myData = 1;
    myRecord2.OutputCompelete();
}

Even though both are using the same type, "MY_RECORD", each of them needs to be declared separately. The generated SPIR-V code for the HLSL above is something like the following,

%str_node1 = OpConstantStringAMDX "myNode1";
%str_node2 = OpConstantStringAMDX "myNode1";

%type_node1 = OpTypeNodePayloadArrayAMDX %type_myRecord;
%type_node2 = OpTypeNodePayloadArrayAMDX %type_myRecord;

OpDecorateId %type_node1 PayloadNodeNameAMDX %str_node1;
OpDecorateId %type_node2 PayloadNodeNameAMDX %str_node2;

Note that %type_node1 and %type_node2 are equivalent, but they must be declared separately because each of them will be decorated with different nodeID string.

@jkwak-work jkwak-work added the pr: non-breaking PRs without breaking changes label Feb 26, 2025
@jkwak-work jkwak-work self-assigned this Feb 26, 2025
@jkwak-work jkwak-work changed the title Allow duplication of OpTypeNodePayloadArrayAMDX [WIP] Allow duplication of OpTypeNodePayloadArrayAMDX Feb 26, 2025
@jkwak-work jkwak-work force-pushed the fix/workgraphcs_fix_blocking_issue_for_lionel branch from 2241997 to f203597 Compare February 26, 2025 21:37
@jkwak-work
Copy link
Collaborator Author

This PR is not ready for review nor merge.

The main problem is that it is not able to take "nodeID" in a String type.
Current implementation takes an int-type and convert it to "NodeID_%d" during the emitting, which is a hack.

I need to get the nodeID from a new attribute [NodeID("name")] to make it a proper PR.

@jkwak-work
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

@jkwak-work jkwak-work force-pushed the fix/workgraphcs_fix_blocking_issue_for_lionel branch 2 times, most recently from 9b45d1b to 085fda4 Compare March 13, 2025 04:00
@jkwak-work
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

jkwak-work and others added 9 commits March 19, 2025 10:43
This commit is mainly to allow OpTypeNodePayloadArrayAMDX to be
duplicated in SPIRV when using WorkGraphs.

AMD spec requires that there should be dedicated type declaration with
OpTypeNodePayloadArrayAMDX for each "node". And each of them should be
decorated with PayloadNodeNameAMDX.
https://docs.vulkan.org/features/latest/features/proposals/VK_AMDX_shader_enqueue.html

This behavior is very different from how "types" are declared. Normally
types are declared only once and they are shared across the whole
program. But for the work-graphs, each "RecordType" belongs to each node
and they need to be declared for each node.

Consider the following HLSL example where a producer triggers two
consumer nodes. Each node will be differentiated by "NodeID" attribute.

```
[Shader("node")]
[NodeLaunch("broadcasting")]
[NumThreads(4,1,1)]
void myFancyNode(...,
    [NodeID("myNode1")] NodeOutput<MY_RECORD> myRecords1,
    [NodeID("myNode2")] NodeOutput<MY_RECORD> myRecords2,
    ...)
{
    ThreadNodeOutputRecords<MY_RECORD> myRecord1 = myRecords1.GetThreadNodeOutputRecords(1);
    myRecord1.myData = 1;
    myRecord1.OutputCompelete();

    ThreadNodeOutputRecords<MY_RECORD> myRecord2 = myRecords2.GetThreadNodeOutputRecords(1);
    myRecord2.myData = 1;
    myRecord2.OutputCompelete();
}
```

Even though both are using the same type, "MY_RECORD", each of them
needs to be declared separately. The generated SPIR-V code for the
HLSL above is something like the following,
```
%str_node1 = OpConstantStringAMDX "myNode1";
%str_node2 = OpConstantStringAMDX "myNode1";

%type_node1 = OpTypeNodePayloadArrayAMDX %type_myRecord;
%type_node2 = OpTypeNodePayloadArrayAMDX %type_myRecord;

OpDecorateId %type_node1 PayloadNodeNameAMDX %str_node1;
OpDecorateId %type_node2 PayloadNodeNameAMDX %str_node2;
```
Note that `%type_node1` and `%type_node2` are equivalent, but they must
be declared separately because each of them will be decorated with
different nodeID string.

Verified

This commit was signed with the committer’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.
@jkwak-work jkwak-work force-pushed the fix/workgraphcs_fix_blocking_issue_for_lionel branch from 132a91e to 274c029 Compare March 19, 2025 17:49
@jkwak-work
Copy link
Collaborator Author

/format

@slangbot
Copy link
Contributor

🌈 Formatted, please merge the changes from this PR

Co-authored-by: slangbot <186143334+slangbot@users.noreply.github.com>
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.

[WorkGraphs] experimental shader code causes Slang to crash
2 participants