-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[HLSL][RootSignature] Define and integrate rootsig clang attr and decl #137690
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 all commits
87680ba
17694ec
0990882
8c693f0
5b05ef6
141c732
6e842d7
a0fd705
80609f4
d42f270
d809806
45b2d92
403dbf8
bf8c671
a16dde1
5d39779
1b712ca
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 |
---|---|---|
|
@@ -2,6 +2,7 @@ set(LLVM_LINK_COMPONENTS | |
BinaryFormat | ||
Core | ||
FrontendOpenMP | ||
FrontendHLSL | ||
Support | ||
TargetParser | ||
) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,12 @@ | |
#include "clang/Basic/TargetInfo.h" | ||
#include "clang/Basic/TokenKinds.h" | ||
#include "clang/Lex/LiteralSupport.h" | ||
#include "clang/Parse/ParseHLSLRootSignature.h" | ||
#include "clang/Parse/Parser.h" | ||
#include "clang/Parse/RAIIObjectsForParser.h" | ||
#include "clang/Sema/DeclSpec.h" | ||
#include "clang/Sema/EnterExpressionEvaluationContext.h" | ||
#include "clang/Sema/Lookup.h" | ||
#include "clang/Sema/ParsedTemplate.h" | ||
#include "clang/Sema/Scope.h" | ||
#include "clang/Sema/SemaCodeCompletion.h" | ||
|
@@ -5311,6 +5313,90 @@ void Parser::ParseMicrosoftUuidAttributeArgs(ParsedAttributes &Attrs) { | |
} | ||
} | ||
|
||
void Parser::ParseMicrosoftRootSignatureAttributeArgs(ParsedAttributes &Attrs) { | ||
assert(Tok.is(tok::identifier) && | ||
"Expected an identifier to denote which MS attribute to consider"); | ||
IdentifierInfo *RootSignatureIdent = Tok.getIdentifierInfo(); | ||
assert(RootSignatureIdent->getName() == "RootSignature" && | ||
"Expected RootSignature identifier for root signature attribute"); | ||
|
||
SourceLocation RootSignatureLoc = Tok.getLocation(); | ||
ConsumeToken(); | ||
|
||
// Ignore the left paren location for now. | ||
BalancedDelimiterTracker T(*this, tok::l_paren); | ||
if (T.consumeOpen()) { | ||
Diag(Tok, diag::err_expected) << tok::l_paren; | ||
return; | ||
} | ||
|
||
auto ProcessStringLiteral = [this]() -> std::optional<StringLiteral *> { | ||
if (!isTokenStringLiteral()) | ||
return std::nullopt; | ||
|
||
ExprResult StringResult = ParseUnevaluatedStringLiteralExpression(); | ||
if (StringResult.isInvalid()) | ||
return std::nullopt; | ||
|
||
if (auto Lit = dyn_cast<StringLiteral>(StringResult.get())) | ||
return Lit; | ||
|
||
return std::nullopt; | ||
}; | ||
|
||
auto StrLiteral = ProcessStringLiteral(); | ||
if (!StrLiteral.has_value()) { | ||
Diag(Tok, diag::err_expected_string_literal) | ||
<< /*in attributes...*/ 4 << RootSignatureIdent->getName(); | ||
SkipUntil(tok::r_paren, StopAtSemi | StopBeforeMatch); | ||
T.consumeClose(); | ||
return; | ||
} | ||
|
||
// Construct our identifier | ||
StringRef Signature = StrLiteral.value()->getString(); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Sorry for the late post-commit review, but I just noticed this patch because it's doing a lot of sema work from within the parser. Is there a reason this wasn't done using an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Primarily, we wanted to prevent needing to invoke However, there isn't any reason to prevent a pattern like:
To move the sema work out. Does that sound reasonable? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have created an issue to track this here: #142834. In the issue is the proposed solution and we can have the discussion there about correctness. |
||
auto Hash = llvm::hash_value(Signature); | ||
std::string IdStr = "__hlsl_rootsig_decl_" + std::to_string(Hash); | ||
IdentifierInfo *DeclIdent = &(Actions.getASTContext().Idents.get(IdStr)); | ||
|
||
LookupResult R(Actions, DeclIdent, SourceLocation(), | ||
Sema::LookupOrdinaryName); | ||
// Check if we have already found a decl of the same name, if we haven't | ||
// then parse the root signature string and construct the in-memory elements | ||
if (!Actions.LookupQualifiedName(R, Actions.CurContext)) { | ||
SourceLocation SignatureLoc = | ||
StrLiteral.value()->getExprLoc().getLocWithOffset( | ||
1); // offset 1 for '"' | ||
// Invoke the root signature parser to construct the in-memory constructs | ||
hlsl::RootSignatureLexer Lexer(Signature, SignatureLoc); | ||
SmallVector<llvm::hlsl::rootsig::RootElement> RootElements; | ||
hlsl::RootSignatureParser Parser(RootElements, Lexer, PP); | ||
if (Parser.parse()) { | ||
T.consumeClose(); | ||
return; | ||
} | ||
|
||
// Create the Root Signature | ||
auto *SignatureDecl = HLSLRootSignatureDecl::Create( | ||
Actions.getASTContext(), /*DeclContext=*/Actions.CurContext, | ||
RootSignatureLoc, DeclIdent, RootElements); | ||
SignatureDecl->setImplicit(); | ||
Actions.PushOnScopeChains(SignatureDecl, getCurScope()); | ||
} | ||
|
||
// Create the arg for the ParsedAttr | ||
IdentifierLoc *ILoc = ::new (Actions.getASTContext()) | ||
IdentifierLoc(RootSignatureLoc, DeclIdent); | ||
|
||
ArgsVector Args = {ILoc}; | ||
|
||
if (!T.consumeClose()) | ||
Attrs.addNew(RootSignatureIdent, | ||
SourceRange(RootSignatureLoc, T.getCloseLocation()), nullptr, | ||
SourceLocation(), Args.data(), Args.size(), | ||
ParsedAttr::Form::Microsoft()); | ||
} | ||
|
||
/// ParseMicrosoftAttributes - Parse Microsoft attributes [Attr] | ||
/// | ||
/// [MS] ms-attribute: | ||
|
@@ -5345,6 +5431,8 @@ void Parser::ParseMicrosoftAttributes(ParsedAttributes &Attrs) { | |
break; | ||
if (Tok.getIdentifierInfo()->getName() == "uuid") | ||
ParseMicrosoftUuidAttributeArgs(Attrs); | ||
else if (Tok.getIdentifierInfo()->getName() == "RootSignature") | ||
ParseMicrosoftRootSignatureAttributeArgs(Attrs); | ||
else { | ||
IdentifierInfo *II = Tok.getIdentifierInfo(); | ||
SourceLocation NameLoc = Tok.getLocation(); | ||
|
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 extend this to dump more info about the root elements. For instance, number of elements, or iterate through the types, etc.
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 think through how we want to serialize the root signature to text as part of the AST. It might be nice to have something so that we can test/inspect 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.
Noting:
There exists unit testing to check in-memory representation is constructed correctly
Tests in this pr ensure that the decl and attr are constructed/allocated
So there is technically a small test gap that the constructed root-elements are copied correctly over, which could be demonstrated by adding serialization for testing.
With that said, I think adding serialization to this pr would get out of its scope, so #138025 will track that.
Do we want to block this pr on implementing the serialization of the elements used in the testcase? (I have verified the elements are copied over correctly on a local quick-hack impl of serialization)
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.
From design we will block on #138025 such that we can provide sufficient testing.
We will go with an initial approach to just dump all information of the in-memory constructs and we can revisit this to maybe only print explicitly set parameters if this becomes to verbose/hard to debug with.