Skip to content

[CoopVec] Add Linear Algebra common header with tests #7350

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 18 commits into
base: staging-sm6.9
Choose a base branch
from

Conversation

bob80905
Copy link
Collaborator

@bob80905 bob80905 commented Apr 15, 2025

This PR introduces the linear algebra header file, and places it in a location that is by default included in all HLSL compilation.
The builtins in the API aren't yet defined, and depend on the #7290 PR merging first.
The tests that have been added have temporary diagnostic messages while 7290 is in progress. They will need to be updated.
Open to feedback on better / suggested error messages, or whether there shouldn't be any sema-level validation for these errors.

Fixes #7304

@bob80905 bob80905 changed the base branch from main to staging-sm6.9 April 15, 2025 17:28
Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

I expect we'll need to mark most of these checks as expected to fail.

@github-project-automation github-project-automation bot moved this from New to In progress in HLSL Roadmap Apr 15, 2025
@bob80905 bob80905 marked this pull request as ready for review April 16, 2025 20:45
Copy link
Contributor

github-actions bot commented Apr 17, 2025

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

@bob80905 bob80905 force-pushed the add_linalg_h branch 2 times, most recently from aed847d to 0b32642 Compare April 18, 2025 22:28
Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

A few comments / questions I'd like to get cleared up before approving.

Copy link
Member

@damyanp damyanp left a comment

Choose a reason for hiding this comment

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

LGTM, assuming the test runs succeed.

If the proposal example doesn't get executed often then it'll rot. Is there anything we can do to move it somewhere where it runs? If not, then we've got the example in the proposal itself so I don't know if there's much value keeping another copy around in the DXC repo.

@@ -0,0 +1,143 @@
// RUN: %dxc -I %hlsl_headers -T lib_6_9 -enable-16bit-types %s
Copy link
Member

Choose a reason for hiding this comment

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

Does this actually get executed during the build?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This location won't run it as a test. I do think it's kind of awkward to keep it given there's a copy in the proposal itself, so I'll just delete it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a sema test right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Right

@@ -0,0 +1,178 @@
// Header for linear algebra APIs.

#if ((__SHADER_TARGET_MAJOR > 6) || \
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should we make this header error if it is included when targeting older shader models?
What about targeting SPIRV?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think it's fine to include the header when the shader is targeting older shader models. The user might have some preprocessor code to use these APIs if 6.9 is available, and to default to a fallback if not.
It is my understanding that whether we're targeting SPIRV or not doesn't matter, the builtins will be lowered to the correct IR regardless.

Copy link
Member

Choose a reason for hiding this comment

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

This is only supported for d3d / dxil (which is why everything is / will be in the dx namespace)

Copy link
Collaborator

Choose a reason for hiding this comment

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

The SPIRV path doesn't generate IR in DXC, it goes straight from the AST to SPIRV, which would likely cause the compiler either to crash or to emit a less-than helpful diagnostic.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe we could add something like this to the top of the file?

#if __spirv__
#error "Cooperative matrix not (yet) supported for SPIRV"
#endif

Copy link
Member

Choose a reason for hiding this comment

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

s/matrix/vector

// Header for linear algebra APIs.

#if __spirv__
#error "Cooperative matrix/vectors not (yet) supported for SPIRV"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
#error "Cooperative matrix/vectors not (yet) supported for SPIRV"
#error "Cooperative vectors not (yet) supported for SPIRV"

Sorry for my bad suggestion. Cooperative matrix actually is supported in SPIRV, they have a separate header for it.

// This is a copy of \tools\clang\test\CodeGenDXIL\hlsl\linalg\outerproductaccumulate.hlsl
// except that spirv is targeted

// expected-error@dx/linalg.h:4{{Cooperative matrix/vectors not (yet) supported for SPIRV}}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
// expected-error@dx/linalg.h:4{{Cooperative matrix/vectors not (yet) supported for SPIRV}}
// expected-error@dx/linalg.h:4{{Cooperative vectors not (yet) supported for SPIRV}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

This file doesn't seem to belong here.

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

Successfully merging this pull request may close these issues.

3 participants