[HLSL] Add implicit resource element type concepts to AST #112600
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
damyanp
reviewed
Oct 17, 2024
pow2clk
reviewed
Oct 17, 2024
| @@ -356,6 +426,9 @@ struct TemplateParameterListBuilder { | |||
| QualType T = Builder.Template->getInjectedClassNameSpecialization(); | |||
| T = S.Context.getInjectedClassNameType(Builder.Record, T); | |||
|
|
|||
| ArrayRef<TemplateArgument> TempArgs = | |||
| Builder.Template->getInjectedTemplateArgs(); | |||
llvm-beanz
reviewed
Oct 17, 2024
Comment on lines
628
to
633
| std::vector<NamedDecl *> TemplateParamsVec = {T}; | ||
| llvm::ArrayRef<NamedDecl *> TemplateParams(TemplateParamsVec); | ||
|
|
||
| clang::TemplateParameterList *ConceptParams = | ||
| clang::TemplateParameterList::Create(context, DeclLoc, DeclLoc, | ||
| TemplateParams, DeclLoc, nullptr); |
There was a problem hiding this comment.
Suggested change
| std::vector<NamedDecl *> TemplateParamsVec = {T}; | |
| llvm::ArrayRef<NamedDecl *> TemplateParams(TemplateParamsVec); | |
| clang::TemplateParameterList *ConceptParams = | |
| clang::TemplateParameterList::Create(context, DeclLoc, DeclLoc, | |
| TemplateParams, DeclLoc, nullptr); | |
| clang::TemplateParameterList *ConceptParams = | |
| clang::TemplateParameterList::Create(context, DeclLoc, DeclLoc, | |
| {T}, DeclLoc, nullptr); |
llvm-beanz
reviewed
Oct 17, 2024
Comment on lines
371
to
385
| std::vector<TemplateArgument> ConceptConvertedArgsVec = {ConceptTA}; | ||
| ArrayRef<TemplateArgument> ConceptConvertedArgs = ConceptConvertedArgsVec; | ||
|
|
||
| clang::QualType CSETType = context.getTypeDeclType(T); | ||
|
|
||
| TemplateArgument CSETA = TemplateArgument(CSETType); | ||
|
|
||
| std::vector<TemplateArgument> CSEConvertedArgsVec = {CSETA}; | ||
| ArrayRef<TemplateArgument> CSEConvertedArgs = CSEConvertedArgsVec; | ||
|
|
||
| ImplicitConceptSpecializationDecl *ImplicitCSEDecl = | ||
| ImplicitConceptSpecializationDecl::Create( | ||
| context, Builder.Record->getDeclContext(), loc, CSEConvertedArgs); | ||
|
|
||
| const ConstraintSatisfaction CS(CD, ConceptConvertedArgs); |
There was a problem hiding this comment.
Suggested change
| std::vector<TemplateArgument> ConceptConvertedArgsVec = {ConceptTA}; | |
| ArrayRef<TemplateArgument> ConceptConvertedArgs = ConceptConvertedArgsVec; | |
| clang::QualType CSETType = context.getTypeDeclType(T); | |
| TemplateArgument CSETA = TemplateArgument(CSETType); | |
| std::vector<TemplateArgument> CSEConvertedArgsVec = {CSETA}; | |
| ArrayRef<TemplateArgument> CSEConvertedArgs = CSEConvertedArgsVec; | |
| ImplicitConceptSpecializationDecl *ImplicitCSEDecl = | |
| ImplicitConceptSpecializationDecl::Create( | |
| context, Builder.Record->getDeclContext(), loc, CSEConvertedArgs); | |
| const ConstraintSatisfaction CS(CD, ConceptConvertedArgs); | |
| clang::QualType CSETType = context.getTypeDeclType(T); | |
| TemplateArgument CSETA = TemplateArgument(CSETType); | |
| ImplicitConceptSpecializationDecl *ImplicitCSEDecl = | |
| ImplicitConceptSpecializationDecl::Create( | |
| context, Builder.Record->getDeclContext(), loc, {CSETA}); | |
| const ConstraintSatisfaction CS(CD, {ConceptTA}); |
damyanp
reviewed
Oct 18, 2024
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
damyanp
reviewed
Oct 18, 2024
…emplatetypeparmdecl
bob80905
force-pushed
the
add_concepts_to_ast
branch
from
13c6ba0 to
d770236
Compare
I'd prefer not to, because this PR is already pretty big and I have a separate task that is singularly dedicated to finalizing the constraint expression, which will include incorporating the new builtin. |
hekota
reviewed
Nov 5, 2024
| void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() { | ||
| CXXRecordDecl *Decl; | ||
| ConceptDecl *TypeBufferConcept = |
There was a problem hiding this comment.
Typo
Suggested change
| ConceptDecl *TypeBufferConcept = | |
| ConceptDecl *TypedBufferConcept = |
hekota
approved these changes
Nov 6, 2024
Comment on lines
344
to
346
| concept is_valid_line_vector =sizeof(T) <= 16; | ||
|
|
||
| template<typename element_type> requires is_valid_line_vector<element_type> |
| ); | ||
|
|
||
| T->setDeclContext(DC); | ||
| T->setReferenced(); |
There was a problem hiding this comment.
Why do you need to set this explicitly?
There was a problem hiding this comment.
Setting the decl context is necessary because it puts the decl into the right indentation in the AST dump. Otherwise, I believe the decl would be placed at the same scope as one indentation below the translation unit decl, which is not where the decl belongs.
I'll see if I can get away with removing setReferenced here.
Comment on lines
639
to
640
| IdentifierInfo &IsTypedResourceElementCompatibleII = | ||
| Context.Idents.get("__is_typed_resource_element_compatible"); |
There was a problem hiding this comment.
Nit - move this just before DeclName declaration, or better yet use Context.Idents.get("__is_typed_resource_element_compatible") directly in the DeclarationName constructor.
It makes it easier to read when variables are declared closer to where they are is used.
damyanp
reviewed
Nov 7, 2024
|
|
||
| template<typename element_type> requires | ||
| is_typed_resource_element_compatible<element_type> | ||
|
|
There was a problem hiding this comment.
The blank line between the template bit and the struct really confused me for a while trying to read this.
|
|
||
| QualType ConceptTType = Context.getTypeDeclType(ConceptTTPD); | ||
|
|
||
| // this is the 2nd template argument node in the AST above |
There was a problem hiding this comment.
I'm not entirely sure what "the AST above" refers to.
There was a problem hiding this comment.
Ah, this was when I had the entire AST inside a comment previously, haven't updated it, will fix!
Comment on lines
378
to
379
| // this fake TemplateTypeParmDecl is used to construct a template argument | ||
| // that will be used to construct the ImplicitConceptSpecializationDecl |
There was a problem hiding this comment.
It isn't being placed into the AST, the sole reason for its existence is to allow a path to construct a TemplateArgument, and that TemplateArgument will actually be placed into the AST. The fake TemplateTypeParmDecl is otherwise unused and discarded.
There was a problem hiding this comment.
This sounds wrong... Like, completely wrong. You're absolutely adding it to the AST, that's what happens when you create a declaration and put it into a declaration context.
This also seems like we're putting this declaration into the wrong declaration context.
I don't see any of your tests verifying the AST shape of the concept declaration. Can you add tests for that so that we can see what the concept's AST looks like?
There was a problem hiding this comment.
On further investigation, yes I believe it is being placed into the AST. I didn't realize that setting the DeclContext correlated with AST placement. However, comparing the generated AST with similar C++ code, I do think the declaration context for this template type parm decl is correct. (That is, the context is the ClassTemplateDecl, and it shows up directly indented under the ClassTemplateDecl in the AST.
I've added tests for the concept declaration AST.
llvm-beanz
reviewed
Nov 13, 2024
| // to construct the ImplicitConceptSpecializationDecl | ||
| TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create( | ||
| Context, // AST context | ||
| Context.getTranslationUnitDecl(), // DeclContext |
There was a problem hiding this comment.
This is where the DeclContext is wrong that I referenced in https://github.com/llvm/llvm-project/pull/112600/files/6edf031b5e736b38cf3551ccc872351f9c8a07ca#r1835111011
Suggested change
| Context.getTranslationUnitDecl(), // DeclContext | |
| Builder.Record->getDeclContext();, // DeclContext |
|
|
||
| IdentifierInfo &ElementTypeII = Context.Idents.get("element_type"); | ||
| TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create( | ||
| Context, Context.getTranslationUnitDecl(), DeclLoc, DeclLoc, |
There was a problem hiding this comment.
Pretty sure this is also wrong here:
Suggested change
| Context, Context.getTranslationUnitDecl(), DeclLoc, DeclLoc, | |
| Context, NSD->getDeclContext(), DeclLoc, DeclLoc, |
|
|
||
| // Create a ConceptDecl | ||
| ConceptDecl *CD = | ||
| ConceptDecl::Create(Context, Context.getTranslationUnitDecl(), DeclLoc, |
There was a problem hiding this comment.
Suggested change
| ConceptDecl::Create(Context, Context.getTranslationUnitDecl(), DeclLoc, | |
| ConceptDecl::Create(Context, NSD->getDeclContext(), DeclLoc, |
| CD->setTemplateParameters(ConceptParams); | ||
|
|
||
| // Add the concept declaration to the Translation Unit Decl | ||
| Context.getTranslationUnitDecl()->addDecl(CD); |
There was a problem hiding this comment.
We should be putting the concepts under the hlsl namespace not under the top level declaration where they may conflict with user-defined declarations.
Suggested change
| Context.getTranslationUnitDecl()->addDecl(CD); | |
| NSD->getDeclContext()->addDecl(CD); |
llvm-beanz
approved these changes
Nov 15, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/144/builds/11651 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/12/builds/9628 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/140/builds/10923 Here is the relevant piece of the build log for the reference |
bob80905
added a commit
that referenced
this pull request
Nov 15, 2024
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/133/builds/6845 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/11/builds/8210 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/190/builds/9516 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/153/builds/14719 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/60/builds/12759 Here is the relevant piece of the build log for the reference |
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/56/builds/12264 Here is the relevant piece of the build log for the reference |
bob80905
added a commit
that referenced
this pull request
Nov 22, 2024
This PR is step one on the journey to implement resource element type validation via C++20 concepts. The PR sets up the infrastructure for injecting implicit concept decls / concept specialization expressions into the AST, which will then be evaluated after template arguments are instantiated. This is not meant to be a complete implementation of the desired validation for HLSL, there are a couple of missing elements: We need the __builtin_hlsl_is_typed_resource_element_compatible builtin to be implemented. We need other constraints, like is_intangible We need to put the first 2 points together, and construct a finalized constraint expression, which should differ between typed and raw buffers This is just an initial PR that puts some of the core infrastructure in place. This PR is an edit of #112600, so that new tests that were put into main don't fail Fixes #75676
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR is step one on the journey to implement resource element type validation via C++20 concepts. The PR sets up the infrastructure for injecting implicit concept decls / concept specialization expressions into the AST, which will then be evaluated after template arguments are instantiated. This is not meant to be a complete implementation of the desired validation for HLSL,
there are a couple of missing elements:
This is just an initial PR that puts some of the core infrastructure in place.