From 4e5e01e73991fc47d5b964de8c0865d901060c32 Mon Sep 17 00:00:00 2001 From: baldurk Date: Fri, 11 Mar 2022 18:43:15 +0000 Subject: [PATCH] Improve consistency in shader constant reflection * This reports the struct vartype now, ensures that struct sizes are always reported and alignments are respected, along with some other fixes to more accurately report memory layouts of structs. --- renderdoc/data/glsl_shaders.cpp | 13 +- renderdoc/driver/gl/gl_replay.cpp | 6 +- renderdoc/driver/gl/gl_shader_refl.cpp | 150 +++++++++-- .../driver/shaders/dxbc/dxbc_container.cpp | 5 - .../driver/shaders/dxbc/dxbc_reflect.cpp | 47 ++++ .../driver/shaders/spirv/spirv_reflect.cpp | 235 ++++++++++++++++-- .../driver/shaders/spirv/spirv_reflect.h | 9 +- 7 files changed, 415 insertions(+), 50 deletions(-) diff --git a/renderdoc/data/glsl_shaders.cpp b/renderdoc/data/glsl_shaders.cpp index 4b72b054d..e5dcd5b2a 100644 --- a/renderdoc/data/glsl_shaders.cpp +++ b/renderdoc/data/glsl_shaders.cpp @@ -1011,8 +1011,8 @@ void main() { INFO("UBO member: " << member.name.c_str()); CHECK(member.byteOffset == 192); - // this doesn't reflect in native introspection, so we skip it - // CHECK(member.type.descriptor.elements == 3); + CHECK(member.type.descriptor.type == VarType::Struct); + CHECK(member.type.descriptor.arrayByteStride == 48); REQUIRE_ARRAY_SIZE(member.type.members.size(), 3); { @@ -1257,9 +1257,9 @@ void main() { INFO("SSBO member: " << member.name.c_str()); CHECK(member.byteOffset == 40); - // this doesn't reflect in native introspection, so we skip it - // CHECK(member.type.descriptor.elements == 3); + CHECK(member.type.descriptor.type == VarType::Struct); CHECK(member.type.descriptor.arrayByteStride == 24); + CHECK(member.type.descriptor.elements == 3); REQUIRE_ARRAY_SIZE(member.type.members.size(), 3); { @@ -1335,6 +1335,7 @@ void main() { INFO("SSBO member: " << member.name.c_str()); CHECK(member.byteOffset == 0); + CHECK(member.type.descriptor.type == VarType::Struct); CHECK(member.type.descriptor.arrayByteStride == 48); CHECK(member.type.descriptor.elements == ~0U); @@ -1346,6 +1347,8 @@ void main() { INFO("SSBO submember: " << submember.name.c_str()); CHECK(submember.byteOffset == 0); + CHECK(submember.type.descriptor.type == VarType::Struct); + CHECK(submember.type.descriptor.arrayByteStride == 24); REQUIRE_ARRAY_SIZE(submember.type.members.size(), 3); { @@ -1396,6 +1399,8 @@ void main() { INFO("SSBO submember: " << submember.name.c_str()); CHECK(submember.byteOffset == 24); + CHECK(submember.type.descriptor.type == VarType::Struct); + CHECK(submember.type.descriptor.arrayByteStride == 24); REQUIRE_ARRAY_SIZE(submember.type.members.size(), 3); { diff --git a/renderdoc/driver/gl/gl_replay.cpp b/renderdoc/driver/gl/gl_replay.cpp index a0fbd5cbe..360274afa 100644 --- a/renderdoc/driver/gl/gl_replay.cpp +++ b/renderdoc/driver/gl/gl_replay.cpp @@ -2101,7 +2101,7 @@ void GLReplay::OpenGLFillCBufferVariables(ResourceId shader, GLuint prog, bool b if(!variables[i].type.members.empty()) { - if(desc.elements == 0) + if(desc.elements <= 1) { OpenGLFillCBufferVariables(shader, prog, bufferBacked, prefix + var.name.c_str() + ".", variables[i].type.members, var.members, data); @@ -2151,7 +2151,7 @@ void GLReplay::OpenGLFillCBufferVariables(ResourceId shader, GLuint prog, bool b RDCERR("Uniform is buffer backed - index expected"); // if this is an array, generate empty members - if(desc.elements > 0) + if(desc.elements > 1) { rdcarray elems; for(uint32_t a = 0; a < desc.elements; a++) @@ -2185,7 +2185,7 @@ void GLReplay::OpenGLFillCBufferVariables(ResourceId shader, GLuint prog, bool b if(!bufferBacked) offset = 0; - if(desc.elements == 0) + if(desc.elements <= 1) { if(!bufferBacked) { diff --git a/renderdoc/driver/gl/gl_shader_refl.cpp b/renderdoc/driver/gl/gl_shader_refl.cpp index 4904e0ae3..6cf75e1c7 100644 --- a/renderdoc/driver/gl/gl_shader_refl.cpp +++ b/renderdoc/driver/gl/gl_shader_refl.cpp @@ -804,13 +804,17 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa if(var.name[c - 3] == '[' && var.name[c - 2] == '0' && var.name[c - 1] == ']') var.name.resize(c - 3); else - var.type.descriptor.elements = 0; + var.type.descriptor.elements = 1; GLint topLevelStride = 0; if(query == eGL_BUFFER_VARIABLE) { GLenum propName = eGL_TOP_LEVEL_ARRAY_STRIDE; GL.glGetProgramResourceiv(sepProg, query, varIdx, 1, &propName, 1, NULL, &topLevelStride); + + // if ARRAY_SIZE is 0 this is an unbounded array + if(values[4] == 0) + var.type.descriptor.elements = ~0U; } rdcarray *parentmembers = defaultBlock; @@ -832,6 +836,7 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa int arrayIdx = 0; bool blockLevel = true; + int level = 0; // reverse figure out structures and structure arrays while(strchr(nm, '.') || strchr(nm, '[')) @@ -883,21 +888,35 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa parentVar.type.descriptor.name = "struct"; parentVar.type.descriptor.rows = 0; parentVar.type.descriptor.columns = 0; - parentVar.type.descriptor.type = var.type.descriptor.type; + parentVar.type.descriptor.type = VarType::Struct; parentVar.type.descriptor.elements = - isarray && !multiDimArray ? RDCMAX(1U, uint32_t(arrayIdx + 1)) : 0; + isarray && !multiDimArray ? RDCMAX(1U, uint32_t(arrayIdx + 1)) : 1; parentVar.type.descriptor.matrixByteStride = 0; RDCASSERTMSG("Stride is too large for uint16_t", topLevelStride <= 0xffff); parentVar.type.descriptor.arrayByteStride = RDCMIN((uint32_t)topLevelStride, 0xffffu) & 0xffff; // consider all block-level SSBO structs to have infinite elements if they are an array at all + // for structs that aren't the last struct in a block which can't be infinite, this will be + // fixup'd later by looking at the offset of subsequent elements if(blockLevel && topLevelStride && isarray) parentVar.type.descriptor.elements = ~0U; if(!blockLevel) topLevelStride = 0; + // this is no longer block level after the first array, or the first struct member. + // + // this logic is because whether or not a block has a name affects what comes back. E.g. + // buffer ssbo { float ssbo_foo } ; will just be "ssbo_foo", whereas + // buffer ssbo { float ssbo_foo } root; will be "root.ssbo_foo" + // + // we only use blocklevel for the check above, to see if we are on a block-level array to mark + // it as unknown size (to be fixed later), so this check can be a little fuzzy as long as it + // doesn't have false positives. + if(isarray || level >= 1) + blockLevel = false; + bool found = false; // if we can find the base variable already, we recurse into its members @@ -915,8 +934,6 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa parentmembers = &((*parentmembers)[i].type.members); found = true; - blockLevel = false; - break; } } @@ -996,6 +1013,8 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa parentmembers = NULL; break; } + + level++; } if(parentmembers) @@ -1045,13 +1064,108 @@ void ReconstructVarTree(GLenum query, GLuint sepProg, GLuint varIdx, GLint numPa } } -void MakeChildByteOffsetsRelative(ShaderConstant &member) +static uint32_t GetVarAlignment(bool std140, const ShaderConstant &c) +{ + if(!c.type.members.empty()) + { + uint32_t ret = 4; + for(const ShaderConstant &m : c.type.members) + ret = RDCMAX(ret, GetVarAlignment(std140, m)); + + if(std140) + ret = AlignUp16(ret); + return ret; + } + + uint8_t vecSize = c.type.descriptor.columns; + + if(c.type.descriptor.rows > 1 && c.type.descriptor.ColMajor()) + vecSize = c.type.descriptor.rows; + + if(vecSize <= 1) + return 4; + if(vecSize == 2) + return 8; + return 16; +} + +static uint32_t GetVarArrayStride(bool std140, const ShaderConstant &c) +{ + uint32_t stride; + if(!c.type.members.empty()) + { + const ShaderConstant &lastChild = c.type.members.back(); + stride = GetVarArrayStride(std140, lastChild); + if(lastChild.type.descriptor.elements > 1 && lastChild.type.descriptor.elements != ~0U) + stride *= lastChild.type.descriptor.elements; + stride = AlignUp(lastChild.byteOffset + stride, GetVarAlignment(std140, c)); + } + else + { + if(c.type.descriptor.elements > 1) + { + stride = c.type.descriptor.arrayByteStride; + } + else + { + stride = VarTypeByteSize(c.type.descriptor.type); + + if(c.type.descriptor.rows > 1) + { + if(std140) + { + stride *= 4; + + if(c.type.descriptor.ColMajor()) + stride *= RDCMAX((uint8_t)1, c.type.descriptor.columns); + else + stride *= RDCMAX((uint8_t)1, c.type.descriptor.rows); + } + else + { + if(c.type.descriptor.ColMajor()) + { + stride *= RDCMAX((uint8_t)1, c.type.descriptor.columns); + if(c.type.descriptor.rows == 3) + stride *= 4; + else + stride *= RDCMAX((uint8_t)1, c.type.descriptor.rows); + } + else + { + stride *= RDCMAX((uint8_t)1, c.type.descriptor.rows); + if(c.type.descriptor.columns == 3) + stride *= 4; + else + stride *= RDCMAX((uint8_t)1, c.type.descriptor.columns); + } + } + } + else + { + if(c.type.descriptor.columns == 3 && std140) + stride *= 4; + else + stride *= RDCMAX((uint8_t)1, c.type.descriptor.columns); + } + } + } + + return stride; +} + +void FixupStructOffsetsAndSize(bool std140, ShaderConstant &member) { for(ShaderConstant &child : member.type.members) { - MakeChildByteOffsetsRelative(child); + FixupStructOffsetsAndSize(std140, child); child.byteOffset -= member.byteOffset; } + + if(!member.type.members.empty()) + { + member.type.descriptor.arrayByteStride = GetVarArrayStride(std140, member); + } } int ParseVersionStatement(const char *version) @@ -1155,7 +1269,7 @@ void MakeShaderReflection(GLenum shadType, GLuint sepProg, ShaderReflection &ref res.isTexture = true; res.variableType.descriptor.rows = 1; res.variableType.descriptor.columns = 4; - res.variableType.descriptor.elements = 0; + res.variableType.descriptor.elements = 1; res.variableType.descriptor.arrayByteStride = 0; res.variableType.descriptor.matrixByteStride = 0; @@ -1700,7 +1814,7 @@ void MakeShaderReflection(GLenum shadType, GLuint sepProg, ShaderReflection &ref res.resType = TextureType::Buffer; res.variableType.descriptor.rows = 0; res.variableType.descriptor.columns = 0; - res.variableType.descriptor.elements = 0; + res.variableType.descriptor.elements = 1; res.variableType.descriptor.arrayByteStride = 0; res.variableType.descriptor.matrixByteStride = 0; res.variableType.descriptor.name = "buffer"; @@ -1801,6 +1915,11 @@ void MakeShaderReflection(GLenum shadType, GLuint sepProg, ShaderReflection &ref for(size_t ssbo = 0; ssbo < ssbos.size(); ssbo++) { rdcarray &ssboVars = rwresources[ssbos[ssbo]].variableType.members; + + // can't make perfect guesses of struct alignment but assume std430 for ssbos + for(ShaderConstant &member : ssboVars) + FixupStructOffsetsAndSize(false, member); + for(size_t rootMember = 0; rootMember + 1 < ssboVars.size(); rootMember++) { ShaderConstant &member = ssboVars[rootMember]; @@ -1808,14 +1927,14 @@ void MakeShaderReflection(GLenum shadType, GLuint sepProg, ShaderReflection &ref const uint32_t memberSizeBound = ssboVars[rootMember + 1].byteOffset - member.byteOffset; const uint32_t stride = member.type.descriptor.arrayByteStride; - if(stride != 0 && member.type.descriptor.elements <= 1 && memberSizeBound > 2 * stride) + if(stride != 0 && member.type.descriptor.elements == ~0U) { - member.type.descriptor.elements = memberSizeBound / stride; + if(memberSizeBound >= 2 * stride) + member.type.descriptor.elements = memberSizeBound / stride; + else + member.type.descriptor.elements = 1; } } - - for(ShaderConstant &member : ssboVars) - MakeChildByteOffsetsRelative(member); } delete[] members; @@ -1870,8 +1989,9 @@ void MakeShaderReflection(GLenum shadType, GLuint sepProg, ShaderReflection &ref sort(ubos[i]); + // can't make perfect guesses of struct alignment but assume std140 for ubos for(ShaderConstant &member : ubos[i]) - MakeChildByteOffsetsRelative(member); + FixupStructOffsetsAndSize(true, member); std::swap(cblock.variables, ubos[i]); diff --git a/renderdoc/driver/shaders/dxbc/dxbc_container.cpp b/renderdoc/driver/shaders/dxbc/dxbc_container.cpp index 601198b5f..2ce8ec1b4 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_container.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_container.cpp @@ -320,11 +320,6 @@ rdcstr TypeName(CBufferVariableType::Descriptor desc) if(desc.rows > 1) { ret = StringFormat::Fmt("%s%dx%d", type, desc.rows, desc.cols); - - if(desc.varClass == CLASS_MATRIX_ROWS) - { - ret = "row_major " + ret; - } } else if(desc.cols > 1) { diff --git a/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp b/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp index fe05f453c..c81b25256 100644 --- a/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp +++ b/renderdoc/driver/shaders/dxbc/dxbc_reflect.cpp @@ -31,6 +31,22 @@ static ShaderConstant MakeConstantBufferVariable(bool cbufferPacking, const DXBC::CBufferVariable &var); +static void FixupEmptyStructs(rdcarray &members) +{ + for(size_t i = 0; i < members.size(); i++) + { + if(members[i].byteOffset == ~0U) + { + // don't try to calculate offset for trailing empty structs, just delete them + if(i == members.size() - 1) + members.pop_back(); + // other empty struct members take the offset of the next member + else + members[i].byteOffset = members[i + 1].byteOffset; + } + } +} + static ShaderConstantType MakeShaderConstantType(bool cbufferPacking, DXBC::CBufferVariableType type) { ShaderConstantType ret; @@ -56,7 +72,26 @@ static ShaderConstantType MakeShaderConstantType(bool cbufferPacking, DXBC::CBuf if(type.descriptor.varClass == DXBC::CLASS_STRUCT) { + // fxc's reported byte size for UAV structs is not reliable (booo). It seems to assume some + // padding that doesn't exist, especially in arrays. So e.g.: + // struct nested_with_padding + // { + // float a; float4 b; float c; float3 d[4]; + // }; + // is tightly packed and only contains 1+4+1+4*3 floats = 18*4 = 72 bytes + // however an array of nested_with_padding foo[2] will have size listed as 176 bytes. + // + // fortunately, since packing is tight we can look at the 'columns' field which is the number of + // floats in the struct, and multiply that uint32_t stride = type.descriptor.bytesize / RDCMAX(1U, type.descriptor.elements); + if(!cbufferPacking) + { + stride = type.descriptor.cols * sizeof(float); + // the exception is empty structs have 1 cols, probably because of a max(1,...) somewhere + if(type.descriptor.bytesize == 0) + stride = 0; + } + RDCASSERTMSG("Stride is too large for uint16_t", stride <= 0xffff); ret.descriptor.arrayByteStride = RDCMIN(stride, 0xffffu) & 0xffff; @@ -65,6 +100,8 @@ static ShaderConstantType MakeShaderConstantType(bool cbufferPacking, DXBC::CBuf ret.descriptor.arrayByteStride = AlignUp16(ret.descriptor.arrayByteStride); ret.descriptor.rows = ret.descriptor.columns = 0; + + ret.descriptor.type = VarType::Struct; } else { @@ -78,6 +115,8 @@ static ShaderConstantType MakeShaderConstantType(bool cbufferPacking, DXBC::CBuf for(size_t i = 0; i < type.members.size(); i++) ret.members.push_back(MakeConstantBufferVariable(cbufferPacking, type.members[i])); + FixupEmptyStructs(ret.members); + if(!ret.members.empty()) { ret.descriptor.rows = 0; @@ -96,6 +135,12 @@ static ShaderConstant MakeConstantBufferVariable(bool cbufferPacking, const DXBC ret.defaultValue = 0; ret.type = MakeShaderConstantType(cbufferPacking, var.type); + // fxc emits negative values for offsets of empty structs sometimes. Replace that with a single + // value so we can say 'use the previous value' + if(ret.type.descriptor.type == VarType::Struct && ret.type.members.empty() && + ret.byteOffset > 0xF0000000) + ret.byteOffset = ~0U; + return ret; } @@ -337,6 +382,8 @@ void MakeShaderReflection(DXBC::DXBCContainer *dxbc, ShaderReflection *refl, cb.variables.push_back( MakeConstantBufferVariable(true, dxbc->GetReflection()->CBuffers[i].variables[v])); } + + FixupEmptyStructs(cb.variables); } mapping->samplers.resize(dxbc->GetReflection()->Samplers.size()); diff --git a/renderdoc/driver/shaders/spirv/spirv_reflect.cpp b/renderdoc/driver/shaders/spirv/spirv_reflect.cpp index ab20b5f94..25fa2fa25 100644 --- a/renderdoc/driver/shaders/spirv/spirv_reflect.cpp +++ b/renderdoc/driver/shaders/spirv/spirv_reflect.cpp @@ -314,6 +314,117 @@ static bool HasCommandLineInModuleProcessed(rdcspv::Generator gen) gen == rdcspv::Generator::ShadercoverGlslang); } +struct StructSizes +{ + uint32_t scalarAlign = 1; + uint32_t baseAlign = 1; + uint32_t extendedAlign = 1; + + uint32_t scalarSize = 0; + uint32_t baseSize = 0; + uint32_t extendedSize = 0; +}; + +StructSizes CalculateStructProps(uint32_t emptyStructSize, const ShaderConstant &c) +{ + StructSizes ret; + + if(c.type.descriptor.type != VarType::Struct) + { + // A scalar of size N has a scalar alignment of N. + // A vector or matrix type has a scalar alignment equal to that of its component type. + // An array type has a scalar alignment equal to that of its element type. + ret.scalarAlign = VarTypeByteSize(c.type.descriptor.type); + + // A scalar has a base alignment equal to its scalar alignment. + ret.baseAlign = ret.scalarAlign; + + // A row-major matrix of C columns has a base alignment equal to the base alignment of a vector + // of C matrix components. + uint8_t vecSize = c.type.descriptor.columns; + uint8_t matSize = c.type.descriptor.rows; + + // A column-major matrix has a base alignment equal to the base alignment of the matrix column + // type. + if(c.type.descriptor.rows > 1 && c.type.descriptor.ColMajor()) + { + vecSize = c.type.descriptor.rows; + matSize = c.type.descriptor.columns; + } + + // A two-component vector has a base alignment equal to twice its scalar alignment. + if(vecSize == 2) + ret.baseAlign *= 2; + // A three- or four-component vector has a base alignment equal to four times its scalar + // alignment. + else if(vecSize == 3 || vecSize == 4) + ret.baseAlign *= 4; + + // An array has a base alignment equal to the base alignment of its element type. + // N/A + + // A scalar, vector or matrix type has an extended alignment equal to its base alignment. + ret.extendedAlign = ret.baseAlign; + + // An array or structure type has an extended alignment equal to the largest extended alignment + // of any of its members, rounded up to a multiple of 16. + if(c.type.descriptor.elements > 1) + ret.extendedAlign = AlignUp16(ret.extendedAlign); + + if(matSize > 1) + ret.extendedAlign = ret.baseAlign = c.type.descriptor.matrixByteStride; + + ret.scalarSize = ret.scalarAlign * RDCMAX(c.type.descriptor.rows, (uint8_t)1) * + RDCMAX(c.type.descriptor.columns, (uint8_t)1) * + RDCMAX(c.type.descriptor.elements, 1U); + ret.baseSize = ret.baseAlign * matSize * RDCMAX(c.type.descriptor.elements, 1U); + ret.extendedSize = ret.extendedAlign * matSize * RDCMAX(c.type.descriptor.elements, 1U); + } + else + { + for(size_t i = 0; i < c.type.members.size(); i++) + { + const ShaderConstant &m = c.type.members[i]; + + StructSizes member = CalculateStructProps(emptyStructSize, m); + // A structure has a scalar alignment equal to the largest scalar alignment of any of its + // members. + ret.scalarAlign = RDCMAX(ret.scalarAlign, member.scalarAlign); + // A structure has a base alignment equal to the largest base alignment of any of its members. + ret.baseAlign = RDCMAX(ret.baseAlign, member.baseAlign); + // An array or structure type has an extended alignment equal to the largest extended + // alignment of any of its members, rounded up to a multiple of 16. + ret.extendedAlign = RDCMAX(ret.baseAlign, member.extendedAlign); + + if(i + 1 == c.type.members.size()) + { + ret.scalarSize = AlignUp(m.byteOffset + member.scalarSize, ret.scalarAlign); + ret.baseSize = AlignUp(m.byteOffset + member.baseSize, ret.baseAlign); + ret.extendedSize = AlignUp16(m.byteOffset + member.extendedSize); + } + } + + ret.extendedAlign = AlignUp16(ret.extendedAlign); + + // A structure has a base alignment equal to the largest base alignment of any of its members. + // An empty structure has a base alignment equal to the size of the smallest scalar type + // permitted by the capabilities declared in the SPIR-V module. (e.g., for a 1 byte aligned + // empty struct in the StorageBuffer storage class, StorageBuffer8BitAccess or + // UniformAndStorageBuffer8BitAccess must be declared in the SPIR-V module.) + if(c.type.members.empty()) + { + ret.scalarSize = 0; + ret.scalarAlign = emptyStructSize; + ret.baseSize = emptyStructSize; + ret.baseAlign = emptyStructSize; + ret.extendedSize = AlignUp16(emptyStructSize); + ret.extendedAlign = AlignUp16(emptyStructSize); + } + } + + return ret; +} + namespace rdcspv { Reflector::Reflector() @@ -951,6 +1062,10 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st const bool pushConst = (global.storage == StorageClass::PushConstant); const bool atomicCounter = (global.storage == StorageClass::AtomicCounter); + rdcspv::StorageClass effectiveStorage = global.storage; + if(ssbo) + effectiveStorage = StorageClass::StorageBuffer; + Bindpoint bindmap; // set something crazy so this doesn't overlap with a real buffer binding if(pushConst) @@ -1076,8 +1191,8 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st ShaderConstant constant; - MakeConstantBlockVariable(constant, pointerTypes, *varType, strings[global.id], - decorations[global.id], specInfo); + MakeConstantBlockVariable(constant, pointerTypes, effectiveStorage, *varType, + strings[global.id], decorations[global.id], specInfo); if(isArray) constant.type.descriptor.elements = arraySize; @@ -1112,8 +1227,8 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st res.variableType.descriptor.type = VarType::Float; res.variableType.descriptor.name = varType->name; - MakeConstantBlockVariables(*varType, 0, 0, res.variableType.members, pointerTypes, - specInfo); + MakeConstantBlockVariables(effectiveStorage, *varType, 0, 0, res.variableType.members, + pointerTypes, specInfo); rwresources.push_back(shaderrespair(bindmap, res)); } @@ -1126,7 +1241,8 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st cblock.name = StringFormat::Fmt("uniforms%u", global.id.value()); cblock.bufferBacked = !pushConst; - MakeConstantBlockVariables(*varType, 0, 0, cblock.variables, pointerTypes, specInfo); + MakeConstantBlockVariables(effectiveStorage, *varType, 0, 0, cblock.variables, + pointerTypes, specInfo); if(!varType->children.empty()) cblock.byteSize = CalculateMinimumByteSize(cblock.variables); @@ -1160,8 +1276,8 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st name = StringFormat::Fmt("specID%u", decorations[c.id].specID); ShaderConstant spec; - MakeConstantBlockVariable(spec, pointerTypes, dataTypes[c.type], name, decorations[c.id], - specInfo); + MakeConstantBlockVariable(spec, pointerTypes, rdcspv::StorageClass::PushConstant, + dataTypes[c.type], name, decorations[c.id], specInfo); spec.byteOffset = uint32_t(specblock.variables.size() * sizeof(uint64_t)); spec.defaultValue = c.value.value.u64v[0]; specblock.variables.push_back(spec); @@ -1388,8 +1504,8 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st { ShaderConstant dummy; - MakeConstantBlockVariable(dummy, pointerTypes, dataTypes[it->first], rdcstr(), Decorations(), - specInfo); + MakeConstantBlockVariable(dummy, pointerTypes, dataTypes[it->first].pointerType.storage, + dataTypes[it->first], rdcstr(), Decorations(), specInfo); if(it->second >= reflection.pointerTypes.size()) reflection.pointerTypes.resize(it->second + 1); @@ -1398,8 +1514,9 @@ void Reflector::MakeReflection(const GraphicsAPI sourceAPI, const ShaderStage st } } -void Reflector::MakeConstantBlockVariables(const DataType &structType, uint32_t arraySize, - uint32_t arrayByteStride, rdcarray &cblock, +void Reflector::MakeConstantBlockVariables(rdcspv::StorageClass storage, const DataType &structType, + uint32_t arraySize, uint32_t arrayByteStride, + rdcarray &cblock, SparseIdMap &pointerTypes, const rdcarray &specInfo) const { @@ -1414,8 +1531,8 @@ void Reflector::MakeConstantBlockVariables(const DataType &structType, uint32_t cblock.resize(arraySize); for(uint32_t i = 0; i < arraySize; i++) { - MakeConstantBlockVariable(cblock[i], pointerTypes, structType, StringFormat::Fmt("[%u]", i), - decorations[structType.id], specInfo); + MakeConstantBlockVariable(cblock[i], pointerTypes, storage, structType, + StringFormat::Fmt("[%u]", i), decorations[structType.id], specInfo); cblock[i].byteOffset = relativeOffset; @@ -1430,13 +1547,90 @@ void Reflector::MakeConstantBlockVariables(const DataType &structType, uint32_t cblock.resize(structType.children.size()); for(size_t i = 0; i < structType.children.size(); i++) - MakeConstantBlockVariable(cblock[i], pointerTypes, dataTypes[structType.children[i].type], - structType.children[i].name, structType.children[i].decorations, - specInfo); + { + MakeConstantBlockVariable(cblock[i], pointerTypes, storage, + dataTypes[structType.children[i].type], structType.children[i].name, + structType.children[i].decorations, specInfo); + } + + uint32_t emptyStructSize = 4; + + if(storage == rdcspv::StorageClass::StorageBuffer) + { + if(capabilities.find(rdcspv::Capability::StorageBuffer8BitAccess) != capabilities.end() || + capabilities.find(rdcspv::Capability::UniformAndStorageBuffer8BitAccess) != capabilities.end()) + emptyStructSize = 1; + else if(capabilities.find(rdcspv::Capability::StorageBuffer16BitAccess) != capabilities.end() || + capabilities.find(rdcspv::Capability::UniformAndStorageBuffer16BitAccess) != + capabilities.end()) + emptyStructSize = 2; + } + else if(storage == rdcspv::StorageClass::Uniform) + { + if(capabilities.find(rdcspv::Capability::UniformAndStorageBuffer8BitAccess) != capabilities.end()) + emptyStructSize = 1; + else if(capabilities.find(rdcspv::Capability::UniformAndStorageBuffer16BitAccess) != + capabilities.end()) + emptyStructSize = 2; + } + else if(storage == rdcspv::StorageClass::PushConstant) + { + if(capabilities.find(rdcspv::Capability::StoragePushConstant8) != capabilities.end()) + emptyStructSize = 1; + else if(capabilities.find(rdcspv::Capability::StoragePushConstant16) != capabilities.end()) + emptyStructSize = 2; + } + + for(size_t i = 0; i < cblock.size(); i++) + { + // for structs that aren't in arrays, we need to define their byte size (stride). Without + // knowing the packing rules this shader is complying with, this is not fully possible. + // + // what we do is choose the most conservative size - so that this struct's size alone doesn't + // invalidate compliance with a particular ruleset (e.g. std140). + // + // we calculate the scalar, base, and extended sizes of the struct sizes of the struct. The + // largest one that fits between this struct and the next member is the one we use. If there is + // no next member, we always use the base size as it's impossible to tell how much trailing + // padding the shader expected. + // + // If we guess wrongly small, members after this struct will need an [[offset]] decoration, + // If we guess wrongly large the struct itself will need a [[size]] decoration + // Since we're choosing the largest valid size, it will always be just a [[size]] which might be + // unnecessary (if e.g. somewhere else the shader demonstrates scalar packing so the padded size + // is larger than the scalar calculated size) but that's only present in one place. + + if(cblock[i].type.descriptor.type == VarType::Struct && + cblock[i].type.descriptor.arrayByteStride == 0) + { + // this should not be an array - if it is SPIR-V requires an array byte stride, and this + // calculation below is also invalid. + RDCASSERTEQUAL(cblock[i].type.descriptor.elements, 1); + + StructSizes sizes = CalculateStructProps(emptyStructSize, cblock[i]); + + uint32_t availSize = ~0U; + if(i + 1 < cblock.size()) + availSize = cblock[i + 1].byteOffset - cblock[i].byteOffset; + else if(arrayByteStride != 0) + availSize = arrayByteStride - cblock[i].byteOffset; + + // expect at least the scalar size to be available otherwise this struct seems to overlap + RDCASSERT(sizes.scalarSize <= availSize, sizes.scalarSize, availSize); + + if(sizes.extendedSize <= availSize) + cblock[i].type.descriptor.arrayByteStride = sizes.extendedSize; + else if(sizes.baseSize <= availSize) + cblock[i].type.descriptor.arrayByteStride = sizes.baseSize; + else + cblock[i].type.descriptor.arrayByteStride = sizes.scalarSize; + } + } } void Reflector::MakeConstantBlockVariable(ShaderConstant &outConst, - SparseIdMap &pointerTypes, const DataType &type, + SparseIdMap &pointerTypes, + rdcspv::StorageClass storage, const DataType &type, const rdcstr &name, const Decorations &varDecorations, const rdcarray &specInfo) const { @@ -1524,20 +1718,23 @@ void Reflector::MakeConstantBlockVariable(ShaderConstant &outConst, RDCASSERT(curType->type == DataType::StructType || curType->type == DataType::ArrayType); - outConst.type.descriptor.type = VarType::Float; + outConst.type.descriptor.type = VarType::Struct; outConst.type.descriptor.rows = 0; outConst.type.descriptor.columns = 0; outConst.type.descriptor.name = curType->name; - MakeConstantBlockVariables(*curType, outConst.type.descriptor.elements, + MakeConstantBlockVariables(storage, *curType, outConst.type.descriptor.elements, outConst.type.descriptor.arrayByteStride, outConst.type.members, pointerTypes, specInfo); if(curType->type == DataType::ArrayType) { + outConst.type.descriptor.name = type.name; + // if the inner type is an array, it will be expanded in our members list. So don't also // redundantly keep the element count + outConst.type.descriptor.arrayByteStride *= outConst.type.descriptor.elements; outConst.type.descriptor.elements = 1; } } diff --git a/renderdoc/driver/shaders/spirv/spirv_reflect.h b/renderdoc/driver/shaders/spirv/spirv_reflect.h index 1d0f2ed61..981f6d078 100644 --- a/renderdoc/driver/shaders/spirv/spirv_reflect.h +++ b/renderdoc/driver/shaders/spirv/spirv_reflect.h @@ -107,13 +107,14 @@ private: rdcstr StringiseConstant(rdcspv::Id id) const; void CheckDebuggable(bool &debuggable, rdcstr &debugStatus) const; - void MakeConstantBlockVariables(const DataType &structType, uint32_t arraySize, - uint32_t arrayByteStride, rdcarray &cblock, + void MakeConstantBlockVariables(rdcspv::StorageClass storage, const DataType &structType, + uint32_t arraySize, uint32_t arrayByteStride, + rdcarray &cblock, SparseIdMap &pointerTypes, const rdcarray &specInfo) const; void MakeConstantBlockVariable(ShaderConstant &outConst, SparseIdMap &pointerTypes, - const DataType &type, const rdcstr &name, - const Decorations &varDecorations, + rdcspv::StorageClass storage, const DataType &type, + const rdcstr &name, const Decorations &varDecorations, const rdcarray &specInfo) const; void AddSignatureParameter(const bool isInput, const ShaderStage stage, const Id id, const Id structID, uint32_t ®Index,