-
Notifications
You must be signed in to change notification settings - Fork 12k
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
[HLSL] Add implicit resource element type concepts to AST #112600
Conversation
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.
Some comments based on a quick read through:
@@ -356,6 +426,9 @@ struct TemplateParameterListBuilder { | |||
QualType T = Builder.Template->getInjectedClassNameSpecialization(); | |||
T = S.Context.getInjectedClassNameType(Builder.Record, T); | |||
|
|||
ArrayRef<TemplateArgument> TempArgs = | |||
Builder.Template->getInjectedTemplateArgs(); |
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.
Unused variable TempArgs
?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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); |
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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}); |
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.
Some more notes
✅ With the latest revision this PR passed the C/C++ code formatter. |
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. Would be good to get review from someone more familiar with ASTs and external sema source.
…emplatetypeparmdecl
13c6ba0
to
d770236
Compare
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.
Mostly looking good to me. A few small comments.
|
||
// first get the "sizeof(T) <= 16" expression, as a binary operator | ||
BinaryOperator *SizeOfLEQ16 = constructSizeOfLEQ16Expr(Context, NameLoc, T); | ||
// TODO: add the '__builtin_hlsl_is_line_vector_layout_compatible' builtin |
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 think we had agreed on a different name for this. Also is there an issue filed that you can reference in this TODO?
} | ||
|
||
ConceptDecl *constructTypedBufferConceptDecl(Sema &S) { | ||
DeclContext *DC = S.CurContext; |
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.
Do we want this to be Sema's current context, or the HLSL namespace context? My guess is the later.
It probably doesn't matter too much, but it would be nice to make sure we're not injecting AST nodes into the top-level declaration context or a user-defined context.
SourceLocation DeclLoc = SourceLocation(); | ||
|
||
IdentifierInfo &IsValidLineVectorII = | ||
Context.Idents.get("is_valid_line_vector"); |
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 think we also had a different name for this...
Can you also prefix the name with double underscore (__
) since it is an implementation detail in the compiler rather than an intentionally user-surfaced language feature?
Since your PR that adds |
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. |
void HLSLExternalSemaSource::defineHLSLTypesWithForwardDeclarations() { | ||
CXXRecordDecl *Decl; | ||
ConceptDecl *TypeBufferConcept = |
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.
Few minor things, otherwise LGTM!
concept is_valid_line_vector =sizeof(T) <= 16; | ||
template<typename element_type> requires is_valid_line_vector<element_type> |
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.
); | ||
|
||
T->setDeclContext(DC); | ||
T->setReferenced(); |
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.
IdentifierInfo &IsTypedResourceElementCompatibleII = | ||
Context.Idents.get("__is_typed_resource_element_compatible"); |
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.
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.
Some notes, mostly about the comments.
template<typename element_type> requires | ||
is_typed_resource_element_compatible<element_type> | ||
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.
The blank line between the template bit and the struct really confused me for a while trying to read this.
is_typed_resource_element_compatible<element_type> | ||
struct RWBuffer { | ||
element_type Val; |
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 don't think that the code in this comment really matches the AST that's being built.
RWBuffer doesn't have a member of type element_type, for example.
I think that this function is just building up the AST that corresponds to the requires is_typed_resource_element_compatible<element_Type>
part?
The AST nodes for template<typename T> concept is_typed_resource_element_compatible =sizeof(T) <= 16;
itself is created in constructTypedBufferConceptDecl?
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 agree that when parsing the C++ code in the comment, it produces more of the AST than the function is producing, but I still believe that including that extra context in the comments is helpful. The C++ code adds the structure "RWBuffer", and though it isn't being produced by constructConceptSpecializationExpr
, it helps to know what code can be copy pasted into godbolt, for example, to see the AST that would be produced.
I had originally wanted to paste the AST that would be produced, but figured getting the source code would help explain the code better and also allow those who are interested to get the AST from the code.
The first point of the comment says that
"The concept specialization expression (CSE) constructed below is constructed
so that it matches the CSE that is constructed when parsing
the below C++ code:"
Which is still accurate. The code in the function isn't claiming to be responsible for the whole AST. I will reword it slightly for more clarity.
For your last 2 questions, yes I think your statements are accurate.
|
||
QualType ConceptTType = Context.getTypeDeclType(ConceptTTPD); | ||
|
||
// this is the 2nd template argument node in the AST above |
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!
// 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's fake about it?
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
// to construct the ImplicitConceptSpecializationDecl | ||
TemplateTypeParmDecl *T = TemplateTypeParmDecl::Create( | ||
Context, // AST context | ||
Context.getTranslationUnitDecl(), // DeclContext |
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 is where the DeclContext is wrong that I referenced in https://github.com/llvm/llvm-project/pull/112600/files/6edf031b5e736b38cf3551ccc872351f9c8a07ca#r1835111011
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.
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, |
|
||
// Create a ConceptDecl | ||
ConceptDecl *CD = | ||
ConceptDecl::Create(Context, Context.getTranslationUnitDecl(), DeclLoc, |
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, |
CD->setTemplateParameters(ConceptParams); | ||
|
||
// Add the concept declaration to the Translation Unit Decl | ||
Context.getTranslationUnitDecl()->addDecl(CD); |
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); |
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
|
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
|
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.