-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[HLSL] Add implicit resource element type concepts to AST #112600
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
Changes from 21 commits
ab60544
b43675f
4307e45
4beb3a3
98200c0
f70fb48
54917b1
6ebe14a
1ecd544
19664f5
ea0ac08
d220a73
8014a47
8516483
defc84e
b373f15
4fecdc4
80d2d25
d770236
1159b02
cf1a7fd
17696d8
6edf031
fae51c5
1722578
f50b917
47f106b
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 | ||||
|---|---|---|---|---|---|---|
|
|
@@ -300,8 +300,9 @@ struct BuiltinTypeDeclBuilder { | |||||
| } | ||||||
|
|
||||||
| TemplateParameterListBuilder addTemplateArgumentList(Sema &S); | ||||||
| BuiltinTypeDeclBuilder &addSimpleTemplateParams(Sema &S, | ||||||
| ArrayRef<StringRef> Names); | ||||||
| BuiltinTypeDeclBuilder & | ||||||
| addSimpleTemplateParams(Sema &S, ArrayRef<StringRef> Names, ConceptDecl *CD); | ||||||
| BuiltinTypeDeclBuilder &addConceptSpecializationExpr(Sema &S); | ||||||
| }; | ||||||
|
|
||||||
| struct TemplateParameterListBuilder { | ||||||
|
|
@@ -323,30 +324,127 @@ struct TemplateParameterListBuilder { | |||||
| S.Context, Builder.Record->getDeclContext(), SourceLocation(), | ||||||
| SourceLocation(), /* TemplateDepth */ 0, Position, | ||||||
| &S.Context.Idents.get(Name, tok::TokenKind::identifier), | ||||||
| /* Typename */ false, | ||||||
| /* ParameterPack */ false); | ||||||
| /* Typename */ true, | ||||||
| /* ParameterPack */ false, | ||||||
| /* HasTypeConstraint*/ false); | ||||||
| if (!DefaultValue.isNull()) | ||||||
| Decl->setDefaultArgument( | ||||||
| S.Context, S.getTrivialTemplateArgumentLoc(DefaultValue, QualType(), | ||||||
| SourceLocation())); | ||||||
|
|
||||||
| Params.emplace_back(Decl); | ||||||
| return *this; | ||||||
| } | ||||||
|
|
||||||
| BuiltinTypeDeclBuilder &finalizeTemplateArgs() { | ||||||
| /* | ||||||
| The concept specialization expression (CSE) constructed below is constructed | ||||||
| so that it matches the CSE that is constructed when parsing | ||||||
| the below C++ code: | ||||||
|
|
||||||
| template<typename T> | ||||||
| concept is_valid_line_vector =sizeof(T) <= 16; | ||||||
|
|
||||||
| template<typename element_type> requires is_valid_line_vector<element_type> | ||||||
|
|
||||||
|
||||||
| struct RWBuffer { | ||||||
| element_type Val; | ||||||
bob80905 marked this conversation as resolved.
Show resolved
Hide resolved
|
||||||
| }; | ||||||
|
|
||||||
| int fn() { | ||||||
| RWBuffer<int> Buf; | ||||||
| } | ||||||
|
|
||||||
| When dumping the AST and filtering for "RWBuffer", the resulting AST | ||||||
| structure is what we're trying to construct below, specifically the | ||||||
| CSE portion. | ||||||
| */ | ||||||
| ConceptSpecializationExpr * | ||||||
| constructConceptSpecializationExpr(Sema &S, ConceptDecl *CD) { | ||||||
| ASTContext &Context = S.getASTContext(); | ||||||
| SourceLocation Loc = Builder.Record->getBeginLoc(); | ||||||
| DeclarationNameInfo DNI(CD->getDeclName(), Loc); | ||||||
| NestedNameSpecifierLoc NNSLoc; | ||||||
| DeclContext *DC = Builder.Record->getDeclContext(); | ||||||
| TemplateArgumentListInfo TALI(Loc, Loc); | ||||||
|
|
||||||
| // Assume that the concept decl has just one template parameter | ||||||
| // This parameter should have been added when CD was constructed | ||||||
| // in getTypedBufferConceptDecl | ||||||
| assert(CD->getTemplateParameters()->size() == 1 && | ||||||
| "unexpected concept decl parameter count"); | ||||||
| TemplateTypeParmDecl *ConceptTTPD = dyn_cast<TemplateTypeParmDecl>( | ||||||
| CD->getTemplateParameters()->getParam(0)); | ||||||
|
|
||||||
| // this fake TemplateTypeParmDecl is used to construct a template argument | ||||||
| // that will be used to construct the ImplicitConceptSpecializationDecl | ||||||
|
||||||
| TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create( | ||||||
| Context, // AST context | ||||||
| Context.getTranslationUnitDecl(), // DeclContext | ||||||
|
||||||
| Context.getTranslationUnitDecl(), // DeclContext | |
| Builder.Record->getDeclContext();, // DeclContext |
Outdated
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.
Why do you need to set this explicitly?
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.
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.
Outdated
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 not entirely sure what "the AST above" refers to.
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.
Ah, this was when I had the entire AST inside a comment previously, haven't updated it, will fix!
Outdated
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.
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.
Outdated
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 this is also wrong here:
| Context, Context.getTranslationUnitDecl(), DeclLoc, DeclLoc, | |
| Context, NSD->getDeclContext(), DeclLoc, DeclLoc, |
Outdated
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.
| ConceptDecl::Create(Context, Context.getTranslationUnitDecl(), DeclLoc, | |
| ConceptDecl::Create(Context, NSD->getDeclContext(), DeclLoc, |
Outdated
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 should be putting the concepts under the hlsl namespace not under the top level declaration where they may conflict with user-defined declarations.
| Context.getTranslationUnitDecl()->addDecl(CD); | |
| NSD->getDeclContext()->addDecl(CD); |
Outdated
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.
Typo
| ConceptDecl *TypeBufferConcept = | |
| ConceptDecl *TypedBufferConcept = |
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.
This still uses the old name.