[HLSL] Forward arguments in BuiltinTypeMethodBuilder::callBuiltin. NFC #117789
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
Introduce BuiltinTypeMethodBuilder::PlaceHolder values and use them in the callBuiltin method in order to specify how we want to forward arguments and pass the resource handle to builtins.
llvmbot
added
clang
Member
|
@llvm/pr-subscribers-clang Author: Justin Bogner (bogner) ChangesIntroduce BuiltinTypeMethodBuilder::PlaceHolder values and use them in the callBuiltin method in order to specify how we want to forward arguments and pass the resource handle to builtins. Full diff: https://github.com/llvm/llvm-project/pull/117789.diff 1 Files Affected:
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index 886a4c098580ae..fae43d187e0532 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -455,7 +455,7 @@ struct TemplateParameterListBuilder {
//
// BuiltinTypeMethodBuilder(Sema, RecordBuilder, "MethodName", ReturnType)
// .addParam("param_name", Type, InOutModifier)
-// .callBuiltin("buildin_name", { BuiltinParams })
+// .callBuiltin("buildin_name", BuiltinParams... )
// .finalizeMethod();
//
// The builder needs to have all of the method parameters before it can create
@@ -465,9 +465,9 @@ struct TemplateParameterListBuilder {
// referenced from the body building methods. Destructor or an explicit call to
// finalizeMethod() will complete the method definition.
//
-// The callBuiltin helper method passes in the resource handle as the first
-// argument of the builtin call. If this is not desired it takes a bool flag to
-// disable this.
+// The callBuiltin helper method accepts constants via `Expr *` or placeholder
+// value arguments to indicate which function arguments to forward to the
+// builtin.
//
// If the method that is being built has a non-void return type the
// finalizeMethod will create a return statent with the value of the last
@@ -489,6 +489,24 @@ struct BuiltinTypeMethodBuilder {
llvm::SmallVector<MethodParam> Params;
llvm::SmallVector<Stmt *> StmtsList;
+ // Argument placeholders, inspired by std::placeholder. These are the indices
+ // of arguments to forward to `callBuiltin`, and additionally `Handle` which
+ // refers to the resource handle.
+ enum class PlaceHolder { _0, _1, _2, _3, Handle = 127 };
+
+ Expr *convertPlaceholder(PlaceHolder PH) {
+ if (PH == PlaceHolder::Handle)
+ return getResourceHandleExpr();
+
+ ASTContext &AST = DeclBuilder.SemaRef.getASTContext();
+ ParmVarDecl *ParamDecl = Method->getParamDecl(static_cast<unsigned>(PH));
+ return DeclRefExpr::Create(
+ AST, NestedNameSpecifierLoc(), SourceLocation(), ParamDecl, false,
+ DeclarationNameInfo(ParamDecl->getDeclName(), SourceLocation()),
+ ParamDecl->getType(), VK_PRValue);
+ }
+ Expr *convertPlaceholder(Expr *E) { return E; }
+
public:
BuiltinTypeMethodBuilder(Sema &S, BuiltinTypeDeclBuilder &DB, StringRef Name,
QualType ReturnTy)
@@ -502,7 +520,10 @@ struct BuiltinTypeMethodBuilder {
HLSLParamModifierAttr::Spelling Modifier =
HLSLParamModifierAttr::Keyword_in) {
assert(Method == nullptr && "Cannot add param, method already created");
- llvm_unreachable("not yet implemented");
+ const IdentifierInfo &II = DeclBuilder.SemaRef.getASTContext().Idents.get(
+ Name, tok::TokenKind::identifier);
+ Params.emplace_back(II, Ty, Modifier);
+ return *this;
}
private:
@@ -564,9 +585,9 @@ struct BuiltinTypeMethodBuilder {
OK_Ordinary);
}
- BuiltinTypeMethodBuilder &
- callBuiltin(StringRef BuiltinName, ArrayRef<Expr *> CallParms,
- bool AddResourceHandleAsFirstArg = true) {
+ template <typename... Ts>
+ BuiltinTypeMethodBuilder &callBuiltin(StringRef BuiltinName, Ts... ArgSpecs) {
+ SmallVector<Expr *> Args{convertPlaceholder(std::forward<Ts>(ArgSpecs))...};
// The first statement added to a method or access to 'this` creates the
// declaration.
@@ -579,16 +600,9 @@ struct BuiltinTypeMethodBuilder {
AST, NestedNameSpecifierLoc(), SourceLocation(), FD, false,
FD->getNameInfo(), FD->getType(), VK_PRValue);
- SmallVector<Expr *> NewCallParms;
- if (AddResourceHandleAsFirstArg) {
- NewCallParms.push_back(getResourceHandleExpr());
- for (auto *P : CallParms)
- NewCallParms.push_back(P);
- }
-
- Expr *Call = CallExpr::Create(
- AST, DRE, AddResourceHandleAsFirstArg ? NewCallParms : CallParms,
- FD->getReturnType(), VK_PRValue, SourceLocation(), FPOptionsOverride());
+ Expr *Call =
+ CallExpr::Create(AST, DRE, Args, FD->getReturnType(), VK_PRValue,
+ SourceLocation(), FPOptionsOverride());
StmtsList.push_back(Call);
return *this;
}
@@ -656,18 +670,20 @@ BuiltinTypeDeclBuilder::addSimpleTemplateParams(ArrayRef<StringRef> Names,
}
BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addIncrementCounterMethod() {
+ using PH = BuiltinTypeMethodBuilder::PlaceHolder;
return BuiltinTypeMethodBuilder(SemaRef, *this, "IncrementCounter",
SemaRef.getASTContext().UnsignedIntTy)
- .callBuiltin("__builtin_hlsl_buffer_update_counter",
- {getConstantIntExpr(1)})
+ .callBuiltin("__builtin_hlsl_buffer_update_counter", PH::Handle,
+ getConstantIntExpr(1))
.finalizeMethod();
}
BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addDecrementCounterMethod() {
+ using PH = BuiltinTypeMethodBuilder::PlaceHolder;
return BuiltinTypeMethodBuilder(SemaRef, *this, "DecrementCounter",
SemaRef.getASTContext().UnsignedIntTy)
- .callBuiltin("__builtin_hlsl_buffer_update_counter",
- {getConstantIntExpr(-1)})
+ .callBuiltin("__builtin_hlsl_buffer_update_counter", PH::Handle,
+ getConstantIntExpr(-1))
.finalizeMethod();
}
|
Member
|
@llvm/pr-subscribers-hlsl Author: Justin Bogner (bogner) ChangesIntroduce BuiltinTypeMethodBuilder::PlaceHolder values and use them in the callBuiltin method in order to specify how we want to forward arguments and pass the resource handle to builtins. Full diff: https://github.com/llvm/llvm-project/pull/117789.diff 1 Files Affected:
diff --git a/clang/lib/Sema/HLSLExternalSemaSource.cpp b/clang/lib/Sema/HLSLExternalSemaSource.cpp
index 886a4c098580ae..fae43d187e0532 100644
--- a/clang/lib/Sema/HLSLExternalSemaSource.cpp
+++ b/clang/lib/Sema/HLSLExternalSemaSource.cpp
@@ -455,7 +455,7 @@ struct TemplateParameterListBuilder {
//
// BuiltinTypeMethodBuilder(Sema, RecordBuilder, "MethodName", ReturnType)
// .addParam("param_name", Type, InOutModifier)
-// .callBuiltin("buildin_name", { BuiltinParams })
+// .callBuiltin("buildin_name", BuiltinParams... )
// .finalizeMethod();
//
// The builder needs to have all of the method parameters before it can create
@@ -465,9 +465,9 @@ struct TemplateParameterListBuilder {
// referenced from the body building methods. Destructor or an explicit call to
// finalizeMethod() will complete the method definition.
//
-// The callBuiltin helper method passes in the resource handle as the first
-// argument of the builtin call. If this is not desired it takes a bool flag to
-// disable this.
+// The callBuiltin helper method accepts constants via `Expr *` or placeholder
+// value arguments to indicate which function arguments to forward to the
+// builtin.
//
// If the method that is being built has a non-void return type the
// finalizeMethod will create a return statent with the value of the last
@@ -489,6 +489,24 @@ struct BuiltinTypeMethodBuilder {
llvm::SmallVector<MethodParam> Params;
llvm::SmallVector<Stmt *> StmtsList;
+ // Argument placeholders, inspired by std::placeholder. These are the indices
+ // of arguments to forward to `callBuiltin`, and additionally `Handle` which
+ // refers to the resource handle.
+ enum class PlaceHolder { _0, _1, _2, _3, Handle = 127 };
+
+ Expr *convertPlaceholder(PlaceHolder PH) {
+ if (PH == PlaceHolder::Handle)
+ return getResourceHandleExpr();
+
+ ASTContext &AST = DeclBuilder.SemaRef.getASTContext();
+ ParmVarDecl *ParamDecl = Method->getParamDecl(static_cast<unsigned>(PH));
+ return DeclRefExpr::Create(
+ AST, NestedNameSpecifierLoc(), SourceLocation(), ParamDecl, false,
+ DeclarationNameInfo(ParamDecl->getDeclName(), SourceLocation()),
+ ParamDecl->getType(), VK_PRValue);
+ }
+ Expr *convertPlaceholder(Expr *E) { return E; }
+
public:
BuiltinTypeMethodBuilder(Sema &S, BuiltinTypeDeclBuilder &DB, StringRef Name,
QualType ReturnTy)
@@ -502,7 +520,10 @@ struct BuiltinTypeMethodBuilder {
HLSLParamModifierAttr::Spelling Modifier =
HLSLParamModifierAttr::Keyword_in) {
assert(Method == nullptr && "Cannot add param, method already created");
- llvm_unreachable("not yet implemented");
+ const IdentifierInfo &II = DeclBuilder.SemaRef.getASTContext().Idents.get(
+ Name, tok::TokenKind::identifier);
+ Params.emplace_back(II, Ty, Modifier);
+ return *this;
}
private:
@@ -564,9 +585,9 @@ struct BuiltinTypeMethodBuilder {
OK_Ordinary);
}
- BuiltinTypeMethodBuilder &
- callBuiltin(StringRef BuiltinName, ArrayRef<Expr *> CallParms,
- bool AddResourceHandleAsFirstArg = true) {
+ template <typename... Ts>
+ BuiltinTypeMethodBuilder &callBuiltin(StringRef BuiltinName, Ts... ArgSpecs) {
+ SmallVector<Expr *> Args{convertPlaceholder(std::forward<Ts>(ArgSpecs))...};
// The first statement added to a method or access to 'this` creates the
// declaration.
@@ -579,16 +600,9 @@ struct BuiltinTypeMethodBuilder {
AST, NestedNameSpecifierLoc(), SourceLocation(), FD, false,
FD->getNameInfo(), FD->getType(), VK_PRValue);
- SmallVector<Expr *> NewCallParms;
- if (AddResourceHandleAsFirstArg) {
- NewCallParms.push_back(getResourceHandleExpr());
- for (auto *P : CallParms)
- NewCallParms.push_back(P);
- }
-
- Expr *Call = CallExpr::Create(
- AST, DRE, AddResourceHandleAsFirstArg ? NewCallParms : CallParms,
- FD->getReturnType(), VK_PRValue, SourceLocation(), FPOptionsOverride());
+ Expr *Call =
+ CallExpr::Create(AST, DRE, Args, FD->getReturnType(), VK_PRValue,
+ SourceLocation(), FPOptionsOverride());
StmtsList.push_back(Call);
return *this;
}
@@ -656,18 +670,20 @@ BuiltinTypeDeclBuilder::addSimpleTemplateParams(ArrayRef<StringRef> Names,
}
BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addIncrementCounterMethod() {
+ using PH = BuiltinTypeMethodBuilder::PlaceHolder;
return BuiltinTypeMethodBuilder(SemaRef, *this, "IncrementCounter",
SemaRef.getASTContext().UnsignedIntTy)
- .callBuiltin("__builtin_hlsl_buffer_update_counter",
- {getConstantIntExpr(1)})
+ .callBuiltin("__builtin_hlsl_buffer_update_counter", PH::Handle,
+ getConstantIntExpr(1))
.finalizeMethod();
}
BuiltinTypeDeclBuilder &BuiltinTypeDeclBuilder::addDecrementCounterMethod() {
+ using PH = BuiltinTypeMethodBuilder::PlaceHolder;
return BuiltinTypeMethodBuilder(SemaRef, *this, "DecrementCounter",
SemaRef.getASTContext().UnsignedIntTy)
- .callBuiltin("__builtin_hlsl_buffer_update_counter",
- {getConstantIntExpr(-1)})
+ .callBuiltin("__builtin_hlsl_buffer_update_counter", PH::Handle,
+ getConstantIntExpr(-1))
.finalizeMethod();
}
|
damyanp
reviewed
Nov 26, 2024
Contributor
damyanp
left a comment
damyanp
left a comment
There was a problem hiding this comment.
LGTM - a nit and an idea in the comments.
| bool AddResourceHandleAsFirstArg = true) { | ||
| template <typename... Ts> | ||
| BuiltinTypeMethodBuilder &callBuiltin(StringRef BuiltinName, Ts... ArgSpecs) { | ||
| SmallVector<Expr *> Args{convertPlaceholder(std::forward<Ts>(ArgSpecs))...}; |
Contributor
There was a problem hiding this comment.
Is SmallVector smart enough for this to evaporate into being a stack allocated array, or is there a chance that it might make a heap allocation to store the args?
Contributor
There was a problem hiding this comment.
Yes, SmallVector will have stack allocated storage. Its size is derived from the initialization, which is dependent on the number of ArgSpecs.
Contributor
Author
There was a problem hiding this comment.
This raises a good point though - I'll change this to just use std::array here since we know the exact size at compile time.
| .callBuiltin("__builtin_hlsl_buffer_update_counter", | ||
| {getConstantIntExpr(1)}) | ||
| .callBuiltin("__builtin_hlsl_buffer_update_counter", PH::Handle, | ||
| getConstantIntExpr(1)) |
Contributor
There was a problem hiding this comment.
How evil would it be to add a Expr *convertPlaceholder(int) that does the getConstantIntExpr for you, so this could be written as: .callBuiltin("__builtin_hlsl_buffer_update_conter", PH::Handle, 1)?
Contributor
Author
There was a problem hiding this comment.
This feels too magical to me (and also it introduces an annoying ambiguity with a literal zero - is it an int or a null Expr *?)
hekota
reviewed
Nov 26, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Introduce BuiltinTypeMethodBuilder::PlaceHolder values and use them in the callBuiltin method in order to specify how we want to forward arguments and pass the resource handle to builtins.