From 5fd3d84780bd28b82c70ce5a91f09271128187cf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Fri, 8 Nov 2024 15:20:29 +0100 Subject: [PATCH 1/7] [SPIR-V] Add proposal for I/O builtin in Clang MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- proposals/NNNN-spirv-input-builtin.md | 159 ++++++++++++++++++++++++++ 1 file changed, 159 insertions(+) create mode 100644 proposals/NNNN-spirv-input-builtin.md diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md new file mode 100644 index 0000000..05de47d --- /dev/null +++ b/proposals/NNNN-spirv-input-builtin.md @@ -0,0 +1,159 @@ +# SPIR-V Input/Output built-ins + + * Proposal: [NNNN](NNNN-spirv-input-builtin.md) + * Author(s): [Nathan Gauër](https://github.com/Keenuts) + * Status: **Design In Progress** + +## Introduction + +HLSL has semantic input/outputs parameters used to carry information +to and from a shader, e.g: the system semantic `SV_GroupID` or `MY_SEMANTIC`, +a user-defined semantic. + +In SPIR-V, those are translated to `Input` and `Output` variables, +with either a `BuiltIn` or `Location` decoration. + +Example: + +```hlsl +float4 vsmain(float4 Position : POSITION) : SV_POSITION +{ + return Position; +} +``` + +Both `POSITION` and `SV_POSITION` will be represented as an `OpVariable`: + +- `POSITION` will have the `Input` storage class. +- `SV_POSITION` will have the `Output` storage class. + +- `POSITION` will be decorated with `Location 0`. +- `SV_POSITION` will be decorated with `BuiltIn Position`. + +In addition, users can define their own SPIR-V built-ins using inline SPIR-V: + +```hlsl +[[vk::ext_builtin_input(/* NumWorkGroups */ 24)]] +static const uint3 myBuiltIn; +``` + +This document explain how we plan on implementing those in Clang/LLVM. + +## Proposed Solution + +## Static behaving like an external & LLVM + +The current HLSL syntax for inline SPIR-V requires those built-ins to be +`static [const]` variables. The rational was those variables were private to +the invocation, and thus could be written/read without any concurrency issues. +This was fine in DXC as we had our own AST->SPIR-V lowering. + +In Clang/LLVM, that's different: such static variable can be optimized in many +ways: +aggressive ways. + - A non-initialized `static const` is not allowed. + - Any dead-store to a `static int` can be removed. + - Meaning any pure computation parent to this store can be removed. + +Solving this can be done in 3 ways: + - Change the spec, allowing the syntax to be less "magical": use `extern`. + - Fixup the storage class of those variables in Clang. + - Emit an external constructor & destructor for those preventing optimization. + +I'd be in favor of the first solution, which correctly expresses in HLSL what +those built-in variables are, see this +[spec issue](https://github.com/microsoft/hlsl-specs/issues/350). +If we emit those as `extern [thread_local]`, everything works out of the box, +and HLSL feels less magical. + +The second solution is to patch Clang to add corner cases for HLSL: + - allow a `static const` to be left uninitialized. + - patch the storage class from `SC_Static` to `SC_Extern` +Advantage: we keep the spec as-is, but we add hacks in Clang. + +Third solution: keep the static, but add a CXXConstructor which calls a +target-specific intrinsic `load_builtin`. Advantage is Clang already has the +code to handle global initializers, so generating those is OK. +For the output, we'd need to do the opposite: insert another intrinsic +`store_builtin` at the end, which will make sure stores to the variable as not +optimized away. +The disadvantage is we need to emit more target-specific code in the +entry point, meaning more maintenance. + +## Frontend changes + +Once we solved the variable issue, we need two additional bit of information +to emit proper SPIR-V: + - the SPIR-V storage class: `Input` or `Output`. + - the attached `BuiltIn` or `Location` decoration. + +The plan is to add: +- 2 new attributes: `HLSLInputBuiltin` and `HLSLOutputBuiltin` +- 2 new address spaces: `vulkan_input` and `vulkan_output` + +The TD file will attach each attribute to a `SubjectList` with the following +constraints: + +``` +HLSLInputBuiltin: S->hasGlobalStorage() && + S->getStorageClass()==StorageClass::SC_Extern && + S->getType().isConstQualified() + +HLSLOutputBuiltin: S->hasGlobalStorage() && + S->getStorageClass()==StorageClass::SC_Extern && + !S->getType().isConstQualified() + +def HLSLVkExtBuiltinInput: InheritableAttr { + let Spellings = [CXX11<"vk", "ext_builtin_input">]; + let Args = [IntArgument<"BuiltIn">]; + let Subjects = SubjectList<[HLSLInputBuiltin], ErrorDiag>; + let LangOpts = [HLSL]; +} + +def HLSLVkExtBuiltinOutput: InheritableAttr { + let Spellings = [CXX11<"vk", "ext_builtin_output">]; + let Args = [IntArgument<"BuiltIn">]; + let Subjects = SubjectList<[HLSLOutputBuiltin], ErrorDiag>; + let LangOpts = [HLSL]; +} +``` + +When this attribute is encountered, the global variable address space is +changed to either `vulkan_input` or `vulkan_output`. +Then, a `MDNode` is attached: `spirv.Decorations`, adding the `BuiltIn` +decoration using the int argument of the attribute. + +Finally, for input/output parameters, we will; skip the attribute, but create +the same global variable with corresponding `vulkan_` address space, and +the `spirv.Decorations` metadata. + +The `llvm::Value` passed to the function will be a load from this global +variable. + + +## What if we don't change the spec and keep static + +The change mentioned above assumes we move to an `external`. In case we decide +not to change the HLSL spec, this still holds. +What changes is: + - Attr.td: constraint will need to check `SC_Static` + - global variable: a construction **will** be required OR storage class + is changed to `SC_External` internally. + - Sema has to be updated to allow `const static int MyVar;` to be legal. + +## Backend changes + +The SPIR-V backend already supports the `spirv.Decorations` metadata, so this +part works already. + +What's required is to add the translation from the `LangAS` `vulkan_input` and +`vulkan_output` to `Input` and `Output` storage class. This is quite trivial +as we already rely on the address space to determine the `OpVariable` storage +classes. + +## Draft PR + +https://github.com/llvm/llvm-project/pull/115187 + + + From 3211b4e7f93f846712c99c713b99e3feac16694f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Fri, 15 Nov 2024 16:28:35 +0100 Subject: [PATCH 2/7] revert spec change --- proposals/NNNN-spirv-input-builtin.md | 142 +++++++++++++------------- 1 file changed, 70 insertions(+), 72 deletions(-) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md index 05de47d..000f2c6 100644 --- a/proposals/NNNN-spirv-input-builtin.md +++ b/proposals/NNNN-spirv-input-builtin.md @@ -41,66 +41,26 @@ This document explain how we plan on implementing those in Clang/LLVM. ## Proposed Solution -## Static behaving like an external & LLVM - -The current HLSL syntax for inline SPIR-V requires those built-ins to be -`static [const]` variables. The rational was those variables were private to -the invocation, and thus could be written/read without any concurrency issues. -This was fine in DXC as we had our own AST->SPIR-V lowering. - -In Clang/LLVM, that's different: such static variable can be optimized in many -ways: -aggressive ways. - - A non-initialized `static const` is not allowed. - - Any dead-store to a `static int` can be removed. - - Meaning any pure computation parent to this store can be removed. - -Solving this can be done in 3 ways: - - Change the spec, allowing the syntax to be less "magical": use `extern`. - - Fixup the storage class of those variables in Clang. - - Emit an external constructor & destructor for those preventing optimization. - -I'd be in favor of the first solution, which correctly expresses in HLSL what -those built-in variables are, see this -[spec issue](https://github.com/microsoft/hlsl-specs/issues/350). -If we emit those as `extern [thread_local]`, everything works out of the box, -and HLSL feels less magical. - -The second solution is to patch Clang to add corner cases for HLSL: - - allow a `static const` to be left uninitialized. - - patch the storage class from `SC_Static` to `SC_Extern` -Advantage: we keep the spec as-is, but we add hacks in Clang. - -Third solution: keep the static, but add a CXXConstructor which calls a -target-specific intrinsic `load_builtin`. Advantage is Clang already has the -code to handle global initializers, so generating those is OK. -For the output, we'd need to do the opposite: insert another intrinsic -`store_builtin` at the end, which will make sure stores to the variable as not -optimized away. -The disadvantage is we need to emit more target-specific code in the -entry point, meaning more maintenance. - ## Frontend changes -Once we solved the variable issue, we need two additional bit of information -to emit proper SPIR-V: - - the SPIR-V storage class: `Input` or `Output`. - - the attached `BuiltIn` or `Location` decoration. +Global variables marked with Inline SPIR-V will be marked using two new +attributes: +- `HLSLInputBuiltin` +- `HLSLOutputBuiltin` -The plan is to add: -- 2 new attributes: `HLSLInputBuiltin` and `HLSLOutputBuiltin` -- 2 new address spaces: `vulkan_input` and `vulkan_output` +In addition, a new address space will be added: +- `vulkan_private` The TD file will attach each attribute to a `SubjectList` with the following constraints: ``` HLSLInputBuiltin: S->hasGlobalStorage() && - S->getStorageClass()==StorageClass::SC_Extern && + S->getStorageClass()==StorageClass::SC_Static && S->getType().isConstQualified() HLSLOutputBuiltin: S->hasGlobalStorage() && - S->getStorageClass()==StorageClass::SC_Extern && + S->getStorageClass()==StorageClass::SC_Static && !S->getType().isConstQualified() def HLSLVkExtBuiltinInput: InheritableAttr { @@ -118,42 +78,80 @@ def HLSLVkExtBuiltinOutput: InheritableAttr { } ``` -When this attribute is encountered, the global variable address space is -changed to either `vulkan_input` or `vulkan_output`. -Then, a `MDNode` is attached: `spirv.Decorations`, adding the `BuiltIn` -decoration using the int argument of the attribute. +When this attribute is encountered, several changes will occur: +- The address space will be set to `vulkan_private` +- a constructor will be added. +- a destructor will be added. + +The construction will call a target-specific intrinsic: `spv_load_builtin`. +This builtin takes the SPIR-V built-in ID as parameter, and returns the +value of the corresponding built-in. + +The destructor will call another intrinsic: `spv_store_builtin`. This builtin +takes the SPIR-V built-in ID, as well as a value to store as parameter. -Finally, for input/output parameters, we will; skip the attribute, but create -the same global variable with corresponding `vulkan_` address space, and -the `spirv.Decorations` metadata. +And lastly, to support input parameters with semantics, the same intrinsic +will be called in the generated entrypoint wrapper function. -The `llvm::Value` passed to the function will be a load from this global -variable. +Because the 2 new intrinsics will be marked as `MemWrite` or `MemRead`, LLVM +won't be able to optimize them away, and the initial/final load/store will +be kept. +Corresponding code could be: + +``` +[[vk::ext_builtin_input(/* GlobalInvocationId */ 28)]] +static const uint3 myBuiltIn; -## What if we don't change the spec and keep static +void main(uint3 thread_id_from_param : SV_DispatchThreadID) { + [...] +} + +constructor_myBuiltIn() { + myBuiltIn = spv_load_builtin(28); +} + +destructor_myBuiltIn() { + spv_store_builtin(28, myBuiltIn); +} -The change mentioned above assumes we move to an `external`. In case we decide -not to change the HLSL spec, this still holds. -What changes is: - - Attr.td: constraint will need to check `SC_Static` - - global variable: a construction **will** be required OR storage class - is changed to `SC_External` internally. - - Sema has to be updated to allow `const static int MyVar;` to be legal. +void entrypoint() { + constructor_myBuiltIn(); + + uint3 input_param1 = spv_load_builtin(28); + main(input_param1) + + destructor_myBuiltIn(); +} +``` + +The emitted global variable for inline SPIR-V will be in the global scope, +meaning it will require the `Private` storage class in SPIR-V. +Because of that, globals linked to the added attributes will require a new +address space `vulkan_private`. ## Backend changes -The SPIR-V backend already supports the `spirv.Decorations` metadata, so this -part works already. +Backend will translate the new `vulkan_privatge` address space to +`StorageClass::Private`. + +The backend will also need to implement 2 new intrinsics: +``` +llvm_any_ty spv_load_builtin(i32 built_in_id); +spv_store_builtin(i32 built_in_id, llvm_any_ty); +``` + +Both will use the GlobalRegistry to get/create a builtin global variable with +the correct decoration. A call to the `load` will generate an Input builtin, +while a call to the `store` will generate an `Output` builtin. + +Only the first lowering of each intrinsic will generate a builtin. -What's required is to add the translation from the `LangAS` `vulkan_input` and -`vulkan_output` to `Input` and `Output` storage class. This is quite trivial -as we already rely on the address space to determine the `OpVariable` storage -classes. +The load intrinsic will then add an `OpLoad`, while the store an `OpStore`. ## Draft PR -https://github.com/llvm/llvm-project/pull/115187 +https://github.com/llvm/llvm-project/pull/116393 From b30ffd61e8aa7fb29d33897d30e978d49c04d9da Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Tue, 26 Nov 2024 11:51:05 +0100 Subject: [PATCH 3/7] extend info around output/input store/load --- proposals/NNNN-spirv-input-builtin.md | 72 +++++++++++++++++++++++++-- 1 file changed, 67 insertions(+), 5 deletions(-) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md index 000f2c6..43a28e4 100644 --- a/proposals/NNNN-spirv-input-builtin.md +++ b/proposals/NNNN-spirv-input-builtin.md @@ -13,6 +13,14 @@ a user-defined semantic. In SPIR-V, those are translated to `Input` and `Output` variables, with either a `BuiltIn` or `Location` decoration. +Input `BuiltIn` values are private to the executing lane. Reading their +value has no side-effect. If not used, those built-in can safely be removed +from the shader module (Unlike the inputs tagged with `Location`). + +Output 'BuiltIn' values starts with an undefined value. Those are private +to the lane, and storing to them had no other side-effect than to modify the +initially stored value. It is allowed to load those values. + Example: ```hlsl @@ -80,8 +88,8 @@ def HLSLVkExtBuiltinOutput: InheritableAttr { When this attribute is encountered, several changes will occur: - The address space will be set to `vulkan_private` -- a constructor will be added. -- a destructor will be added. +- a constructor will be added for both input & output builtins. +- a destructor will be added to output builtins. The construction will call a target-specific intrinsic: `spv_load_builtin`. This builtin takes the SPIR-V built-in ID as parameter, and returns the @@ -103,6 +111,9 @@ Corresponding code could be: [[vk::ext_builtin_input(/* GlobalInvocationId */ 28)]] static const uint3 myBuiltIn; +[[vk::ext_builtin_output(/* SomeUnknownOutput */ 1234)]] +static uint3 myOutputBuiltIn; + void main(uint3 thread_id_from_param : SV_DispatchThreadID) { [...] } @@ -111,17 +122,22 @@ constructor_myBuiltIn() { myBuiltIn = spv_load_builtin(28); } -destructor_myBuiltIn() { - spv_store_builtin(28, myBuiltIn); +constructor_myOutputBuiltIn() { + myOutputBuiltIn = spv_load_builtin(1234); +} + +destructor_myOutputBuiltIn() { + spv_store_builtin(1234, myOutputBuiltIn); } void entrypoint() { constructor_myBuiltIn(); + constructor_myOutputBuiltIn(); uint3 input_param1 = spv_load_builtin(28); main(input_param1) - destructor_myBuiltIn(); + destructor_myOutputBuiltIn(); } ``` @@ -130,6 +146,52 @@ meaning it will require the `Private` storage class in SPIR-V. Because of that, globals linked to the added attributes will require a new address space `vulkan_private`. +## What about unused or conditionally loaded Input built-ins? + +Loading a built-in has no side-effect. Hence, the load can be duplicated, or +hoisted in the entry function/global ctor. + +If the variable is loaded, and result is never used, LLVM should be able to +optimize those loads away. Same goes for the remaining unused local variable. + +## What about conditionally stored Output built-ins? + +Output built-ins starts with an undefined value. This means if the shader +doesn't modify this builtin, we should either remove it, or leave it +unchanged. + +The simplest option is to load the initial value, and then allow the shader +to modify it. + +The global value ctor would load the builtin value into the global, and the +global dtor store it back. +From the SPIR-V point of view, loading then storing this undefined value +should not have any impact, as builtin load/stores should not have +side-effects. + +This design at least allows us to have correct code, even if the output +builtin modification is gated behing a condition: + - lanes taking this branch would modify the undefined into a known value. + - lanes not taking this branch would load & store back the undefined value. + +# What about unused Output built-ins? + +The issue the ctor/dtor approach has is it prevents LLVM to eliminate unused +output built-ins. + +Assuming we correctly run `dce`, `always-inline`, `dse`, `deadargelim` passes, +we should end up with an unoptimized `spv_load_builtin()` and +`spv_store_builtin()`. + +Because those are marked `MemRead` and `MemWrite`, LLVM won't be able to +optimize them away. + +Solving could probably be done using a custom IR pass: +- this would consider a given BuiltIn ID to represent a location in memory. +- hence, it could associate the load/store pairs. +- if the loaded value remains unchanged when passed to a store, eliminate + both. + ## Backend changes Backend will translate the new `vulkan_privatge` address space to From edba6c7aba88b90c7ab0d7d448eb6236f5afac8b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 28 Nov 2024 17:15:09 +0100 Subject: [PATCH 4/7] change design to use variables MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- proposals/NNNN-spirv-input-builtin.md | 180 +++++++------------------- 1 file changed, 50 insertions(+), 130 deletions(-) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md index 43a28e4..b0ac3ca 100644 --- a/proposals/NNNN-spirv-input-builtin.md +++ b/proposals/NNNN-spirv-input-builtin.md @@ -17,32 +17,30 @@ Input `BuiltIn` values are private to the executing lane. Reading their value has no side-effect. If not used, those built-in can safely be removed from the shader module (Unlike the inputs tagged with `Location`). -Output 'BuiltIn' values starts with an undefined value. Those are private -to the lane, and storing to them had no other side-effect than to modify the -initially stored value. It is allowed to load those values. - -Example: +Output 'BuiltIn' values are slightly different: + - Their initial value is undefined. Loading them is UB. + - Storing to it has a side effect: removing the pipeline default value. + - Loading the built-in once stored to is defined: it returns the last + set value. +Examples: ```hlsl -float4 vsmain(float4 Position : POSITION) : SV_POSITION -{ - return Position; -} + float a = my_output_builtin * 123.f; // Undefined behavior. + my_output_builtin = my_output_builtin; // Undefined behavior. + my_output_builtin = 0.f; // Replacing pipeline's default value with 0.f. + float b = my_output_builtin; // Defined, b = 0.f; ``` -Both `POSITION` and `SV_POSITION` will be represented as an `OpVariable`: - -- `POSITION` will have the `Input` storage class. -- `SV_POSITION` will have the `Output` storage class. - -- `POSITION` will be decorated with `Location 0`. -- `SV_POSITION` will be decorated with `BuiltIn Position`. - -In addition, users can define their own SPIR-V built-ins using inline SPIR-V: +In HLSL, Input/Output built-ins can be accessed through two methods: ```hlsl [[vk::ext_builtin_input(/* NumWorkGroups */ 24)]] -static const uint3 myBuiltIn; +static const uint3 from_a_global; + +void main(uint3 from_a_semantic : SV_ThreadId) +{ + uint3 a = from_a_semantic + from_a_global; +} ``` This document explain how we plan on implementing those in Clang/LLVM. @@ -56,8 +54,9 @@ attributes: - `HLSLInputBuiltin` - `HLSLOutputBuiltin` -In addition, a new address space will be added: -- `vulkan_private` +In addition, a two new address spaces will be added: +- `hlsl_input` +- `hlsl_output` The TD file will attach each attribute to a `SubjectList` with the following constraints: @@ -87,133 +86,54 @@ def HLSLVkExtBuiltinOutput: InheritableAttr { ``` When this attribute is encountered, several changes will occur: -- The address space will be set to `vulkan_private` -- a constructor will be added for both input & output builtins. -- a destructor will be added to output builtins. - -The construction will call a target-specific intrinsic: `spv_load_builtin`. -This builtin takes the SPIR-V built-in ID as parameter, and returns the -value of the corresponding built-in. +- AS will be set to `hlsl_input` for input built-ins. +- AS will be set to `hlsl_output` for input built-ins. +- a `spirv.Decoration` metadata is added with the `BuiltIn ` decoration. -The destructor will call another intrinsic: `spv_store_builtin`. This builtin -takes the SPIR-V built-in ID, as well as a value to store as parameter. +The AS change will allow the back-end to correctly determine the variable +storage class. +The metadata will be converted `OpDecorate BuiltIn `. -And lastly, to support input parameters with semantics, the same intrinsic -will be called in the generated entrypoint wrapper function. -Because the 2 new intrinsics will be marked as `MemWrite` or `MemRead`, LLVM -won't be able to optimize them away, and the initial/final load/store will -be kept. - -Corresponding code could be: +The same mechanism will be used for semantic inputs, but we'll also create +load/stores in the entry-point wrapper to be equivalent to: ``` [[vk::ext_builtin_input(/* GlobalInvocationId */ 28)]] -static const uint3 myBuiltIn; +static const uint3 dispatch_thread_id; -[[vk::ext_builtin_output(/* SomeUnknownOutput */ 1234)]] -static uint3 myOutputBuiltIn; +[[vk::ext_builtin_output(/* ValidOutputSemantic */ 1234)]] +static uint3 output_semantic; -void main(uint3 thread_id_from_param : SV_DispatchThreadID) { +[numthreads(1, 1, 1)] +uint3 csmain(uint3 id : SV_DispatchThreadID) : SV_SomeValidOutputSemantic { [...] } -constructor_myBuiltIn() { - myBuiltIn = spv_load_builtin(28); -} - -constructor_myOutputBuiltIn() { - myOutputBuiltIn = spv_load_builtin(1234); -} - -destructor_myOutputBuiltIn() { - spv_store_builtin(1234, myOutputBuiltIn); -} - -void entrypoint() { - constructor_myBuiltIn(); - constructor_myOutputBuiltIn(); - - uint3 input_param1 = spv_load_builtin(28); - main(input_param1) - - destructor_myOutputBuiltIn(); +void generated_entrypoint() { + output_semantic = main(dispatch_thread_id); } ``` -The emitted global variable for inline SPIR-V will be in the global scope, -meaning it will require the `Private` storage class in SPIR-V. -Because of that, globals linked to the added attributes will require a new -address space `vulkan_private`. - -## What about unused or conditionally loaded Input built-ins? - -Loading a built-in has no side-effect. Hence, the load can be duplicated, or -hoisted in the entry function/global ctor. - -If the variable is loaded, and result is never used, LLVM should be able to -optimize those loads away. Same goes for the remaining unused local variable. - -## What about conditionally stored Output built-ins? - -Output built-ins starts with an undefined value. This means if the shader -doesn't modify this builtin, we should either remove it, or leave it -unchanged. - -The simplest option is to load the initial value, and then allow the shader -to modify it. - -The global value ctor would load the builtin value into the global, and the -global dtor store it back. -From the SPIR-V point of view, loading then storing this undefined value -should not have any impact, as builtin load/stores should not have -side-effects. - -This design at least allows us to have correct code, even if the output -builtin modification is gated behing a condition: - - lanes taking this branch would modify the undefined into a known value. - - lanes not taking this branch would load & store back the undefined value. - -# What about unused Output built-ins? - -The issue the ctor/dtor approach has is it prevents LLVM to eliminate unused -output built-ins. - -Assuming we correctly run `dce`, `always-inline`, `dse`, `deadargelim` passes, -we should end up with an unoptimized `spv_load_builtin()` and -`spv_store_builtin()`. - -Because those are marked `MemRead` and `MemWrite`, LLVM won't be able to -optimize them away. - -Solving could probably be done using a custom IR pass: -- this would consider a given BuiltIn ID to represent a location in memory. -- hence, it could associate the load/store pairs. -- if the loaded value remains unchanged when passed to a store, eliminate - both. +In have the user's entrypoint returns a struct with semantic on fields, +the entrypoint will have 1 store per semantic, and the module 1 global per +semantic. ## Backend changes -Backend will translate the new `vulkan_privatge` address space to -`StorageClass::Private`. - -The backend will also need to implement 2 new intrinsics: -``` -llvm_any_ty spv_load_builtin(i32 built_in_id); -spv_store_builtin(i32 built_in_id, llvm_any_ty); -``` - -Both will use the GlobalRegistry to get/create a builtin global variable with -the correct decoration. A call to the `load` will generate an Input builtin, -while a call to the `store` will generate an `Output` builtin. - -Only the first lowering of each intrinsic will generate a builtin. - -The load intrinsic will then add an `OpLoad`, while the store an `OpStore`. - -## Draft PR +Backend will translate the new `vulkan_input` address space to +`StorageClass::Input`, and `vulkan_output` to `StorageClass::Output`. -https://github.com/llvm/llvm-project/pull/116393 +Decoration lowering is already available. +No change is required for the entrypoint wrapper. +# FAQ +## Why not follow the DXIL design with load/store functions? +SPIR-V implements built-ins as variables. +Storing to an output built-in has a hidden side-effect on the pipeline. +Implementing this as a global variable is the most natural way to implement +this. Implementing it like DXIL using functions would require tracking those, +and only generating the input/output variable if at least one read/write is +valid. From f72399a4fb5d09436fea88f03b8922fcf622e51a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 2 Dec 2024 11:23:52 +0100 Subject: [PATCH 5/7] pr feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- proposals/NNNN-spirv-input-builtin.md | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md index b0ac3ca..d7570c0 100644 --- a/proposals/NNNN-spirv-input-builtin.md +++ b/proposals/NNNN-spirv-input-builtin.md @@ -49,8 +49,8 @@ This document explain how we plan on implementing those in Clang/LLVM. ## Frontend changes -Global variables marked with Inline SPIR-V will be marked using two new -attributes: +Global variables marked with `vk::ext_builtin_input` or +`vk::ext_builtin_output` will be marked in the AST using two new attributes: - `HLSLInputBuiltin` - `HLSLOutputBuiltin` @@ -86,13 +86,13 @@ def HLSLVkExtBuiltinOutput: InheritableAttr { ``` When this attribute is encountered, several changes will occur: -- AS will be set to `hlsl_input` for input built-ins. -- AS will be set to `hlsl_output` for input built-ins. +- Address space will be set to `hlsl_input` for input built-ins. +- Address space will be set to `hlsl_output` for output built-ins. - a `spirv.Decoration` metadata is added with the `BuiltIn ` decoration. -The AS change will allow the back-end to correctly determine the variable +The address space change will allow the back-end to correctly determine the variable storage class. -The metadata will be converted `OpDecorate BuiltIn `. +The metadata will be converted to `OpDecorate BuiltIn `. The same mechanism will be used for semantic inputs, but we'll also create @@ -115,16 +115,15 @@ void generated_entrypoint() { } ``` -In have the user's entrypoint returns a struct with semantic on fields, -the entrypoint will have 1 store per semantic, and the module 1 global per -semantic. +If the entrypoint returns a struct with semantic on fields, the entrypoint +wrapper will have 1 store per semantic, and the module 1 global per semantic. ## Backend changes -Backend will translate the new `vulkan_input` address space to -`StorageClass::Input`, and `vulkan_output` to `StorageClass::Output`. +The SPIR-V backend will translate the new `hlsl_input` address space to +`StorageClass::Input`, and `hlsl_output` to `StorageClass::Output`. -Decoration lowering is already available. +The SPIR-V backend already accepts the `spirv.Decoration` metadata. No change is required for the entrypoint wrapper. # FAQ From 9e7702d8e62dc24e3e7a6e17298c171193563e21 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Mon, 2 Dec 2024 11:26:23 +0100 Subject: [PATCH 6/7] pr feedback MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Nathan Gauër --- proposals/NNNN-spirv-input-builtin.md | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/NNNN-spirv-input-builtin.md index d7570c0..e2eb3d4 100644 --- a/proposals/NNNN-spirv-input-builtin.md +++ b/proposals/NNNN-spirv-input-builtin.md @@ -11,11 +11,12 @@ to and from a shader, e.g: the system semantic `SV_GroupID` or `MY_SEMANTIC`, a user-defined semantic. In SPIR-V, those are translated to `Input` and `Output` variables, -with either a `BuiltIn` or `Location` decoration. +with either a `BuiltIn` or `Location` decoration. This proposal only +focuses the `BuiltIn` interface variables. Input `BuiltIn` values are private to the executing lane. Reading their value has no side-effect. If not used, those built-in can safely be removed -from the shader module (Unlike the inputs tagged with `Location`). +from the shader module. Output 'BuiltIn' values are slightly different: - Their initial value is undefined. Loading them is UB. From 456f8c794740bc08a2b3eb793a9239e74dcbdb27 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Nathan=20Gau=C3=ABr?= Date: Thu, 6 Mar 2025 13:07:02 +0100 Subject: [PATCH 7/7] add proposal number --- .../{NNNN-spirv-input-builtin.md => 0019-spirv-input-builtin.md} | 0 1 file changed, 0 insertions(+), 0 deletions(-) rename proposals/{NNNN-spirv-input-builtin.md => 0019-spirv-input-builtin.md} (100%) diff --git a/proposals/NNNN-spirv-input-builtin.md b/proposals/0019-spirv-input-builtin.md similarity index 100% rename from proposals/NNNN-spirv-input-builtin.md rename to proposals/0019-spirv-input-builtin.md