diff options
| -rw-r--r-- | source/slang/compiler.h | 6 | ||||
| -rw-r--r-- | source/slang/diagnostic-defs.h | 3 | ||||
| -rw-r--r-- | source/slang/parameter-binding.cpp | 131 | ||||
| -rw-r--r-- | source/slang/type-layout.cpp | 32 | ||||
| -rw-r--r-- | tests/diagnostics/vk-bindings.slang | 16 | ||||
| -rw-r--r-- | tests/diagnostics/vk-bindings.slang.expected | 7 | ||||
| -rw-r--r-- | tests/reflection/parameter-block-explicit-space.slang | 104 | ||||
| -rw-r--r-- | tests/reflection/parameter-block-explicit-space.slang.expected | 103 |
8 files changed, 359 insertions, 43 deletions
diff --git a/source/slang/compiler.h b/source/slang/compiler.h index 7be5b332d..fbd6b3f15 100644 --- a/source/slang/compiler.h +++ b/source/slang/compiler.h @@ -241,6 +241,12 @@ namespace Slang MatrixLayoutMode getDefaultMatrixLayoutMode(); }; + /// Are we generating code for a D3D API? + bool isD3DTarget(TargetRequest* targetReq); + + /// Are we generating code for a Khronos API (OpenGL or Vulkan)? + bool isKhronosTarget(TargetRequest* targetReq); + // Compute the "effective" profile to use when outputting the given entry point // for the chosen code-generation target. // diff --git a/source/slang/diagnostic-defs.h b/source/slang/diagnostic-defs.h index 8466485e7..2a08dfb6a 100644 --- a/source/slang/diagnostic-defs.h +++ b/source/slang/diagnostic-defs.h @@ -368,6 +368,9 @@ DIAGNOSTIC(39009, Error, expectedSpace, "expected 'space', got '$0'") DIAGNOSTIC(39010, Error, expectedSpaceIndex, "expected a register space index after 'space'") DIAGNOSTIC(39011, Error, componentMaskNotSupported, "explicit register component masks are not yet supported in Slang") DIAGNOSTIC(39012, Error, packOffsetNotSupported, "explicit 'packoffset' bindings are not yet supported in Slang") +DIAGNOSTIC(39013, Warning, registerModifierButNoVulkanLayout, "shader parameter '$0' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan") +DIAGNOSTIC(39014, Error, unexpectedSpecifierAfterSpace, "unexpected specifier after register space: '$0'") +DIAGNOSTIC(39015, Error, wholeSpaceParameterRequiresZeroBinding, "shader parameter '$0' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '$1' is not allowed") DIAGNOSTIC(39013, Error, dontExpectOutParametersForStage, "the '$0' stage does not support `out` or `inout` entry point parameters") DIAGNOSTIC(39014, Error, dontExpectInParametersForStage, "the '$0' stage does not support `in` entry point parameters") diff --git a/source/slang/parameter-binding.cpp b/source/slang/parameter-binding.cpp index bc0f5b760..58891a956 100644 --- a/source/slang/parameter-binding.cpp +++ b/source/slang/parameter-binding.cpp @@ -338,7 +338,7 @@ struct SharedParameterBindingContext // TODO: We should eventually strip this down to // just the subset of fields on the target that // can influence layout decisions. - TargetRequest* targetRequest; + TargetRequest* targetRequest = nullptr; LayoutRulesFamilyImpl* defaultLayoutRules; @@ -443,6 +443,36 @@ static void splitNameAndIndex( outDigits = UnownedStringSlice(digitsBegin, digitsEnd); } +LayoutResourceKind findRegisterClassFromName(UnownedStringSlice const& registerClassName) +{ + switch( registerClassName.size() ) + { + case 1: + switch (*registerClassName.begin()) + { + case 'b': return LayoutResourceKind::ConstantBuffer; + case 't': return LayoutResourceKind::ShaderResource; + case 'u': return LayoutResourceKind::UnorderedAccess; + case 's': return LayoutResourceKind::SamplerState; + + default: + break; + } + break; + + case 5: + if( registerClassName == "space" ) + { + return LayoutResourceKind::RegisterSpace; + } + break; + + default: + break; + } + return LayoutResourceKind::None; +} + LayoutSemanticInfo ExtractLayoutSemanticInfo( ParameterBindingContext* context, HLSLLayoutSemantic* semantic) @@ -470,31 +500,9 @@ LayoutSemanticInfo ExtractLayoutSemanticInfo( UnownedStringSlice registerIndexDigits; splitNameAndIndex(registerName, registerClassName, registerIndexDigits); - // All of the register classes we support are single ASCII characters, - // so we really just care about the first byte, but we want to be - // careful and only look at it if the register class name is one - // byte long. - char registerClassChar = registerClassName.size() == 1 ? *registerClassName.begin() : 0; - LayoutResourceKind kind = LayoutResourceKind::None; - switch (registerClassChar) + LayoutResourceKind kind = findRegisterClassFromName(registerClassName); + if(kind == LayoutResourceKind::None) { - case 'b': - kind = LayoutResourceKind::ConstantBuffer; - break; - - case 't': - kind = LayoutResourceKind::ShaderResource; - break; - - case 'u': - kind = LayoutResourceKind::UnorderedAccess; - break; - - case 's': - kind = LayoutResourceKind::SamplerState; - break; - - default: getSink(context)->diagnose(semantic->registerName, Diagnostics::unknownRegisterClass, registerClassName); return info; } @@ -524,13 +532,17 @@ LayoutSemanticInfo ExtractLayoutSemanticInfo( UnownedStringSlice spaceDigits; splitNameAndIndex(spaceName, spaceSpelling, spaceDigits); - if( spaceSpelling != UnownedTerminatedStringSlice("space") ) + if( kind == LayoutResourceKind::RegisterSpace ) + { + getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::unexpectedSpecifierAfterSpace, spaceName); + } + else if( spaceSpelling != UnownedTerminatedStringSlice("space") ) { - getSink(context)->diagnose(semantic->registerName, Diagnostics::expectedSpace, spaceSpelling); + getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::expectedSpace, spaceSpelling); } else if( spaceDigits.size() == 0 ) { - getSink(context)->diagnose(semantic->registerName, Diagnostics::expectedSpaceIndex); + getSink(context)->diagnose(registerSemantic->spaceName, Diagnostics::expectedSpaceIndex); } else { @@ -1491,6 +1503,26 @@ static void addExplicitParameterBindings_HLSL( RefPtr<ParameterInfo> parameterInfo, RefPtr<VarLayout> varLayout) { + // We only want to apply D3D `register` modifiers when compiling for + // D3D targets. + // + // TODO: Nominally, the `register` keyword allows for a shader + // profile to be specified, so that a given binding only + // applies for a specific profile: + // + // https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/dx-graphics-hlsl-variable-register + // + // We might want to consider supporting that syntax in the + // long run, in order to handle bindings for multiple targets + // in a more consistent fashion (whereas using `register` for D3D + // and `[[vk::binding(...)]]` for Vulkan creates a lot of + // visual noise). + // + // For now we do the filtering on target in a very direct fashion: + // + if(!isD3DTarget(context->getTargetRequest())) + return; + auto typeLayout = varLayout->typeLayout; auto varDecl = varLayout->varDecl; @@ -1527,11 +1559,35 @@ static void addExplicitParameterBindings_HLSL( } } +static void maybeDiagnoseMissingVulkanLayoutModifier( + ParameterBindingContext* context, + DeclRef<VarDeclBase> const& varDecl) +{ + // If the user didn't specify a `binding` (and optional `set`) for Vulkan, + // but they *did* specify a `register` for D3D, then that is probably an + // oversight on their part. + if( auto registerModifier = varDecl.getDecl()->FindModifier<HLSLRegisterSemantic>() ) + { + getSink(context)->diagnose(registerModifier, Diagnostics::registerModifierButNoVulkanLayout, varDecl.GetName()); + } +} + static void addExplicitParameterBindings_GLSL( ParameterBindingContext* context, RefPtr<ParameterInfo> parameterInfo, RefPtr<VarLayout> varLayout) { + + // We only want to apply GLSL-style layout modifers + // when compiling for a Khronos-related target. + // + // TODO: This should have some finer granularity + // so that we are able to distinguish between + // Vulkan and OpenGL as targets. + // + if(!isKhronosTarget(context->getTargetRequest())) + return; + auto typeLayout = varLayout->typeLayout; auto varDecl = varLayout->varDecl; @@ -1556,11 +1612,28 @@ static void addExplicitParameterBindings_GLSL( auto attr = varDecl.getDecl()->FindModifier<GLSLBindingAttribute>(); if (!attr) { + maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl); return; } semanticInfo.index = attr->binding; semanticInfo.space = attr->set; } + else if( (resInfo = typeLayout->FindResourceInfo(LayoutResourceKind::RegisterSpace)) != nullptr ) + { + // Try to find `set` + auto attr = varDecl.getDecl()->FindModifier<GLSLBindingAttribute>(); + if (!attr) + { + maybeDiagnoseMissingVulkanLayoutModifier(context, varDecl); + return; + } + if( attr->binding != 0) + { + getSink(context)->diagnose(attr, Diagnostics::wholeSpaceParameterRequiresZeroBinding, varDecl.GetName(), attr->binding); + } + semanticInfo.index = attr->set; + semanticInfo.space = 0; + } else if( (resInfo = typeLayout->FindResourceInfo(LayoutResourceKind::VertexInput)) != nullptr ) { // Try to find `location` binding @@ -2564,6 +2637,7 @@ void generateParameterBindings( sharedContext.compileRequest = compileReq; sharedContext.defaultLayoutRules = layoutContext.getRulesFamily(); sharedContext.programLayout = programLayout; + sharedContext.targetRequest = targetReq; // Create a sub-context to collect parameters that get // declared into the global scope @@ -2816,6 +2890,7 @@ RefPtr<ProgramLayout> specializeProgramLayout( sharedContext.compileRequest = targetReq->compileRequest; sharedContext.defaultLayoutRules = layoutContext.getRulesFamily(); sharedContext.programLayout = newProgramLayout; + sharedContext.targetRequest = targetReq; // Create a sub-context to collect parameters that get // declared into the global scope diff --git a/source/slang/type-layout.cpp b/source/slang/type-layout.cpp index 41476a7e5..4e9a05b53 100644 --- a/source/slang/type-layout.cpp +++ b/source/slang/type-layout.cpp @@ -823,7 +823,7 @@ static bool isOpenGLTarget(TargetRequest*) return false; } -static bool isD3DTarget(TargetRequest* targetReq) +bool isD3DTarget(TargetRequest* targetReq) { switch( targetReq->target ) { @@ -839,6 +839,20 @@ static bool isD3DTarget(TargetRequest* targetReq) } } +bool isKhronosTarget(TargetRequest* targetReq) +{ + switch( targetReq->target ) + { + default: + return false; + + case CodeGenTarget::GLSL: + case CodeGenTarget::SPIRV: + case CodeGenTarget::SPIRVAssembly: + return true; + } +} + static bool isD3D11Target(TargetRequest*) { // We aren't officially supporting D3D11 right now @@ -886,21 +900,9 @@ static bool isSM5_1OrLater(TargetRequest* targetReq) static bool isVulkanTarget(TargetRequest* targetReq) { - switch( targetReq->target ) - { - default: - return false; - - case CodeGenTarget::GLSL: - case CodeGenTarget::SPIRV: - case CodeGenTarget::SPIRVAssembly: - break; - } - - // For right now, any GLSL-related target is assumed + // For right now, any Khronos-related target is assumed // to be a Vulkan target. - - return true; + return isKhronosTarget(targetReq); } static bool shouldAllocateRegisterSpaceForParameterBlock( diff --git a/tests/diagnostics/vk-bindings.slang b/tests/diagnostics/vk-bindings.slang new file mode 100644 index 000000000..f5a8bdeba --- /dev/null +++ b/tests/diagnostics/vk-bindings.slang @@ -0,0 +1,16 @@ +// vk-bindings.slang + +//TEST:SIMPLE:-target spirv + +// D3D `register` without VK binding +Texture2D t : register(t0); + +struct S { float4 a; }; + +// Parameter block with non-zero binding: +[[vk::binding(2,1)]] +ParameterBlock<S> b; + +[shader("compute")] +void main() +{}
\ No newline at end of file diff --git a/tests/diagnostics/vk-bindings.slang.expected b/tests/diagnostics/vk-bindings.slang.expected new file mode 100644 index 000000000..8e085b55f --- /dev/null +++ b/tests/diagnostics/vk-bindings.slang.expected @@ -0,0 +1,7 @@ +result code = -1 +standard error = { +tests/diagnostics/vk-bindings.slang(6): warning 39013: shader parameter 't' has a 'register' specified for D3D, but no '[[vk::binding(...)]]` specified for Vulkan +tests/diagnostics/vk-bindings.slang(11): error 39015: shader parameter 'b' consumes whole descriptor sets, so the binding must be in the form '[[vk::binding(0, ...)]]'; the non-zero binding '2' is not allowed +} +standard output = { +} diff --git a/tests/reflection/parameter-block-explicit-space.slang b/tests/reflection/parameter-block-explicit-space.slang new file mode 100644 index 000000000..5679a1c35 --- /dev/null +++ b/tests/reflection/parameter-block-explicit-space.slang @@ -0,0 +1,104 @@ +// parameter-block-explicit-space.slang + +//TEST:REFLECTION:-D__SLANG__ -stage fragment -entry main -profile sm_5_1 -target hlsl +//TEST:COMPARE_HLSL:-stage fragment -entry main -profile sm_5_1 + +#ifdef __SLANG__ +struct A +{ + float4 au; + Texture2D at1; + Texture2D at2; + SamplerState as; +} + +struct X +{ + float4 xu; +} + +struct B +{ + float4 bu; + Texture2D bt; + SamplerState bs; + + // TODO: This line leads to a crash +// ConstantBuffer<X> bx; +} + + + +[[vk::binding(0,2)]] +ParameterBlock<A> a : register(space2); + +[[vk::binding(0,3)]] +ParameterBlock<B> b : register(space3); + +float4 use(float4 val) { return val; } +float4 use(Texture2D t, SamplerState s) +{ + return t.Sample(s, 0.0); +} + +float4 main() : SV_Target +{ + return use(a.au) + + use(a.at1, a.as) + + use(a.at2, a.as) + + use(b.bu) + + use(b.bt, b.bs) + // + use(b.bx.xu) + ; +} + +#else + +#define A A_0 +#define a a_0 +#define au au_0 +#define at1 a_at1_0 +#define at2 a_at2_0 +#define as a_as_0 + +#define B B_0 +#define b b_0 +#define bu bu_0 +#define bt b_bt_0 +#define bs b_bs_0 + +struct A +{ + float4 au; +}; +cbuffer _S1 : register(b0, space2) +{ A a; } +Texture2D at1 : register(t0, space2); +Texture2D at2 : register(t1, space2); +SamplerState as : register(s0, space2); + +struct B +{ + float4 bu; +}; +cbuffer _S3 : register(b0, space3) +{ B b; } +Texture2D bt : register(t0, space3); +SamplerState bs : register(s0, space3); + +float4 use(float4 val) { return val; } +float4 use(Texture2D t, SamplerState s) +{ + return t.Sample(s, 0.0); +} + +float4 main() : SV_TARGET +{ + return use(a.au) + + use(at1, as) + + use(at2, as) + + use(b.bu) + + use(bt, bs); +} + +#endif diff --git a/tests/reflection/parameter-block-explicit-space.slang.expected b/tests/reflection/parameter-block-explicit-space.slang.expected new file mode 100644 index 000000000..e683a641f --- /dev/null +++ b/tests/reflection/parameter-block-explicit-space.slang.expected @@ -0,0 +1,103 @@ +result code = 0 +standard error = { +} +standard output = { +{ + "parameters": [ + { + "name": "a", + "binding": {"kind": "constantBuffer", "space": 2, "index": 0, "count": 0}, + "type": { + "kind": "parameterBlock", + "elementType": { + "kind": "struct", + "name": "A", + "fields": [ + { + "name": "au", + "type": { + "kind": "vector", + "elementCount": 4, + "elementType": { + "kind": "scalar", + "scalarType": "float32" + } + }, + "binding": {"kind": "uniform", "offset": 0, "size": 16} + }, + { + "name": "at1", + "type": { + "kind": "resource", + "baseShape": "texture2D" + }, + "binding": {"kind": "shaderResource", "index": 0} + }, + { + "name": "at2", + "type": { + "kind": "resource", + "baseShape": "texture2D" + }, + "binding": {"kind": "shaderResource", "index": 1} + }, + { + "name": "as", + "type": { + "kind": "samplerState" + }, + "binding": {"kind": "samplerState", "index": 0} + } + ] + } + } + }, + { + "name": "b", + "binding": {"kind": "constantBuffer", "space": 3, "index": 0, "count": 0}, + "type": { + "kind": "parameterBlock", + "elementType": { + "kind": "struct", + "name": "B", + "fields": [ + { + "name": "bu", + "type": { + "kind": "vector", + "elementCount": 4, + "elementType": { + "kind": "scalar", + "scalarType": "float32" + } + }, + "binding": {"kind": "uniform", "offset": 0, "size": 16} + }, + { + "name": "bt", + "type": { + "kind": "resource", + "baseShape": "texture2D" + }, + "binding": {"kind": "shaderResource", "index": 0} + }, + { + "name": "bs", + "type": { + "kind": "samplerState" + }, + "binding": {"kind": "samplerState", "index": 0} + } + ] + } + } + } + ], + "entryPoints": [ + { + "name": "main", + "stage:": "fragment" + } + ] +} +} |
