Update proposals for implicit binding and resource constructors to match implementation (#282)

Author: hekota

proposals/0024-implicit-resource-binding.md

@@ -448,26 +448,25 @@ particular entry point is linked to a final shader.
 
 ### Clang Frontend
 
-Unbound resources will be initialized by a resource class constructor that takes
-3 arguments - the `space`, `range` and `index`. This constructor will be
-declared in `HLSLExternalSemaSource` as part of resource class setup.
+Unbound resources will be initialized by a resource class constructor for
+implicit binding which is described [here](0025-resource-constructors.md#constructor-for-resources-with-implicit-binding).
 
-The arguments specify the virtual register space (defaulting to 0),
-required descriptor range and an index of the resource in the range. 
+The `order_id` number will be generated by in `SemaHLSL` class and it will
+be used to uniquely identify the unbound resource, and it will also reflect the
+order in which the resource has been declared. Implicit binding assignments
+depend on the order the resources were declared, so this will help the compiler
+to preserve the order. Additionally, it will also help the compiler distinquish
+between individual resources with the same type and binding range.
 
 The constructor will call Clang builtin function
-`__builtin_hlsl_create_handle_from_implicit_binding` and will pass along the
-`space`, `range` and `index` values, and it will also include the uninitialized
-resource handle. The type of the handle argument will be used to infer the
-specific resource handle type returned by the buildin function. This will happen
-in `SemaHLSL::CheckBuiltinFunctionCall` the same way we infer return types for
-HLSL intrinsic builtins based on the builtin arguments.
+`__builtin_hlsl_create_handle_from_implicit_binding` and will pass along all the
+arguments provided to the constructor plus the resource handle.
 
 ```c++
 template <typename T> class RWBuffer<T> {
-  RWBuffer(unsigned space, int range, unsigned index) {
-    __handle = __builtin_hlsl_create_handle_from_implicit_binding(__handle, space, range, index);
-  }
+  RWBuffer(unsigned spaceNo, int range, unsigned index, unsigned orderId, const char *name) {
+    __handle = __builtin_hlsl_resource_createhandlefromimplicitbinding(__handle, spaceNo, range, index, orderId, name);
+  }  
 }
 ```
 
@@ -485,36 +484,72 @@ The signature of the `@llvm.dx.resource.handlefromimplicitbinding` intrinsic wil
 | %space	|	i32 |	Virtual register space |
 | %range	|	i32 |	Range size of the binding |
 | %index	|	i32	| Index from the beginning of the binding |
+| %name	  |	ptr	| Pointer to a global constant string that is the name of the resource |
 
 _Note: We might need to add a uniformity bit here, unless we can derive it from uniformity analysis._
 
-The `order_id` number will be generated by Clang Codegen in `GCHLSLRuntime`. It
-will be used to uniquely identify the unbound resource, and it will also reflect
-the order in which the resource has been declared. Implicit binding assignments
-depend on the order the resources were declared, so this will help the compiler
-to preserve the order. Additionally, it will also help the compiler distinquish
-between individual resources with the same type and binding range.
-
 ### LLVM Binding Assignment Pass
 
-The implicit binding assignment for DirectX will happen in an LLVM pass
-`DXILResourceImplicitBindings`. The pass will scan the module for all instances of
-`@llvm.dx.resource.handlefrombinding` and create a map of available register
-slots. Then it will gather all instances of
-`@llvm.dx.resource.handlefromimplicitbinding` calls and sort them by
-`%order_id`. Then for each group of calls operating on the same unique resource
-it will:
+The implicit binding assignment will happen in two phases - `DXILResourceBindingAnalysis` and `DXILResourceImplicitBindings`.
+
+#### 1. Analysis of available register spaces
+
+A new `DXILResourceBindingAnalysis` will scan the module for all instances of
+`@llvm.dx.resource.handlefrombinding` calls and create a map of available
+register slots and spaces. The results of the analysis will be stored in
+`DXILResourceBindingInfo`.
+
+While the map is being created the analysis can easily detect whether any of the
+register bindings overlap, which is something the compiler needs to diagnose.
+However, in order to issue a useful error message for this, the analysis would
+have to track which register ranges are occupied by which resource. That is
+beyond the need of the common case scenario where all bindings are correct and
+the analysis just need to figure out the available register slot ranges.
+
+For this reason if the analysis finds that some register bindings overlap, it
+will prioritize performance for the common case and just set the
+`hasOverlappingBinding` flag on `DXILResourceBindingInfo` to true. The detailed
+ error message calculation for the uncommon failure code path will happen later
+in `DXILPostOptimizationValidation` pass which can analyze the resource bindings
+in more detail and report exactly which resources are overlapping and where. 
+
+This is the same principle that we use when detecting invalid counter usage for
+structured buffers as described
+[here](0022-resource-instance-analysis.md).
+
+While scanning for the `@llvm.dx.resource.handlefrombinding` calls the analysis
+can also take note on whether there are any
+`@llvm.dx.resource.handlefromimplicitbinding` calls in the module. If there are,
+it will set the `hasImplicitBinding` flag on `DXILResourceBindingInfo` to true.
+This will enable the follow-up `DXILResourceImplicitBindings` pass to exit early
+if there are no implicit bindings in the module.
+
+_Note: In general diagnostics should not be raised in LLVM analyses or
+passes. Analyses may be invalidated and re-ran several times, increasing
+performance impact and raising repeated diagnostics. Diagnostics raised after
+transformations passes also lose source context resulting in less useful error
+messages. However the shader compiler requires certain validations to be done
+after code optimizations which requires the diagnostic to be raised from a pass.
+Impact is minimized by raising the diagnostic only from a limited set of passes
+and minimizing computation in the common case._
+
+#### 2. Assign bindings to implictily bound resource
+
+The implicit binding assignment will happen in an LLVM pass
+`DXILResourceImplicitBindings`. The pass will use the map of available register
+ spaces `DXILResourceBindingInfo` created by `DXILResourceBindingAnalysis`. It
+will scan the module for all instances of
+`@llvm.dx.resource.handlefromimplicitbinding` calls, sort them by `order_id`,
+and then for each group of calls operating on the same unique resource it will:
 - find an available space to bind the resource based on the resource class,
-required range and space
+  required range and space
 - replace the `@llvm.dx.resource.handlefromimplicitbinding` calls and all of their
-uses with `@llvm.dx.resource.handlefrombinding` using the new binding
+uses with `@llvm.dx.resource.handlefrombinding` with the assigned binding
 
 The `DXILResourceImplicitBindings` pass needs to run after all IR optimizations
-passes but before any pass or analysis that relies on
-`@llvm.dx.resource.handlefrombinding` calls to exist for all non-dynamically
-bound resources used by the shader. For example, it needs to run before any pass
-that requires `DXILResourceAnalysis`. The pass invalidate any existing
-`DXILResourceAnalysis` if it assigns any new bindings.
+passes but before any pass that depends on `DXILResourceAnalysis` which relies
+on `@llvm.dx.resource.handlefrombinding` calls to exist for all non-dynamically
+bound resources used by the shader.
 
 ## Alternatives considered (Optional)
 

proposals/0025-resource-constructors.md

@@ -90,8 +90,8 @@ template <typename T> struct RWBuffer {
   ...
 private:
   // For resources with explicit binding
-  RWBuffer(unsigned register, unsigned space, int range, unsigned index) {
-    __handle = __builtin_hlsl_resource_createhandlefrombinding(__handle, register, space, range, index);
+  RWBuffer(unsigned registerNo, unsigned spaceNo, int range, unsigned index, const char *name) {
+    __handle = __builtin_hlsl_resource_createhandlefrombinding(__handle, registerNo, spaceNo, range, index, name);
   }
   ...
 };
@@ -102,6 +102,10 @@ The `__handle` argument passed into the
 to infer the return type of the that function (the same way as for the default
 construtor).
 
+The `name` argument will be used to generate the DXIL resource metadata and also
+for resource diagnostics that need to happen after optimizations later in the
+compiler pipeline.
+
 A call to this constructor will be created by Sema as part of uninitialized
 variable declaration processing (`Sema::ActOnUninitializedDecl`). It will
 work as if it would replace:
@@ -110,7 +114,7 @@ work as if it would replace:
 
 with
 
-`RWBuffer<float> A(3,0,1,0);`.
+`RWBuffer<float> A(3,0,1,0,"A");`.
 
 ### Constructor for resources with implicit binding
 
@@ -125,17 +129,26 @@ template <typename T> struct RWBuffer {
   ...
 private:
   // For resources with implicit binding
-  RWBuffer(unsigned space, int range, unsigned index) {
-    __handle = __builtin_hlsl_resource_createhandlefromimplicitbinding(__handle, space, range, index);
+  RWBuffer(unsigned spaceNo, int range, unsigned index, unsigned orderId, const char *name) {
+    __handle = __builtin_hlsl_resource_createhandlefromimplicitbinding(__handle, spaceNo, range, index, orderId, name);
   }
   ...
 };
 ```
 
 The `__handle` argument passed into the
-`__builtin_hlsl_resource_createhandlefromimplicitbinding` Clang builtin function will
-be used to infer the return type of the that function (the same way as for the
-default construtor).
+`__builtin_hlsl_resource_createhandlefromimplicitbinding` Clang builtin function
+will be used to infer the return type of the that function (the same way as for
+the default construtor).
+
+The `orderId` number will be generated by in `SemaHLSL` class and it will be
+used to uniquely identify the unbound resource, and it will also reflect the
+order in which the resource has been declared. It will be used later on in the
+compiler to assign implicit bindings to resources in the right order.
+
+The `name` argument will be used to generate the DXIL resource metadata and also
+for resource diagnostics that need to happen after optimizations later in the
+compiler pipeline.
 
 A call to this constructor will be created by Sema as part of uninitialized
 variable declaration processing (`Sema::ActOnUninitializedDecl`). It will
@@ -145,7 +158,7 @@ work as if it would replace:
 
 with
 
-`RWBuffer<float> A(0,1,0);`.
+`RWBuffer<float> A(0,1,0,0,"A");`.
 
 Or if the resource has a space-only binding annotation, it will work as if it
 would replace:
@@ -154,7 +167,7 @@ would replace:
 
 with
 
-`RWBuffer<float> A(13,1,0);`.
+`RWBuffer<float> A(13,1,0,0,"A");`.
 
 ### Copy constructor and assignment operator