[SM6.10][LinAlg] Impl MatrixRef, CreateMatrix builtins #7883
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1306,6 +1306,48 @@ CXXRecordDecl *hlsl::DeclareHitObjectType(NamespaceDecl &NSDecl) { | |
| return RecordDecl; | ||
| } | ||
|
|
||
| CXXRecordDecl *hlsl::DeclareMatrixRefType(ASTContext &Context) { | ||
| // MatrixRef { ... } | ||
| BuiltinTypeDeclBuilder TypeDeclBuilder(Context.getTranslationUnitDecl(), | ||
| "__builtin_la_MatrixRef"); | ||
|
Collaborator
There was a problem hiding this comment. I overlooked this before, without template arguments, I don't know where you will get the property information that the matrix provides as it is solely stored in the header-defined type that contains this type
Collaborator
There was a problem hiding this comment. Though I guess this relates to my previous question of whether annotateMatrix is expected to be called explicitly from HLSL. in that case, the type information from the header would be available, but that's not available for user function parameters.
Collaborator
Author
There was a problem hiding this comment. The property information would be tracked by the templated header class and forwarded along to annotate as needed
Collaborator
There was a problem hiding this comment. Do you have that working in a branch somewhere? I just don't see how that information would be available to clang in a user function needing an annotate without adding a dependency at the compiler implementation level to a class written in an HLSL header, which seems like a bad idea.
Collaborator
Author
There was a problem hiding this comment. I don't no. I was planning on getting deep into things once I got back but this whole discussion preempted that :) I'm not sure where you see a potential compiler dependency on the class though, this works (although optimization seems to be deleting everything) without any special knowledge from the compiler and it is pretty much a strawman of what I plan to do. https://godbolt.org/z/dnYfn7eTv
Collaborator
There was a problem hiding this comment. Thanks for that. That's what I suspected. That works fine for methods of the matrix class defined in the header, but what about user functions that pass matrices around? They shouldn't pass the MatrixRef around, but there shouldn't be any reason they can't pass the dx::linalg::matrix class
Collaborator
Author
There was a problem hiding this comment. Do you have specific user functions in mind? The whole point of dx::linalg::Matrix is to restrict the matrix usage down to only the allowed operations. Anything that a user would want to do with the MatrixRef should be accessable via the header API
Collaborator
There was a problem hiding this comment. I'm not referring to passing MatrixRef into a function, but MatrixRef will be a member of dx::linalg::Matrix and passing it around as a member doesn't change the need for an annotate. Disallowing shader authors to pass dx::linalg::Matrix to their own functions would be an astonishing and frustrating restriction. That said, in the meeting wednesday, @tex3d suggested that if the MatrixRef sees any use in the user function, it would be through the header calls, so annotates placed there will handle it rather similarly in outcome to how the annotateHandles are generated for resources. I find that convincing. All this could be rendered irrelevant depending on @llvm-beanz's unreleased proposal |
||
| TypeDeclBuilder.startDefinition(); | ||
|
|
||
| // Add handle to mark as HLSL object. | ||
| TypeDeclBuilder.addField("h", GetHLSLObjectHandleType(Context)); | ||
| CXXRecordDecl *RecordDecl = TypeDeclBuilder.getRecordDecl(); | ||
|
|
||
| CanQualType canQualType = Context.getCanonicalType( | ||
| Context.getRecordType(TypeDeclBuilder.getRecordDecl())); | ||
|
|
||
| // Add constructor that will be lowered to IOP___builtin_la_CreateMatrix. | ||
| CXXConstructorDecl *pConstructorDecl = nullptr; | ||
| TypeSourceInfo *pTypeSourceInfo = nullptr; | ||
| CreateConstructorDeclaration( | ||
| Context, RecordDecl, Context.VoidTy, {}, | ||
| Context.DeclarationNames.getCXXConstructorName(canQualType), false, | ||
| &pConstructorDecl, &pTypeSourceInfo); | ||
| RecordDecl->addDecl(pConstructorDecl); | ||
| pConstructorDecl->addAttr(HLSLIntrinsicAttr::CreateImplicit( | ||
| Context, "op", "", | ||
| static_cast<int>(hlsl::IntrinsicOp::IOP___builtin_la_CreateMatrix))); | ||
| pConstructorDecl->addAttr(HLSLCXXOverloadAttr::CreateImplicit(Context)); | ||
|
Comment on lines
+1322
to
+1333
Collaborator
There was a problem hiding this comment. Just for clarity, this is the MatrixRef constructor I mentioned before that it seems there is now opposition to on account of it being inexpressible in HLSL
Collaborator
Author
There was a problem hiding this comment. I see, FWIW I was blindly copying a lot of this for the first time when I first made this PR. Because of this there were some things that fell into the "I don't know if this should be here but I don't have the familiarly yet to make that decision, but review should catch it if its a surprise" While I was trying my best to understand stuff as I was writing it there was a lot of code to managed and I suppose I didn't internalize that this was creating a capital C constructor. Thanks for calling this bit of code and I am sorry for causing confusion with it. Please do call anything else out in the PR that looks strange or surprising :) |
||
|
|
||
| // Add AvailabilityAttribute for SM6.10+ | ||
| VersionTuple VT610 = VersionTuple(6, 10); | ||
| AvailabilityAttr *AAttr = AvailabilityAttr::CreateImplicit( | ||
| Context, &Context.Idents.get(""), clang::VersionTuple(6, 10), | ||
| clang::VersionTuple(), clang::VersionTuple(), false, ""); | ||
| RecordDecl->addAttr(AAttr); | ||
|
|
||
| // Add the implicit HLSLMatrixRefAttr attribute to unambiguously recognize the | ||
| // builtin HitObject type. | ||
| RecordDecl->addAttr(HLSLMatrixRefAttr::CreateImplicit(Context)); | ||
| RecordDecl->setImplicit(true); | ||
|
|
||
| // Add to namespace | ||
| return RecordDecl; | ||
| } | ||
|
|
||
| CXXRecordDecl *hlsl::DeclareResourceType(ASTContext &context, bool bSampler) { | ||
| // struct ResourceDescriptor { uint8 desc; } | ||
| StringRef Name = bSampler ? ".Sampler" : ".Resource"; | ||
|
|
||
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
ifdefbasically 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_POINTERis always part of the enumeration), and update the corresponding tableg_LegalIntrinsicCompTypesinSemaHLSL.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.