From 54d3c5f6657b1099326e1ce7ec0f0692e7025442 Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Fri, 6 Dec 2019 09:29:09 -0800 Subject: Remove legacy feature for merging global shader parameters (#1139) * Remove legacy feature for merging global shader parameters There is a fair amount of special-case code in the Slang compiler today to deal with the scenario where a programmer declares the "same" shader parameter across two different translation units: ```hlsl // a.hlsl Texture2D a; cbuffer C { float4 c; } ``` ```hlsl // b.hlsl cbuffer C { float4 c; } Texture2D b; ``` An important note here is that the declaration of `C` may be in a header file that both `a.hlsl` and `b.hlsl` `#include`, because from the standpoint of the parser and later stages of the compiler, there is no difference between `C` being in an included file vs. it being copy-pasted across both `a.hlsl` and `b.hlsl`. When a user invokes `slangc a.hlsl b.hlsl` (or the equivalent via the API), then they may decide that it is "obvious" that the shader parameter `C` is the "same" in both `a.hlsl` and `b.hlsl`. Knowing that the parameter is the "same" may lead them to make certain assumptions: * They may assume that generated code for entry points in `a.hlsl` and `b.hlsl` will both agree on the exact `register`/`binding` occupied by `C`. * They may assume that reflection information for their program will only reflect `C` once, and it will reflect it in a way that is applicable to entry points in both `a.hlsl` and `b.hlsl` * They may assume that the compiler can and should handle this use case even when `C` contains fields with `struct` types that are declared in both `a.hlsl` and `b.hlsl` that have the "same" definition. * They may assume that in cases where `C` is declared inconsistently between `a.hlsl` and `b.hlsl` the compiler can and will diagnose an error. Making these assumptions work in practice required a lot of special-case code: * When composing/linking programs was `ComponentType`s we had to include a special case `LegacyProgram` type that could provide these "do what I mean" semantics, since they are *not* what one would want in the general case for a `CompositeComponentType`. * During enumeration of global shader parameter in a `LegacyProgram`, we had to detect parameters from distinct modules (translation units) with the same name, and then enforce that they must have the "same" type (via an ad hoc recursive structural type match). No other semantic checking logic needs or uses that kind of structural check. * During parameter binding generation, we need to handle the case where a single global shader parameter might have multiple declarations, and make sure to collect explicit bindings from all of them (checking for inconsistency) and also to apply generated bindings to all of them. * The `mapVarToLayout` member in `StructTypeLayout` is a concession to the fact that we might have multiple `VarDecl`s for each field of the struct that represents the global scope, we might need to look up a field and its layout using any of those declarations (much of the need for this field had gone away now that IR passes are largely using IR-based layout). All of these different special cases added more complex code in many places in the compiler, all to support a scenario that isn't especially common. Most users won't be affected by the original issue, because they will do one of several things that rule it out: * Anybody using `slangc` like a stand-in for `fxc` or `dxc` and compiling one translation unit at a time will not suffer from any problems. If/when such users want consistent bindings across translation units, they already use either explicit binding or rely on consistent ordering and implicit binding. * Anybody who puts all the entry points that get combined into a pass/pipeline in a single file will not have problems. They will automatically get consistent bindings because of Slang's guarantees, and there can't be duplicated declarations when there is only one translation unit. * Anybody using `import` to factor out common declarations while compiling multiple translation units at once will not be affected. Parameters declared in an `import`ed module are the "same" in a much deeper way that it is trivial for Slang to support. Only users of the Falcor framework are likely to be affected by this, and they have two easy migration paths: either put related entry points into the same file, or factor common parameters into an `import`ed module. (It is also worth noting that for command-line `slangc`, it is possible to have a single module with multiple `.slang` files in it, which can all see global declarations like parameters across all the files. Anybody who buys into doing things the Slang Way should have no problem avoiding duplicated declarations) With the rationale out of the way, the actual change mostly just amounts to deleting lots of code that is no longer needed. An astute reviewer might notice several `assert`-fail conditions where complex Slang features were never actually made to work correctly with this legacy behavior. A small number of test cases broke with the code changes, but these were tests that specifically exercised the behavior being removed. In the case of the tests around binding/reflection generating, I rewrote the tests to use one of the idomatic workarounds (putting the shared parameters into an `import`ed module), but doing so required me to add support for `#include` when doing pass-through compilation with `fxc`. That logic added a bit more cruft than I had originally hoped to this commit, but having `#include` support when doing pass-through compilation is probably a net win. * fixup: 64-bit warning --- source/slang/slang-parameter-binding.cpp | 202 ++++++++----------------------- 1 file changed, 52 insertions(+), 150 deletions(-) (limited to 'source/slang/slang-parameter-binding.cpp') diff --git a/source/slang/slang-parameter-binding.cpp b/source/slang/slang-parameter-binding.cpp index 9e11152c4..5cd4156c3 100644 --- a/source/slang/slang-parameter-binding.cpp +++ b/source/slang/slang-parameter-binding.cpp @@ -306,13 +306,13 @@ struct UsedRangeSet : RefObject // Information on a single parameter struct ParameterInfo : RefObject { - // Layout info for the concrete variables that will make up this parameter - List> varLayouts; + // Layout info for the variable that represents this parameter + RefPtr varLayout; ParameterBindingInfo bindingInfo[kLayoutResourceKindCount]; // The translation unit this parameter is specific to, if any - TranslationUnitRequest* translationUnit = nullptr; +// TranslationUnitRequest* translationUnit = nullptr; ParameterInfo() { @@ -696,9 +696,9 @@ static RefPtr _createVarLayout( // Collect a single declaration into our set of parameters static void collectGlobalScopeParameter( - ParameterBindingContext* context, - GlobalShaderParamInfo const& shaderParamInfo, - SubstitutionSet globalGenericSubst) + ParameterBindingContext* context, + ShaderParamInfo const& shaderParamInfo, + SubstitutionSet globalGenericSubst) { auto varDeclRef = shaderParamInfo.paramDeclRef; @@ -722,7 +722,7 @@ static void collectGlobalScopeParameter( // Now create a variable layout that we can use RefPtr varLayout = _createVarLayout(typeLayout, varDeclRef); - // The logic in `check.cpp` that created the `GlobalShaderParamInfo` + // The logic in `check.cpp` that created the `ShaderParamInfo` // will have identified any cases where there might be multiple // global variables that logically represent the same shader parameter. // @@ -734,30 +734,10 @@ static void collectGlobalScopeParameter( ParameterInfo* parameterInfo = new ParameterInfo(); context->shared->parameters.add(parameterInfo); - // Add the first variable declaration to the list of declarations for the parameter - parameterInfo->varLayouts.add(varLayout); - - // Add any additional variables to the list of declarations - for( auto additionalVarDeclRef : shaderParamInfo.additionalParamDeclRefs ) - { - // TODO: We should either eliminate the design choice where different - // declarations of the "same" shade parameter get merged across - // translation units (it is effectively just a compatiblity feature), - // or we should clean things up earlier in the chain so that we can - // re-use a single `VarLayout` across all of the different declarations. - // - // TODO: It would also make sense in these cases to ensure that - // such global shader parameters get the same mangled name across - // all translation units, so that they can automatically be collapsed - // during linking. - - RefPtr additionalVarLayout = new VarLayout(); - additionalVarLayout->typeLayout = typeLayout; - additionalVarLayout->varDecl = additionalVarDeclRef; - additionalVarLayout->pendingVarLayout = varLayout->pendingVarLayout; - - parameterInfo->varLayouts.add(additionalVarLayout); - } + // Add the created var layout to the parameter information structure, + // so that we can update it as we proceed with parameter binding. + // + parameterInfo->varLayout = varLayout; } static RefPtr findUsedRangeSetForSpace( @@ -831,12 +811,6 @@ static void addExplicitParameterBinding( || bindingInfo.space != semanticInfo.space ) { getSink(context)->diagnose(varDecl, Diagnostics::conflictingExplicitBindingsForParameter, getReflectionName(varDecl)); - - auto firstVarDecl = parameterInfo->varLayouts[0]->varDecl.getDecl(); - if( firstVarDecl != varDecl ) - { - getSink(context)->diagnose(firstVarDecl, Diagnostics::seeOtherDeclarationOf, getReflectionName(firstVarDecl)); - } } // TODO(tfoley): `register` semantics can technically be @@ -859,13 +833,13 @@ static void addExplicitParameterBinding( markSpaceUsed(context, semanticInfo.space); } auto overlappedVarLayout = usedRangeSet->usedResourceRanges[(int)semanticInfo.kind].Add( - parameterInfo->varLayouts[0], + parameterInfo->varLayout, semanticInfo.index, semanticInfo.index + count); if (overlappedVarLayout) { - auto paramA = parameterInfo->varLayouts[0]->varDecl.getDecl(); + auto paramA = parameterInfo->varLayout->varDecl.getDecl(); auto paramB = overlappedVarLayout->varDecl.getDecl(); auto& diagnosticInfo = Diagnostics::parameterBindingsOverlap; @@ -1052,19 +1026,24 @@ void generateParameterBindings( ParameterBindingContext* context, RefPtr parameterInfo) { - // There must be at least one declaration for the parameter. - SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.getCount() != 0); + // There must have been a declaration for the parameter. + SLANG_RELEASE_ASSERT(parameterInfo->varLayout); - // Iterate over all declarations looking for explicit binding information. - for( auto& varLayout : parameterInfo->varLayouts ) - { - // Handle HLSL `register` and `packoffset` modifiers - addExplicitParameterBindings_HLSL(context, parameterInfo, varLayout); + // We will look for explicit binding information on the declaration. + auto varLayout = parameterInfo->varLayout; + // Handle HLSL `register` and `packoffset` modifiers + addExplicitParameterBindings_HLSL(context, parameterInfo, varLayout); - // Handle GLSL `layout` modifiers - addExplicitParameterBindings_GLSL(context, parameterInfo, varLayout); - } + + // Handle GLSL `layout` modifiers and `[vk::...]` attributes. + // + // TODO: We should deprecate the support for `layout` and then rename + // these `_HLSL` and `_GLSL` functions to be more explicit and clear + // about the fact that they are specific to the *target* and not to + // the *source language* (as they were at one point). + // + addExplicitParameterBindings_GLSL(context, parameterInfo, varLayout); } // Generate the binding information for a shader parameter. @@ -1292,17 +1271,12 @@ static void completeBindingsForParameter( ParameterBindingContext* context, RefPtr parameterInfo) { - // We will use the first declaration of the parameter as - // a stand-in for all the declarations, so it is important - // that earlier code has validated that the declarations - // "match". - - SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.getCount() != 0); - auto firstVarLayout = parameterInfo->varLayouts.getFirst(); + auto varLayout = parameterInfo->varLayout; + SLANG_RELEASE_ASSERT(varLayout); completeBindingsForParameterImpl( context, - firstVarLayout, + varLayout, parameterInfo->bindingInfo, parameterInfo); @@ -1310,10 +1284,7 @@ static void completeBindingsForParameter( // all the relevant resource kinds, so we can apply these to the // declarations: - for(auto& varLayout : parameterInfo->varLayouts) - { - applyBindingInfoToParameter(varLayout, parameterInfo->bindingInfo); - } + applyBindingInfoToParameter(varLayout, parameterInfo->bindingInfo); } static void completeBindingsForParameter( @@ -1925,11 +1896,10 @@ struct ScopeLayoutBuilder } void _addParameter( - RefPtr firstVarLayout, - ParameterInfo* parameterInfo) + RefPtr varLayout) { // Does the parameter have any uniform data? - auto layoutInfo = firstVarLayout->typeLayout->FindResourceInfo(LayoutResourceKind::Uniform); + auto layoutInfo = varLayout->typeLayout->FindResourceInfo(LayoutResourceKind::Uniform); LayoutSize uniformSize = layoutInfo ? layoutInfo->count : 0; if( uniformSize != 0 ) { @@ -1937,44 +1907,24 @@ struct ScopeLayoutBuilder UniformLayoutInfo fieldInfo( uniformSize, - firstVarLayout->typeLayout->uniformAlignment); + varLayout->typeLayout->uniformAlignment); LayoutSize uniformOffset = m_rules->AddStructField( &m_structLayoutInfo, fieldInfo); - if( parameterInfo ) - { - for( auto& varLayout : parameterInfo->varLayouts ) - { - varLayout->findOrAddResourceInfo(LayoutResourceKind::Uniform)->index = uniformOffset.getFiniteValue(); - } - } - else - { - firstVarLayout->findOrAddResourceInfo(LayoutResourceKind::Uniform)->index = uniformOffset.getFiniteValue(); - } + varLayout->findOrAddResourceInfo(LayoutResourceKind::Uniform)->index = uniformOffset.getFiniteValue(); } - m_structLayout->fields.add(firstVarLayout); + m_structLayout->fields.add(varLayout); - if( parameterInfo ) - { - for( auto& varLayout : parameterInfo->varLayouts ) - { - m_structLayout->mapVarToLayout.Add(varLayout->varDecl.getDecl(), varLayout); - } - } - else - { - m_structLayout->mapVarToLayout.Add(firstVarLayout->varDecl.getDecl(), firstVarLayout); - } + m_structLayout->mapVarToLayout.Add(varLayout->varDecl.getDecl(), varLayout); } void addParameter( RefPtr varLayout) { - _addParameter(varLayout, nullptr); + _addParameter(varLayout); // Any "pending" items on a field type become "pending" items // on the overall `struct` type layout. @@ -1997,17 +1947,17 @@ struct ScopeLayoutBuilder void addParameter( ParameterInfo* parameterInfo) { - SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.getCount() != 0); - auto firstVarLayout = parameterInfo->varLayouts.getFirst(); + auto varLayout = parameterInfo->varLayout; + SLANG_RELEASE_ASSERT(varLayout); - _addParameter(firstVarLayout, parameterInfo); + _addParameter(varLayout); // Global parameters will have their non-orindary/uniform // pending data handled by the main parameter binding // logic, but we still need to construct a layout // that includes any pending data. // - if(auto fieldPendingVarLayout = firstVarLayout->pendingVarLayout) + if(auto fieldPendingVarLayout = varLayout->pendingVarLayout) { auto fieldPendingTypeLayout = fieldPendingVarLayout->typeLayout; @@ -2404,13 +2354,6 @@ struct CollectGlobalGenericArgumentsVisitor : ComponentTypeVisitor { specialized->getBaseComponentType()->acceptVisitor(this, specialized->getSpecializationInfo()); } - - void visitLegacy(LegacyProgram* legacy, CompositeComponentType::CompositeSpecializationInfo* specializationInfo) SLANG_OVERRIDE - { - // TODO: Need to do something in this case... - SLANG_UNUSED(legacy); - SLANG_UNUSED(specializationInfo); - } }; /// Collect an ordered list of all the specialization arguments given for global generic specialization parameters in `program`. @@ -2563,34 +2506,6 @@ struct CollectParametersVisitor : ComponentTypeVisitor } } - - void visitLegacy(LegacyProgram* legacy, CompositeComponentType::CompositeSpecializationInfo* specializationInfo) SLANG_OVERRIDE - { - // A legacy program is also a leaf case, and we - // can enumerate its parameters directly. - // - // Note: there is a mismatch here where we really - // ought to be tracking specialization arguments - // for a `LegacyProgram` akin to how they are - // tracked for a `Module`, but right now we try - // to do it like a `CompositeComponentType`. - // As a result we are just ignoring specialization - // information here, which will lead to incorrect - // results if somebody every uses specialization - // together with the "legacy" program case. - // - // TODO: eliminate this problem by getting rid of - // `LegacyProgram`, rather than spend time trying - // to make this corner case actually work. - // - SLANG_UNUSED(specializationInfo); - - auto paramCount = legacy->getShaderParamCount(); - for(Index pp = 0; pp < paramCount; ++pp) - { - collectGlobalScopeParameter(m_context, legacy->getShaderParam(pp), SubstitutionSet()); - } - } }; /// Recursively collect the global shader parameters and entry points in `program`. @@ -2751,13 +2666,6 @@ struct CompleteBindingsVisitor : ComponentTypeVisitor visitLeafParams(module); } - void visitLegacy(LegacyProgram* legacy, CompositeComponentType::CompositeSpecializationInfo* specializationInfo) SLANG_OVERRIDE - { - SLANG_UNUSED(specializationInfo); - // A legacy program is a leaf case: we just want to visit each parameter. - visitLeafParams(legacy); - } - void visitLeafParams(ComponentType* componentType) { auto paramCount = componentType->getShaderParamCount(); @@ -2881,12 +2789,6 @@ struct FlushPendingDataVisitor : ComponentTypeVisitor visitLeafParams(module); } - void visitLegacy(LegacyProgram* legacy, CompositeComponentType::CompositeSpecializationInfo* specializationInfo) SLANG_OVERRIDE - { - SLANG_UNUSED(specializationInfo); - visitLeafParams(legacy); - } - void visitLeafParams(ComponentType* componentType) { // In the "leaf" case we just allocate space for any @@ -2897,9 +2799,9 @@ struct FlushPendingDataVisitor : ComponentTypeVisitor { auto globalParamIndex = m_counters->globalParamCounter++; auto globalParamInfo = m_context->shared->parameters[globalParamIndex]; - auto firstVarLayout = globalParamInfo->varLayouts[0]; + auto varLayout = globalParamInfo->varLayout; - _allocateBindingsForPendingData(m_context, firstVarLayout->pendingVarLayout); + _allocateBindingsForPendingData(m_context, varLayout->pendingVarLayout); } } @@ -3072,14 +2974,14 @@ RefPtr generateParameterBindings( { for( auto& parameterInfo : sharedContext.parameters ) { - SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.getCount() != 0); - auto firstVarLayout = parameterInfo->varLayouts.getFirst(); + auto varLayout = parameterInfo->varLayout; + SLANG_RELEASE_ASSERT(varLayout); // Does the field have any uniform data? - if( firstVarLayout->typeLayout->FindResourceInfo(LayoutResourceKind::Uniform) ) + if( varLayout->typeLayout->FindResourceInfo(LayoutResourceKind::Uniform) ) { needDefaultConstantBuffer = true; - diagnoseGlobalUniform(&sharedContext, firstVarLayout->varDecl); + diagnoseGlobalUniform(&sharedContext, varLayout->varDecl); } } } @@ -3102,12 +3004,12 @@ RefPtr generateParameterBindings( // for (auto& parameterInfo : sharedContext.parameters) { - SLANG_RELEASE_ASSERT(parameterInfo->varLayouts.getCount() != 0); - auto firstVarLayout = parameterInfo->varLayouts.getFirst(); + auto varLayout = parameterInfo->varLayout; + SLANG_RELEASE_ASSERT(varLayout); // For each parameter, we will look at each resource it consumes. // - for (auto resInfo : firstVarLayout->typeLayout->resourceInfos) + for (auto resInfo : varLayout->typeLayout->resourceInfos) { // We don't care about whole register spaces/sets, since // we don't need to allocate a default space/set for a parameter -- cgit v1.2.3