From f4ac13d6718d6433f69eb21311110c8225a95aee Mon Sep 17 00:00:00 2001 From: Tim Foley Date: Fri, 14 Jul 2017 12:00:29 -0700 Subject: Adjust type layout when parameter block constains member using the same resource If we have something like to following in HLSL: cbuffer C { Texture2D t; ... } and we are compiling to GLSL, then both `C` and `C.t` consume the same kind of resource (a descriptor-table slot). The way reflection was working right now, querying the index of `C` would return its binding (let's say it is `4` just to be concrete) and then a query on `C::t` would give its offset, which was being computed as `0` because it is the first field in the logical `struct` type. That obviously leads to bad math and requires some subtle `+1`s in cases to get things right (e.g., when scalaring during lowering, I had to carefully add one in some cases). It is unreasonable to expect users to deal with this. This commit changes it so that the offset of field `C::t` is `1` so that hopefully more things Just Work. The special-case logic in lowering is now gone. One important catch here is that this pretty much only works in the case where the element type of a parameter block is a `struct` type (which is really all that makes sense right now). If we ever want to generalize this in the future, then it will probably be necessary to change the `TypeLayout` case for parameter blocks to store a `VarLayout` for the element, rather than just a `TypeLayout`. --- source/slang/lower.cpp | 35 ----------- source/slang/parameter-binding.cpp | 5 +- source/slang/type-layout.cpp | 121 +++++++++++++++++++++++++++---------- source/slang/type-layout.h | 15 +++-- 4 files changed, 101 insertions(+), 75 deletions(-) diff --git a/source/slang/lower.cpp b/source/slang/lower.cpp index c7278201a..caa1ab075 100644 --- a/source/slang/lower.cpp +++ b/source/slang/lower.cpp @@ -1828,25 +1828,6 @@ struct LoweringVisitor needsOffset = true; break; } - - // Even if the base offset of the parent is zero, we may still - // need to offset the child, because the parent consumes a - // resources of the same kind... - if (primaryVarLayout->typeLayout->type->As()) - { - switch (rr.kind) - { - default: - break; - - case LayoutResourceKind::ConstantBuffer: - case LayoutResourceKind::DescriptorTableSlot: - needsOffset = true; - break; - } - if (needsOffset) - break; - } } } if (needsOffset) @@ -1867,22 +1848,6 @@ struct LoweringVisitor { newResInfo->index += parentInfo->index; newResInfo->space += parentInfo->space; - - // Very special-case hack to deal with the case where the parent - // itself consumes a resources of the same type as the field. - if (primaryVarLayout->typeLayout->type->As()) - { - switch (resInfo.kind) - { - default: - break; - - case LayoutResourceKind::ConstantBuffer: - case LayoutResourceKind::DescriptorTableSlot: - newResInfo->index += 1; - break; - } - } } } diff --git a/source/slang/parameter-binding.cpp b/source/slang/parameter-binding.cpp index 01727fb36..007c023e2 100644 --- a/source/slang/parameter-binding.cpp +++ b/source/slang/parameter-binding.cpp @@ -1342,8 +1342,9 @@ void generateParameterBindings( { auto globalConstantBufferLayout = createParameterBlockTypeLayout( nullptr, - globalScopeStructLayout, - globalScopeRules); + globalScopeRules, + globalScopeRules->GetObjectLayout(ShaderParameterKind::ConstantBuffer), + globalScopeStructLayout); globalScopeLayout = globalConstantBufferLayout; } diff --git a/source/slang/type-layout.cpp b/source/slang/type-layout.cpp index 2e0f3035a..6ca2d68fb 100644 --- a/source/slang/type-layout.cpp +++ b/source/slang/type-layout.cpp @@ -681,28 +681,14 @@ static SimpleLayoutInfo getParameterBlockLayoutInfo( RefPtr createParameterBlockTypeLayout( RefPtr parameterBlockType, - RefPtr elementTypeLayout, - LayoutRulesImpl* rules) + LayoutRulesImpl* parameterBlockRules, + SimpleLayoutInfo parameterBlockInfo, + RefPtr elementTypeLayout) { - SimpleLayoutInfo info; - if (parameterBlockType) - { - info = getParameterBlockLayoutInfo( - parameterBlockType, - rules); - } - else - { - // If there is no concrete type, then it seems like we are - // being asked to compute layout for the global scope - info = rules->GetObjectLayout(ShaderParameterKind::ConstantBuffer); - } - - auto typeLayout = new ParameterBlockTypeLayout(); typeLayout->type = parameterBlockType; - typeLayout->rules = rules; + typeLayout->rules = parameterBlockRules; typeLayout->elementTypeLayout = elementTypeLayout; @@ -711,7 +697,7 @@ createParameterBlockTypeLayout( // originally (which should be a single binding "slot" // and hence no uniform data). // - typeLayout->uniformAlignment = info.alignment; + typeLayout->uniformAlignment = parameterBlockInfo.alignment; assert(!typeLayout->FindResourceInfo(LayoutResourceKind::Uniform)); assert(typeLayout->uniformAlignment == 1); @@ -725,11 +711,11 @@ createParameterBlockTypeLayout( // Make sure that we allocate resource usage for the // parameter block itself. - if( info.size ) + if( parameterBlockInfo.size ) { typeLayout->addResourceUsage( - info.kind, - info.size); + parameterBlockInfo.kind, + parameterBlockInfo.size); } // Now, if the element type itself had any resources, then @@ -749,6 +735,48 @@ createParameterBlockTypeLayout( return typeLayout; } +RefPtr +createParameterBlockTypeLayout( + RefPtr parameterBlockType, + LayoutRulesImpl* parameterBlockRules, + RefPtr elementType, + LayoutRulesImpl* elementTypeRules) +{ + // First compute resource usage of the block itself. + // For now we assume that the layout of the block can + // always be described in a `SimpleLayoutInfo` (only + // a single resource kind consumed). + SimpleLayoutInfo info; + if (parameterBlockType) + { + info = getParameterBlockLayoutInfo( + parameterBlockType, + parameterBlockRules); + } + else + { + // If there is no concrete type, then it seems like we are + // being asked to compute layout for the global scope + info = parameterBlockRules->GetObjectLayout(ShaderParameterKind::ConstantBuffer); + } + + // Now compute a layout for the elements of the parameter block. + // Note that we need to be careful and deal with the case where + // the elements of the block use the same resource kind consumed + // by the block itself. + + auto elementTypeLayout = CreateTypeLayout( + elementType, + elementTypeRules, + info); + + return createParameterBlockTypeLayout( + parameterBlockType, + parameterBlockRules, + info, + elementTypeLayout); +} + LayoutRulesImpl* getParameterBufferElementTypeLayoutRules( RefPtr parameterBlockType, LayoutRulesImpl* rules) @@ -783,22 +811,20 @@ LayoutRulesImpl* getParameterBufferElementTypeLayoutRules( RefPtr createParameterBlockTypeLayout( RefPtr parameterBlockType, - LayoutRulesImpl* rules) + LayoutRulesImpl* parameterBlockRules) { // Determine the layout rules to use for the contents of the block - auto parameterBlockLayoutRules = getParameterBufferElementTypeLayoutRules( + auto elementTypeRules = getParameterBufferElementTypeLayoutRules( parameterBlockType, - rules); + parameterBlockRules); - // Create and save type layout for the buffer contents. - auto elementTypeLayout = CreateTypeLayout( - parameterBlockType->elementType.Ptr(), - parameterBlockLayoutRules); + auto elementType = parameterBlockType->elementType; return createParameterBlockTypeLayout( parameterBlockType, - elementTypeLayout, - rules); + parameterBlockRules, + elementType, + elementTypeRules); } // Create a type layout for a structured buffer type. @@ -860,10 +886,25 @@ createStructuredBufferTypeLayout( } +SimpleLayoutInfo GetLayoutImpl( + ExpressionType* type, + LayoutRulesImpl* rules, + RefPtr* outTypeLayout, + SimpleLayoutInfo offset); + SimpleLayoutInfo GetLayoutImpl( ExpressionType* type, LayoutRulesImpl* rules, RefPtr* outTypeLayout) +{ + return GetLayoutImpl(type, rules, outTypeLayout, SimpleLayoutInfo()); +} + +SimpleLayoutInfo GetLayoutImpl( + ExpressionType* type, + LayoutRulesImpl* rules, + RefPtr* outTypeLayout, + SimpleLayoutInfo offset) { if (auto parameterBlockType = type->As()) { @@ -1184,6 +1225,15 @@ SimpleLayoutInfo GetLayoutImpl( fieldResourceInfo->index = structTypeResourceInfo->count; structTypeResourceInfo->count += fieldTypeResourceInfo.count; } + + // If the user passed in offset info, then apply it here + if (offset.size) + { + if (auto fieldResInfo = fieldLayout->FindResourceInfo(offset.kind)) + { + fieldResInfo->index += offset.size; + } + } } } @@ -1226,13 +1276,18 @@ SimpleLayoutInfo GetLayout(ExpressionType* inType, LayoutRulesImpl* rules) return GetLayoutImpl(inType, rules, nullptr); } -RefPtr CreateTypeLayout(ExpressionType* type, LayoutRulesImpl* rules) +RefPtr CreateTypeLayout(ExpressionType* type, LayoutRulesImpl* rules, SimpleLayoutInfo offset) { RefPtr typeLayout; - GetLayoutImpl(type, rules, &typeLayout); + GetLayoutImpl(type, rules, &typeLayout, offset); return typeLayout; } +RefPtr CreateTypeLayout(ExpressionType* type, LayoutRulesImpl* rules) +{ + return CreateTypeLayout(type, rules, SimpleLayoutInfo()); +} + SimpleLayoutInfo GetLayout(ExpressionType* type, LayoutRule rule) { LayoutRulesImpl* rulesImpl = GetLayoutRulesImpl(rule); diff --git a/source/slang/type-layout.h b/source/slang/type-layout.h index d2254c9e5..d5c96abf2 100644 --- a/source/slang/type-layout.h +++ b/source/slang/type-layout.h @@ -535,6 +535,7 @@ SimpleLayoutInfo GetLayout(ExpressionType* type, LayoutRulesImpl* rules); SimpleLayoutInfo GetLayout(ExpressionType* type, LayoutRule rule = LayoutRule::Std430); RefPtr CreateTypeLayout(ExpressionType* type, LayoutRulesImpl* rules); +RefPtr CreateTypeLayout(ExpressionType* type, LayoutRulesImpl* rules, SimpleLayoutInfo offset); // @@ -544,15 +545,19 @@ createParameterBlockTypeLayout( RefPtr parameterBlockType, LayoutRulesImpl* rules); -// Create a type layout for a constant buffer type, -// in the case where we already know the layout -// for the element type. RefPtr createParameterBlockTypeLayout( RefPtr parameterBlockType, - RefPtr elementTypeLayout, - LayoutRulesImpl* rules); + LayoutRulesImpl* parameterBlockRules, + RefPtr elementType, + LayoutRulesImpl* elementTypeRules); +RefPtr +createParameterBlockTypeLayout( + RefPtr parameterBlockType, + LayoutRulesImpl* parameterBlockRules, + SimpleLayoutInfo parameterBlockInfo, + RefPtr elementTypeLayout); // Create a type layout for a structured buffer type. RefPtr -- cgit v1.2.3