-
Notifications
You must be signed in to change notification settings - Fork 17
Resource initialization proposal update - add static init methods #336
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
Conversation
…tbinding (#155861) Reorder the arguments of `__builtin_hlsl_resource_handlefromimplicitbinding` builtins to match the order of the `llvm.dx.resource.handlefromimplicitbinding` intrinsics, and also to match the arguments on the static create methods for resource initialization ([described here](llvm/wg-hlsl#336)). Previously the arguments were in the same order as the resource class constructor for implicit binding. The `orderId` argument was intentionally at index `3` to make sure explicit & implicit binding constructors have different signature. Since we are going to replace the constructors that have binding info with static create methods, this is no longer necessary, and it is better for the argument order to match. Related to #154221.
…fromimplicitbinding (#155861) Reorder the arguments of `__builtin_hlsl_resource_handlefromimplicitbinding` builtins to match the order of the `llvm.dx.resource.handlefromimplicitbinding` intrinsics, and also to match the arguments on the static create methods for resource initialization ([described here](llvm/wg-hlsl#336)). Previously the arguments were in the same order as the resource class constructor for implicit binding. The `orderId` argument was intentionally at index `3` to make sure explicit & implicit binding constructors have different signature. Since we are going to replace the constructors that have binding info with static create methods, this is no longer necessary, and it is better for the argument order to match. Related to #154221.
bogner
left a comment
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.
Generally looks good. A few minor notes.
Also, on a very pedantic level, from a language definition point of view C++ (and thus HLSL) doesn't really have "methods", but uses the term "member functions". For the purpose of these documents I think the term "method" is well known enough that it doesn't really matter, but we probably want to be careful about this if it comes to describing any of this in the language standard itself.
| referred to as _resource classes_ or _resource records_. These terms can be used | ||
| interchangeably. | ||
| The record classes can be declared at the global scope, used as function in/out |
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.
Should "record classes" here be one of "resource structs", "resource classes", or "resource records", as defined above?
| `__builtin_hlsl_resource_handlefrombinding` Clang builtin function will be used | ||
| to infer the return type of that function. This is the same way we infer return | ||
| types for HLSL intrinsic builtins based on their arguments, except in the case | ||
| only the type of the argument is used and not its (uninitialized) value. |
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.
Note that saying the value here is uninitialized implies that the default constructor (as mentioned above) specifically does not initialize the handle. This is mostly true, as initializing to poison as described in the section on the default constructor is morally equivalent to uninitialized, but we may need to take care that we aren't generating redundant intrinsic calls for initialization in these initializer functions.
s-perron
left a comment
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. This will be easier to work with. Thanks.
| `RWBuffer<float> A = RWBuffer<float>::__createFromImplicitBinding(0,13,1,0,"A");`. | ||
| ### Resources with dynamic binding |
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 is dynamic binding? I'm curious.
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.
Resources that have binding assigned at runtime: https://microsoft.github.io/DirectX-Specs/d3d/HLSL_SM_6_6_DynamicResources.html
Adds static methods `__createFromBinding` and `__createFromImplicitBinding` to resource classes. These methods will be used for resource initialization instead of resource constructors that take binding information. Updated proposal: llvm/wg-hlsl#336
…5866) Adds static methods `__createFromBinding` and `__createFromImplicitBinding` to resource classes. These methods will be used for resource initialization instead of resource constructors that take binding information. Updated proposal: llvm/wg-hlsl#336
…156544) Use static methods `__createFromBinding` and `__createFromImplicitBinding` to initialize individual resources in Sema HLSL instead of calling resource constructor with binding information per proposal update llvm/wg-hlsl#336. Initialization of resources in arrays will be updated in a separate change because that happens in the codegen layer. Test updates include the use of the `llvm-cxxfilt` tool which takes care of demangling of function names for a more readable test baseline.
…resources (#156544) Use static methods `__createFromBinding` and `__createFromImplicitBinding` to initialize individual resources in Sema HLSL instead of calling resource constructor with binding information per proposal update llvm/wg-hlsl#336. Initialization of resources in arrays will be updated in a separate change because that happens in the codegen layer. Test updates include the use of the `llvm-cxxfilt` tool which takes care of demangling of function names for a more readable test baseline.
…157005) Use static methods `__createFromBinding` and `__createFromImplicitBinding` to initialize resources in resource arrays instead of calling resource constructors with binding information, per proposal update llvm/wg-hlsl#336. Test updates include the use of the `llvm-cxxfilt` tool which takes care of demangling of function names for a more readable test baseline.
…n arrays (#157005) Use static methods `__createFromBinding` and `__createFromImplicitBinding` to initialize resources in resource arrays instead of calling resource constructors with binding information, per proposal update llvm/wg-hlsl#336. Test updates include the use of the `llvm-cxxfilt` tool which takes care of demangling of function names for a more readable test baseline.
Removes resource constructors that take binding information per proposal update llvm/wg-hlsl#336. The constructors are replaced by static `__createFromBinding` and `__createFromImplicitBinding` methods on the resource class.
Removes resource constructors that take binding information per proposal update llvm/wg-hlsl#336. The constructors are replaced by static `__createFromBinding` and `__createFromImplicitBinding` methods on the resource class.
0025-resource-constructors.mdto0025-resource-initialization-and-constructors.md__createFromBindingand__createFromImplicitBindingthat will be used to initialize resource classes