-
Notifications
You must be signed in to change notification settings - Fork 828
[SM6.10] Add LinAng Matrix attributes and two new attributed matrix types #8125
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: user/hekota/pr8090-matrix-builtin-handle-type
Are you sure you want to change the base?
[SM6.10] Add LinAng Matrix attributes and two new attributed matrix types #8125
Conversation
|
|
||
| // If the attribute isn't known, we will not attempt to parse any | ||
| // arguments. | ||
| // if (!hasAttribute(AttrSyntax::CXX, ScopeName, AttrName, |
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.
Weren't any other CXX attributes accepted here before? I thought SPIR-V used [[vk::...]] CXX attributes. Or maybe those aren't C++11 attributes?
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.
Good point! This is the one and only C++11-style attribute we have on a type. I will check how the SPIR-V attributes are handled to make sure I do not break them.
| __builtin_LinAlg_Matrix [[__LinAlg_Matrix_Attributes(ComponentType::I32, 4, 5, MatrixUse::B, MatrixScope::ThreadGroup)]] mat8; | ||
|
|
||
| // expected-error@+1 {{cannot initialize a variable of type '__builtin_LinAlg_Matrix' with an lvalue of type '__builtin_LinAlg_Matrix [[__LinAlg_Matrix_Attributes(ComponentType::I32, 4, 5, MatrixUse::B, MatrixScope::ThreadGroup)]]'}} | ||
| __builtin_LinAlg_Matrix naked_mat = mat8; |
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.
Should it be illegal to declare this type without attributes? Is that the reason for TMP?
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 could make it illegal by intercepting every place where a type can be used and adding a special check and error if it is the __builtin_LinAlgMatrix without attributes. Or we can do nothing because the naked typed cannot be used for anything anyway. The __builtin prefix means users should not be using these directly.
|
|
||
| // Check that the value is a valid range. | ||
| int64_t Value = APVal.getLimitedValue(); | ||
| if (Value < 0) { |
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.
Should we enforce any maximum?
…://github.com/microsoft/DirectXShaderCompiler into linalg-matrix-attributes-and-types
V-FEXrt
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.
LGTM but I did a very rush job on the review. Wanted to mark my general approval before being out the next couple days
You can test this locally with the following command:git-clang-format --diff eea0b2b70ffcb65484aad2a25ac7948cab230b71 cd120b01dc247a3fb2a6dc293734fabd30098588 -- include/dxc/DXIL/DxilConstants.h include/dxc/dxcapi.internal.h tools/clang/include/clang/AST/ASTContext.h tools/clang/include/clang/AST/DataRecursiveASTVisitor.h tools/clang/include/clang/AST/RecursiveASTVisitor.h tools/clang/include/clang/AST/Type.h tools/clang/include/clang/AST/TypeLoc.h tools/clang/include/clang/Sema/SemaHLSL.h tools/clang/include/clang/Serialization/ASTBitCodes.h tools/clang/lib/AST/ASTContext.cpp tools/clang/lib/AST/ASTDumper.cpp tools/clang/lib/AST/ASTImporter.cpp tools/clang/lib/AST/ItaniumMangle.cpp tools/clang/lib/AST/MicrosoftMangle.cpp tools/clang/lib/AST/Type.cpp tools/clang/lib/AST/TypePrinter.cpp tools/clang/lib/Headers/hlsl/dx/linalg.h tools/clang/lib/Parse/ParseDecl.cpp tools/clang/lib/Parse/ParseDeclCXX.cpp tools/clang/lib/Sema/SemaHLSL.cpp tools/clang/lib/Sema/SemaTemplate.cpp tools/clang/lib/Sema/SemaType.cpp tools/clang/lib/Sema/TreeTransform.h tools/clang/lib/Serialization/ASTReader.cpp tools/clang/lib/Serialization/ASTWriter.cpp tools/clang/tools/libclang/CIndex.cppView the diff from clang-format here.diff --git a/tools/clang/include/clang/AST/Type.h b/tools/clang/include/clang/AST/Type.h
index a449b6b2..352045be 100644
--- a/tools/clang/include/clang/AST/Type.h
+++ b/tools/clang/include/clang/AST/Type.h
@@ -1708,7 +1708,7 @@ public:
const; // HLSL attributed __builtin_LinAlgMatrix with dependent
// parameters
// HLSL Change End
-
+
/// Determines if this type, which must satisfy
/// isObjCLifetimeType(), is implicitly __unsafe_unretained rather
/// than implicitly __strong.
|
Adds new type attribute
__LinAlg_Matrix_Attributesthat can be attached to__builtin_LinAlg_Matrixbuilt-in matrix handle type to specify the matrix component type, dimensions, use and scope. Includes enabling C11++ -style attributes on types in DXC.Adds two new AST types that are used to capture the information from the attribute:
AttributedLinAlgMatrixTypetype is used when the attribute specifies concrete matrix parametersDependentAttributedLinAlgMatrixTypeis for cases where the type attributes arguments are dependent on template instantiationThe
AttributedLinAlgMatrixTypeis linked toLinAlgMatrixtype alias used ingen_intrin_main.txtfor built-in function APIs.This change also adds the outline of the LinAlg
Matrixtemplate class todx/linalg.h, including the enumeration types used as arguments to the new attribute and as template parameters ofMatrix.Note that the
ComponentTypeenum indx/linalg.his defined using values matching the existing internalComponentTypeenum that was added in SM 6.9 for cooperative vectors. This is different from the current LinAlg spec. The issue to update the spec is microsoft/hlsl-specs#779.Fixes #8122