-
Notifications
You must be signed in to change notification settings - Fork 813
[SM6.10][LinAlg] Impl MatrixRef, CreateMatrix builtins #7883
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
base: main
Are you sure you want to change the base?
Conversation
| @@ -0,0 +1,8 @@ | |||
| // RUN: %dxc -T cs_6_9 -E main %s -verify | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty sure I also want a .ll test but I don't know the magic incantation to generate the IR upto but right before the dxilgen pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
./utils/hct/ExtractIRForPassTest.py -p dxilgen -o output.ll input.hlsl -- -T cs_6_9 -E main
damyanp
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've read through this and didn't spot any obvious typos.
Do we need to apply the same "experimental DXIL" stuff to this that we're talking about for other SM 6.10 features?
| LICOMPTYPE_VK_BUFFER_POINTER = 55, | ||
| LICOMPTYPE_COUNT = 56 | ||
| #else | ||
| LICOMPTYPE_COUNT = 54 | ||
| LICOMPTYPE_COUNT = 55 | ||
| #endif |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it matter that these values have changed?
I've no reason to think it does, just checking.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm assuming no but maybe someone else can weigh in. The ifdef basically forces them to change though since the last number is only incremented for SPIRV. Beyond that this seems to be an internal enum so changing it shouldn't be observable to end users.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are mostly internal details, but the built-in intrinsic extension mechanism relies on this header (and this enum) as well. That's currently only used by Xbox backend, and wouldn't be impacted by these changes unless updated to depend on them, and even then, using the latest header would keep it in sync.
Ideally, I think we should get rid of the ifdef here (so LICOMPTYPE_VK_BUFFER_POINTER is always part of the enumeration), and update the corresponding table g_LegalIntrinsicCompTypes in SemaHLSL.cpp. But that should be a separate change.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A lot of the ifdef SPIRV conditionals were removed in similar areas. This one was missed possibly as an oversight.
Yep |
|
In a discussion about a month ago, we agreed that createMatrix as written wasn't useful because it has no way of uniquely identifying the matrix in question, so any and all createMatrix calls will get merged as duplicates. From this discussion, we agreed to remove it, which would make the builtin unnecessary. In my own further investigation that I've been trying to get the opportunity to discuss for weeks, I've determined that we may need a createMatrix that is generated for local or global allocas, but not parameters since, without global variables, we have to use the alloca as the unique identifier and the fact that they are opaque handles makes it difficult to cleanly merge duplicate parameter allocas through inlining and subsequent passes. In any event, a creatematrix call that only takes the opcode isn't of much use. |
|
Per our discussion, you can disregard my last comment, however I just remembered that when I proposed a builtin for creatematrix and annotatematrix, @llvm-beanz rejected it on account of it would allow the header implementation or other method using builtins to generate invalid dxil. I found that a compelling argument. I don't know if he changed his mind since. Similar create dxil ops are generated as part of clang CodeGen. As an example I wouldn't strongly recommend following exactly, but as the broad idea, see how resource handle creation is generated:
The way nodehandle creation is generated is the way I would/have approached it though it will be through local/global variables instead of entry function params: DirectXShaderCompiler/tools/clang/lib/CodeGen/CGHLSLMS.cpp Lines 3852 to 3853 in 6679358
|
I think a builtin for The annotate builtin is a bit more complicated but it isn't the "creating invalid dxil" that concerns me as much as I'm unsure that the annotate builtin could be used to create valid DXIL. I'm not strongly opposed to either of these additions a lot of it just depends on the implementation details as we progress. |
Are you saying that create will have a builtin, but annotate will not? As a sanity check, where do you see the create builtin being called in the linalg.h header? |
Currently the plan is that they both have builtins, and the annotate is just overemitted and then cleaned up/deduped in a late pass
Create and initial annotate are in the constructor of the |
Fixes #7846
Implments:
__builtin_la_MatrixRef__builtin_la_CreateMatrix()%dx.types.MatrixRef@dx.op.createMatrix(i32 312)Note: The DXIL op codes will change in response to microsoft/hlsl-specs#698