Skip to content

[spirv] AMD work graphs extension #7353

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

danbrown-amd
Copy link
Contributor

Enables work graphs for SPIR-V target, based on AMD_shader_enqueue extension.
Closes #5960.

Copy link
Contributor

github-actions bot commented Apr 16, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@damyanp damyanp requested review from tex3d and llvm-beanz April 16, 2025 00:40
@damyanp
Copy link
Member

damyanp commented Apr 16, 2025

@tex3d / @llvm-beanz - could you have a look at this please? Although this is tagged as a SPIR-V change, I see changes in DXIL-specific files as well as changes in generic HLSL files that aren't gated behind any spir-v ifdefs.

@danbrown-amd danbrown-amd force-pushed the spirv-work-graphs-pr branch 2 times, most recently from c751daf to 6a4aca0 Compare April 16, 2025 01:19
@s-perron s-perron requested review from Keenuts and s-perron April 17, 2025 10:52
Copy link
Collaborator

@s-perron s-perron left a comment

Choose a reason for hiding this comment

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

This is my first pass. Generally looks pretty good, but it is a large PR. It would be best to separate this into smaller PRs, especially for changes that are not specific to adding workgraphs to the SPIR-V backend.

At a minimum, we should have separate PRs for:

  1. Changes to the AST to enable spec constants in the attributes. This could be a change you do after the main change or before, but not with it because it affect DXIL. If there is a regression there, we want it to be a small change to make it easy to find the bug. We could check if we can change the attributes have ExprAttributesArgs.
  2. Have a separate PR for the OpExecutionMode/OpExecutionModeId refactoring. That is a good change, but, again, if it caused problems, it would be hard to determine where the bug came from.
  3. If you want to change the default target environment, that should be it own PR. That could be a major change for some people. If their code breaks, and they track it to this PR, it would be hard to understand why. Personally, if we change it, we will have to discuss what is should be, and it should not be based on the shader model.
  4. The main change to have compute nodes. (Is that the right term?)
  5. Finally another PR for the mesh nodes.

@danbrown-amd
Copy link
Contributor Author

I agree that it's best to split up this PR. I'll eventually update the branch with the reduced work graphs extension (item 4 of your list).

@danbrown-amd
Copy link
Contributor Author

After a glance at #7084 I'm thinking that it would take care of the second item and provide infrastructure for the first. Could you nudge someone to review it?

I'll probably drop the third item; I'm confident that the changes to CapabilityVisitor will take care of whatever issue I was trying to solve there. Mesh nodes can wait until it's been figured out what will happen with release-preview-mesh-nodes.

@Keenuts
Copy link
Collaborator

Keenuts commented May 6, 2025

Hi,
I was hopping to test this using lavapipe (don't have hardware for this), but looks like they only implemented the first draft of this extensions spec, while this implements the second draft version?

-> Shall we always emit a DXC warning when emitting this extension to say this is a beta extension, and it's might not work/might change? Or is this extensions now "stable"?

@danbrown-amd danbrown-amd force-pushed the spirv-work-graphs-pr branch 4 times, most recently from 2ad475f to 15fc00d Compare May 21, 2025 22:09
@danbrown-amd danbrown-amd force-pushed the spirv-work-graphs-pr branch from 15fc00d to 78e7573 Compare May 21, 2025 22:49
@danbrown-amd
Copy link
Contributor Author

It may not be a bad idea to add a warning. I'm not aware of any concrete plans to revise the spec but expect that will happen eventually. The current one has some limitations, one consequence of which is that passing nodes and node records to called functions is not supported. Also, validation generally has to be turned off because the spec (and therefore SPIRV-Tools) doesn't explicitly allow pointers of storage class NodePayloadAMDX to be function arguments as the entry function wrapper requires.

@Keenuts
Copy link
Collaborator

Keenuts commented May 26, 2025

It may not be a bad idea to add a warning. I'm not aware of any concrete plans to revise the spec but expect that will happen eventually. The current one has some limitations, one consequence of which is that passing nodes and node records to called functions is not supported. Also, validation generally has to be turned off because the spec (and therefore SPIRV-Tools) doesn't explicitly allow pointers of storage class NodePayloadAMDX to be function arguments as the entry function wrapper requires.

Oh good to know, in that case I'd be in favor of either a warning, or gate this being a flag like -fspv-experimental-work-graphs to make it explicit this is not production ready at all.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: New
Development

Successfully merging this pull request may close these issues.

[SPIRV] Work graphs
4 participants