-
Notifications
You must be signed in to change notification settings - Fork 15.2k
Enable matrices in HLSL #111415
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
Enable matrices in HLSL #111415
Changes from 2 commits
f4751fc
1fa442b
58d5186
3f53681
ae1a58b
eeb3165
75f19e4
0531fc9
fa26735
6ef5a91
de15228
fa5b05a
1e65231
767446a
d9ac6e7
cbf13a0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -852,34 +852,50 @@ void TypePrinter::printExtVectorAfter(const ExtVectorType *T, raw_ostream &OS) { | |
|
|
||
| void TypePrinter::printConstantMatrixBefore(const ConstantMatrixType *T, | ||
| raw_ostream &OS) { | ||
| if (Policy.UseHLSLTypes) | ||
| OS << "matrix<"; | ||
| printBefore(T->getElementType(), OS); | ||
| OS << " __attribute__((matrix_type("; | ||
| OS << T->getNumRows() << ", " << T->getNumColumns(); | ||
| OS << ")))"; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This change seems wrong to me. IIUC this changes something like
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See my response to Florian here
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. may be simpler to read to duplicate some code but have the HSL and C++ matrixes types printed completely separately? |
||
| } | ||
|
|
||
| void TypePrinter::printConstantMatrixAfter(const ConstantMatrixType *T, | ||
| raw_ostream &OS) { | ||
| printAfter(T->getElementType(), OS); | ||
| if (Policy.UseHLSLTypes) { | ||
| OS << ", "; | ||
| OS << T->getNumRows() << ", " << T->getNumColumns(); | ||
| OS << ">"; | ||
| } else { | ||
| OS << " __attribute__((matrix_type("; | ||
| OS << T->getNumRows() << ", " << T->getNumColumns(); | ||
| OS << ")))"; | ||
| } | ||
| } | ||
|
|
||
| void TypePrinter::printDependentSizedMatrixBefore( | ||
| const DependentSizedMatrixType *T, raw_ostream &OS) { | ||
| if (Policy.UseHLSLTypes) | ||
| OS << "matrix<"; | ||
| printBefore(T->getElementType(), OS); | ||
| OS << " __attribute__((matrix_type("; | ||
| if (T->getRowExpr()) { | ||
| T->getRowExpr()->printPretty(OS, nullptr, Policy); | ||
| } | ||
| OS << ", "; | ||
| if (T->getColumnExpr()) { | ||
| T->getColumnExpr()->printPretty(OS, nullptr, Policy); | ||
| } | ||
| OS << ")))"; | ||
| } | ||
|
|
||
| void TypePrinter::printDependentSizedMatrixAfter( | ||
| const DependentSizedMatrixType *T, raw_ostream &OS) { | ||
| printAfter(T->getElementType(), OS); | ||
| if (Policy.UseHLSLTypes) | ||
| OS << ", "; | ||
| else | ||
| OS << " __attribute__((matrix_type("; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You still have this in the "after" function. That will print this after pointer annotations which changes its meaning incorrectly. I'm guessing there isn't an existing test case for this, otherwise this change would have broken something. cc: @fhahn
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I guess I missed this one. AFAICT, DependentSizedMatrices are never printed. |
||
|
|
||
| if (Expr *E = T->getRowExpr()) | ||
| E->printPretty(OS, nullptr, Policy); | ||
| OS << ", "; | ||
| if (Expr *E = T->getColumnExpr()) | ||
| E->printPretty(OS, nullptr, Policy); | ||
|
|
||
| if (Policy.UseHLSLTypes) | ||
| OS << ">"; | ||
| else | ||
| OS << ")))"; | ||
|
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The changes to this file do alter how pointer and reference matrices are printed. I'd like to get @fhahn's opinion on this even if on nothing else. |
||
| } | ||
|
|
||
| void | ||
|
|
||
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.
If this is the only line this will be needed then feel free to ignore this comment.
My question is could there be places where
LangOpts.MatrixTypescould diverge from HLSL?If so instead is it possible for
LangOpts.HLSLto turn onLangOpts.MatrixTypes?Uh oh!
There was an error while loading. Please reload this page.
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.
We can add
MatrixTypesto the definition of HLSL's variants in LangStandards.def, which is probably a safer approach to this.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'll look into that. I didn't want to enable matrices by default because I didn't really want to allow the matrix attribute syntax in HLSL.
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 done this two ways now:
ae1a58b just enables matrices when HLSL is enabled
eeb3165 attempts to use LangStandards and make it dependent on HLSL versions
I confess I don't know why the second is preferable to the first. If I've misunderstood @llvm-beanz's suggestion, please let me know.
I'm actually unclear on how any of this prevents the problem @farzonl brought up. If what MatrixTypes represents changes so that it no longer matches what HLSL wants, then those changes will seep in without our notice. Given that the design discussion we had about this rejected the notion that we'd forge our own matrix type that inherited from the clang type, I don't see any way around that problem unless we pay attention to its evolution and weigh in when needed. Not that I'm saying that's a bad idea regardless.
All that the above changes alter is that now matrices can be declared using the clang extension which will sidestep at least the size restrictions we have not yet applied, but intend to.
I'm unconvinced this is an improvement, but it was easy enough to do that I thought I'd give us more concrete options to discuss.