From 695ebd78646140645d52e8ba20bd934ed3b31936 Mon Sep 17 00:00:00 2001 From: galibzon <66021303+galibzon@users.noreply.github.com> Date: Tue, 21 Sep 2021 20:48:13 -0500 Subject: [PATCH] https://github.com/o3de/o3de-azslc/issues/8 (#7) Throw Exception When Data Layout Will Encounter Alignment Differences With DXC '-fvk-use-dx-layout' This is v1.7.29 This is the exhaustive solution to validate the struct/class/SRG Layout data alignment problem when: 1- There are float or float2 variables preceded by matrices or structs that end in matrices of type: float2x2, float3x2, float4x2 In such case the "float" or "float2" should be prepadded by a float3 variable and the solution is describbed in the error produced by AZSLc. 2- The other cases are when "float" type variables are preceded by matrices or structs that end in matrices of type: float2x3, float3x3, float4x3 In such case the "float" must be prepadded by a float2 variables. This solution is describbed in the error produced by AZSLc. Alternatively the user can skip this aligment error validation with '--no-aligment-validation' command line option. Error message example: ------------------------------------------------------------- tests\Emission\AsError\matRC_padding.azsl(,) : IR error #131: Detected potential alignment issues related with DXC flag '-fvk-use-dx-layout'. Alternatively you can use the option '--no-alignment-validation' to void this error and compile as is.: - A 'float3' variable should be added before the variable 'm_f3' in 'struct /A/SD2/m_f3' at Line number 21 of 'D:\o3de-azslc\tests\Emission\AsError\matRC_padding.azsl' - A 'float3' variable should be added before the variable 'm_f5' in 'struct /A/SD2/m_f5' at Line number 23 of 'D:\o3de-azslc\tests\Emission\AsError\matRC_padding.azsl' ------------------------------------------------------------- Signed-off-by: garrieta --- src/AzslcBackend.h | 2 +- src/AzslcException.h | 1 + src/AzslcIntermediateRepresentation.cpp | 194 ++++++++++++------ src/AzslcIntermediateRepresentation.h | 46 +++-- src/AzslcKindInfo.h | 11 + src/AzslcMain.cpp | 20 +- src/AzslcUtils.h | 2 +- tests/Advanced/srg-layouts.py | 61 +----- .../Emission/{ => AsError}/mat33_padding.azsl | 11 +- tests/Emission/AsError/matRC_padding.azsl | 97 +++++++++ tests/Emission/mat33_padding.txt | 72 ------- ...ing_run_with_no-alignment-validation.azsl} | 10 +- ...ding_run_with_no-alignment-validation.txt} | 2 +- 13 files changed, 305 insertions(+), 224 deletions(-) rename tests/Emission/{ => AsError}/mat33_padding.azsl (77%) create mode 100644 tests/Emission/AsError/matRC_padding.azsl delete mode 100644 tests/Emission/mat33_padding.txt rename tests/Emission/{mat33_padding_run_with_skip_padding.azsl => mat33_padding_run_with_no-alignment-validation.azsl} (74%) rename tests/Emission/{mat33_padding_run_with_skip_padding.txt => mat33_padding_run_with_no-alignment-validation.txt} (96%) diff --git a/src/AzslcBackend.h b/src/AzslcBackend.h index 408dabd..6e1a690 100644 --- a/src/AzslcBackend.h +++ b/src/AzslcBackend.h @@ -34,7 +34,7 @@ namespace AZ::ShaderCompiler bool m_emitRowMajor = false; //!< False by default (HLSL standard) bool m_forceEmitMajor = false; //!< True if either -Zpc or -Zpr was specified bool m_padRootConstantCB = false; //!< If True, the emitted root constant CB will padded to 16-byte boundary. - bool m_skipMatrix33Padding = false; //! < If True, disables padding of Matrix33 variables when followed by a word size primitive. + bool m_skipAlignmentValidation = false; //! < If True, disables validation of a known DXC issue when certain word or 2-words size variables are preceded by some MatrixRxC variables. DescriptorCountBounds m_minAvailableDescriptors; //!< Hint about the targeted graphics API's minimal guaranteed usable descriptors int m_maxSpaces = std::numeric_limits::max(); //!< Maximum allocatable register logical space, after which register indexes will accumulate, but spaces will be capped int m_rootConstantsMaxSize = std::numeric_limits::max(); //!< Indicates the number of root constants to be allowed, 0 means root constants not enabled diff --git a/src/AzslcException.h b/src/AzslcException.h index ae2ce2a..2597119 100644 --- a/src/AzslcException.h +++ b/src/AzslcException.h @@ -80,6 +80,7 @@ namespace AZ::ShaderCompiler IR_MULTIPLE_SRG_FALLBACK = 128u, IR_NO_FALLBACK_ASSIGNED = 129u, IR_SRG_WITHOUT_SEMANTIC = 130u, + IR_POTENTIAL_DX12_VS_VULKAN_ALIGNMENT_ERROR = 131u, // emitter error codes EMITTER_INVALID_ARRAY_DIMENSIONS = 256u, diff --git a/src/AzslcIntermediateRepresentation.cpp b/src/AzslcIntermediateRepresentation.cpp index c0e94aa..504f205 100644 --- a/src/AzslcIntermediateRepresentation.cpp +++ b/src/AzslcIntermediateRepresentation.cpp @@ -117,9 +117,9 @@ namespace AZ::ShaderCompiler RegisterRootConstantStruct(middleEndconfigration); - if (!middleEndconfigration.m_skipMatrix33Padding) + if (!middleEndconfigration.m_skipAlignmentValidation) { - PadAllFloat3x3WhenFollowedByFourOrLessBytes(middleEndconfigration); + ValidateAlignmentIssueWhenScalarOrFloat2PrecededByMatrix(middleEndconfigration); } } @@ -632,10 +632,11 @@ namespace AZ::ShaderCompiler m_rootConstantStructUID = rootConstantStructUid; } - void IntermediateRepresentation::PadAllFloat3x3WhenFollowedByFourOrLessBytes(const MiddleEndConfiguration& middleEndconfigration) + void IntermediateRepresentation::ValidateAlignmentIssueWhenScalarOrFloat2PrecededByMatrix(const MiddleEndConfiguration& middleEndconfigration) { - //! Helper lambda to check if a symbol is a 3x3 matrix. - auto isMatrix3x3 = [&](const IdentifierUID& symbolUid) + //! Helper lambda to check if a symbol is a matrix, it also returns + //! the number of columns in @numColumns + auto isMatrix = [&](const IdentifierUID& symbolUid, int& numColumns) { KindInfo* kindInfo = GetKindInfo(symbolUid); const VarInfo* varInfo = kindInfo->GetSubAs(); @@ -650,11 +651,11 @@ namespace AZ::ShaderCompiler return false; } const auto* arithmeticInfo = &varInfo->m_typeInfoExt.m_coreType.m_arithmeticInfo; - if ((arithmeticInfo->m_rows != 3) || (arithmeticInfo->m_cols != 3)) + if (arithmeticInfo->m_rows < 2) { - // Not a 3x3 matrix. skip. return false; } + numColumns = arithmeticInfo->m_cols; if (symbolUid.GetName().find("(") != string::npos) { // It's the return value of a function or a function argument. Skip. @@ -663,8 +664,9 @@ namespace AZ::ShaderCompiler return true; }; - //! Helper lambda. Checks if VarInfo is of scalar type for which its size is 4 or smaller, - auto isWordSizedScalar = [](const MiddleEndConfiguration& middleEndconfigration, const VarInfo* varInfo) + //! Helper lambda. Checks if @varInfo size is 8 or smaller. + //! The actual size in bytes is returned in sizeInBytes. + auto isVariableOf8BytesOrLess = [](const MiddleEndConfiguration& middleEndconfigration, const VarInfo* varInfo, size_t& sizeInBytes) { if (!IsFundamental(varInfo->GetTypeClass())) { @@ -674,17 +676,34 @@ namespace AZ::ShaderCompiler { return false; } - if (varInfo->m_typeInfoExt.GetTotalSize(middleEndconfigration.m_packDataBuffers, middleEndconfigration.m_isRowMajor) > 4) + const auto byteCount = varInfo->m_typeInfoExt.GetTotalSize(middleEndconfigration.m_packDataBuffers, middleEndconfigration.m_isRowMajor); + if (byteCount > 8) { return false; } + sizeInBytes = byteCount; return true; }; - // We'll loop through all symbols, find the 3x3 matrices (float3x3), and We can check the next symbol + // We'll loop through all symbols. const auto& orderedSymbols = m_symbols.m_elastic.m_order; - unordered_set structsWithMatrix3x3AsLastField; - vector symbolsToPrepad; + // Keep record of the structs that end in float2x2, float3x2, float4x2 + unordered_set structsWithMatrixRx2AsLastField; + // and also the structs that end in float2x3, float3x3, float4x3 + unordered_set structsWithMatrixRx3AsLastField; + + enum class PrepadType + { + Float2, + Float3, + Unknown, + }; + struct UidWithPrepadType + { + PrepadType m_prepadType; + IdentifierUID m_uid; + }; + vector symbolsToPrepad; for (size_t symbolIndex = 0; symbolIndex < orderedSymbols.size(); ++symbolIndex) { const IdentifierUID uid = orderedSymbols[symbolIndex]; @@ -703,9 +722,17 @@ namespace AZ::ShaderCompiler } const auto memberCount = memberFieldList.size(); auto& lastMemberUid = memberFieldList[memberCount - 1]; - if (isMatrix3x3(lastMemberUid)) + int numColumns = 0; + if (isMatrix(lastMemberUid, numColumns)) { - structsWithMatrix3x3AsLastField.insert(uid); + if (numColumns == 2) + { + structsWithMatrixRx2AsLastField.insert(uid); + } + else if (numColumns == 3) + { + structsWithMatrixRx3AsLastField.insert(uid); + } } continue; } @@ -716,7 +743,8 @@ namespace AZ::ShaderCompiler continue; } - if (!isWordSizedScalar(middleEndconfigration, varInfo)) + size_t variableSizeInBytes; + if (!isVariableOf8BytesOrLess(middleEndconfigration, varInfo, variableSizeInBytes)) { continue; } @@ -728,10 +756,11 @@ namespace AZ::ShaderCompiler } // We only care if the previous VarInfo is one of: - // 1- A struct or class whose last member is a Matrix3x3. - // XOR - // 2- a Matrix3x3. - // + // 1- A struct or class whose last member is a float2x2, float3x2, float4x2, + // float2x3, float3x3, float4x3. + // OR + // 2- a float2x2, float3x2, float4x2, + // float2x3, float3x3, float4x3. //! Helper lambda to get the previously declared variable. auto getPreviousVarInfo = [&](ssize_t& startSearchSymbolIndex /*in out*/) -> VarInfo* { while (startSearchSymbolIndex >= 0) @@ -756,75 +785,124 @@ namespace AZ::ShaderCompiler continue; } - // Is it a Matrix3x3 + // Is it a Matrix? const auto& prevSymbolUid = orderedSymbols[prevSymbolIndex]; - if (isMatrix3x3(prevSymbolUid)) + int numColumns = 0; + if (isMatrix(prevSymbolUid, numColumns)) { - symbolsToPrepad.push_back(uid); + if (numColumns == 2) + { + symbolsToPrepad.push_back(UidWithPrepadType{PrepadType::Float3, uid}); + } + else if ((numColumns == 3) && (variableSizeInBytes <= 4)) + { + symbolsToPrepad.push_back(UidWithPrepadType{PrepadType::Float2, uid}); + } continue; } - // Is the type a struct or class whose last member is a Matrix3x3? + // Is the type a struct or class whose last member is a Matrix of interest? const auto prevVarTypeUid = prevVarInfo->m_typeInfoExt.m_coreType.m_typeId; - if (structsWithMatrix3x3AsLastField.count(prevVarTypeUid)) + if (structsWithMatrixRx2AsLastField.count(prevVarTypeUid)) + { + symbolsToPrepad.push_back(UidWithPrepadType{PrepadType::Float3, uid}); + } + else if (structsWithMatrixRx3AsLastField.count(prevVarTypeUid) && (variableSizeInBytes <= 4)) { - symbolsToPrepad.push_back(uid); + symbolsToPrepad.push_back(UidWithPrepadType{PrepadType::Float2, uid}); } } // for loop end. - //! Helper method to insert a dummy float2 in the symbol table. - auto insertDummyFloat2 = [&](IdentifierUID insertBeforeThisUid) + //! Helper function that returns a string suggesting padding solution + auto getPaddingSolutionMessage = [&](PrepadType prepadType, IdentifierUID insertBeforeThisUid) -> string { // We can deduce the name of parent struct, class or SRG from the name of the field that should come AFTER - // the dummy float2. + // the dummy float2/float3. auto parentName = GetParentName(insertBeforeThisUid.GetName()); - // Define the name of the new dummy float2: - // Remark: it is possible that We end up adding more than one dummy padding variable - // to the same struct. This is why We have a static counter that will be helpful to avoid - // name collision. - static int s_numDummyFloats = 0; - string dummySymbolLeafName = FormatString("__mat33_pad%d__", s_numDummyFloats++); - QualifiedName dummySymbolFieldName{ JoinPath(parentName, dummySymbolLeafName) }; - - // Add the dummy field to the symbol table. - auto& [newVarUid, newVarKind] = m_symbols.AddIdentifier(dummySymbolFieldName, Kind::Variable); - // We must fill up the data. - VarInfo newVarInfo; - newVarInfo.m_declNode = nullptr; - newVarInfo.m_isPublic = false; - string typeName("float2"); - ExtractedTypeExt padType = { UnqualifiedNameView(typeName), nullptr }; - newVarInfo.m_typeInfoExt = ExtendedTypeInfo{ m_sema.CreateTypeRefInfo(padType), - {}, {}, {}, Packing::MatrixMajor::Default }; - newVarKind.GetSubRefAs() = newVarInfo; - - // Finally, We must insert the symbol just before @insertBeforeThisUid within the - // ordered list of the struct/class/SRG. + // The padding is not needed if the variable @insertBeforeThisUid is the first member + // of a struct/class/SRG KindInfo* parentKindInfo = GetKindInfo({ parentName }); assert(parentKindInfo); - const bool isStructOrClass = parentKindInfo->IsKindOneOf(Kind::Class, Kind::Struct); - if (isStructOrClass) + const bool isClass = parentKindInfo->IsKindOneOf(Kind::Class); + const bool isStruct = parentKindInfo->IsKindOneOf(Kind::Struct); + const bool isSrg = parentKindInfo->IsKindOneOf(Kind::ShaderResourceGroup); + if (isStruct || isClass) { - parentKindInfo->GetSubRefAs().InsertBefore(newVarUid, Kind::Variable, insertBeforeThisUid); + auto firstMemberUid = parentKindInfo->GetSubRefAs().GetMemberFields()[0]; + if (firstMemberUid == insertBeforeThisUid) + { + return {}; + } } - else if (parentKindInfo->IsKindOneOf(Kind::ShaderResourceGroup)) + else if (isSrg) { auto& srgInfo = parentKindInfo->GetSubRefAs(); - srgInfo.m_implicitStruct.InsertBefore(newVarUid, Kind::Variable, insertBeforeThisUid); + auto firstMemberUid = srgInfo.m_implicitStruct.GetMemberFields()[0]; + if (firstMemberUid == insertBeforeThisUid) + { + return {}; + } } else { throw std::logic_error{ "error: Was expecting " + string(parentName) + + " to be struct, class or ShaderResourceGroup" }; } + + string typeName; + if (isClass) + { + typeName = "class"; + } + else if (isStruct) + { + typeName = "struct"; + } + else + { + typeName = "ShaderResourceGroup"; + } + + // Let's get the line number where @insertBeforeThisUid was found in the flat AZSL file. + const auto * tmpThis = this; // To disambiguate which version of GetSymbolSubAs<> to call. + const auto * varInfo = tmpThis->GetSymbolSubAs(insertBeforeThisUid.GetName()); + size_t lineOfDeclaration = varInfo->GetLineOfDeclaration(); + const LineDirectiveInfo* lineInfo = GetNearestPreprocessorLineDirective(lineOfDeclaration); + if (!lineInfo) + { + // When the LineDirectiveInfo* is null, it means We have detected a variable that was added + // by AZSLc itself. e.g. Root Constant padding, etc. + // In such case, this is not an issue We want to interfere with. + return {}; + } + const auto originallineNumber = GetLineNumberInOriginalSourceFile(*lineInfo, lineOfDeclaration); + + string solution = FormatString("- A 'float%d' variable should be added before the variable '%s' in '%s %s' at Line number %zu of '%s'\n", + prepadType == PrepadType::Float2 ? 2 : 3, insertBeforeThisUid.GetNameLeaf().c_str(), typeName.c_str(), parentName.data(), + originallineNumber, lineInfo->m_containingFilename.c_str()); + + return solution; + }; - for (const auto& uid : symbolsToPrepad) + string solutionsReport; + for (const auto& uidAndPrepadType : symbolsToPrepad) { - insertDummyFloat2(uid); + solutionsReport += getPaddingSolutionMessage(uidAndPrepadType.m_prepadType, uidAndPrepadType.m_uid); } + + if (solutionsReport.empty()) + { + return; + } + + string errorMessage( + "Detected potential alignment issues related with DXC flag '-fvk-use-dx-layout'.\n" + "Alternatively you can use the option '--no-alignment-validation' to void this error and compile as is.:\n"); + errorMessage += solutionsReport; + throw AzslcIrException(IR_POTENTIAL_DX12_VS_VULKAN_ALIGNMENT_ERROR, errorMessage); } ////////////////////////////////////////////////////////////////////////// diff --git a/src/AzslcIntermediateRepresentation.h b/src/AzslcIntermediateRepresentation.h index 43ef8f4..3eb04ff 100644 --- a/src/AzslcIntermediateRepresentation.h +++ b/src/AzslcIntermediateRepresentation.h @@ -222,18 +222,28 @@ namespace AZ::ShaderCompiler return {}; } + + // GFX-TODO: https://github.com/o3de/o3de-azslc/issues/9 + // Upgrade DXC and if the alignment issues are fixed when using + // -fvk-use-dx-layout then this function can be deleted. /////////////////////////////////////////////////////////////////////// - //! [GFX TODO] - ATOM-16287. Validate other matrix dimensions. - /////////////////////////////////////////////////////////////////////// - //! This function is useful to workaround a bug in DXC when - //! -fvk-use-dx-layout is specified and there are variables of type Float3x3 - //! followed by primitives of 4 bytes or less. - //! In DX12 the variable of 4 bytes or less starts at offset 44, - //! But in Vulkan ALWAYS (regardless whether -fvk-use-dx-layout is used or not) - //! starts at offset 48. - //! The solution is to check for all member field primitives of size 4bytes or less, if it is emitted - //! after a float3x3 or a variable of type struct that contains a float3x3 as last member. In such case, - //! the word size primitive should be prepended by a "float2" padding variable. + //! This function is useful to check if the alignment of the data in "struct"s + //! or "SRG"s will encounter issues related with a bug in DXC when + //! -fvk-use-dx-layout is specified and there are + //! float or float2 variables + //! preceded by matrices or structs that end in matrices of type: + //! float2x2, float3x2, float4x2 + //! In such case the "float" or "float2" should be be prepadded by a float3 variable + //! and an exception error will be produced with a proposed solution. + //! + //! The other cases are when + //! "float" type variables + //! are preceded by matrices or structs that end in matrices of type: + //! float2x3, float3x3, float4x3 + //! In such case the "float" should be prepadded by a float2 variables. + //! + //! Also it's important to keep track of structs that end in one of the matrices mentioned above. + //! //! //////////////////////////////////////////////////////////// //! Example 1: //! @@ -246,7 +256,7 @@ namespace AZ::ShaderCompiler //! //! struct MyStruct { //! float3x3 m_mat; - //! float2 __pad__; //<-- FIX + //! float2 __pad__; //<-- Suggested FIX //! float m_value; //! } //! @@ -255,28 +265,28 @@ namespace AZ::ShaderCompiler //! //! struct MyStructA { //! float4 m_vec; - //! float3x3 m_mat; + //! float4x2 m_mat; //! } //! //! struct MyStructB { //! MyStructA m_a; - //! float m_value; + //! float2 m_value; //! } //! //! After the Fix: //! //! struct MyStructA { //! float4 m_vec; - //! float3x3 m_mat; + //! float4x2 m_mat; //! } //! //! struct MyStructB { //! MyStructA m_a; - //! float2 __pad__; //<-- FIX - //! float m_value; + //! float3 __pad__; //<-- Suggested FIX + //! float2 m_value; //! } //! - void PadAllFloat3x3WhenFollowedByFourOrLessBytes(const MiddleEndConfiguration& middleEndconfigration); + void ValidateAlignmentIssueWhenScalarOrFloat2PrecededByMatrix(const MiddleEndConfiguration& middleEndconfigration); ////////////////////////////////////////////////////////////////////////// // PreprocessorLineDirective overrides... diff --git a/src/AzslcKindInfo.h b/src/AzslcKindInfo.h index 0ebe818..0903a24 100644 --- a/src/AzslcKindInfo.h +++ b/src/AzslcKindInfo.h @@ -345,6 +345,8 @@ namespace AZ::ShaderCompiler // access array dimensions. think of it as it they apply on the whole variable's type. (so not the generic type) // returns an ArrayDimensions struct const ref. inline const auto& GetArrayDimensions() const; + // Returns the line number, in the AZSL file, where this symbol is declared. + inline size_t GetLineOfDeclaration () const; AstUnnamedVarDecl* m_declNode = nullptr; UnqualifiedName m_identifier; @@ -431,6 +433,15 @@ namespace AZ::ShaderCompiler return m_typeInfoExt.GetDimensions(); } + size_t VarInfo::GetLineOfDeclaration() const + { + if (m_declNode) + { + return m_declNode->start->getLine(); + } + return 0; + } + struct OverloadSetInfo { //! Set the name of the set diff --git a/src/AzslcMain.cpp b/src/AzslcMain.cpp index 2c27106..989e26e 100644 --- a/src/AzslcMain.cpp +++ b/src/AzslcMain.cpp @@ -23,7 +23,7 @@ namespace StdFs = std::filesystem; // For large features or milestones. Minor version allows for breaking changes. Existing tests can change. #define AZSLC_MINOR "7" // For small features or bug fixes. They cannot introduce breaking changes. Existing tests shouldn't change. -#define AZSLC_REVISION "28" // ATOM-4800 +#define AZSLC_REVISION "29" // ATOM-16433 namespace AZ::ShaderCompiler { @@ -204,19 +204,19 @@ namespace AZ::ShaderCompiler::Main R"(Amazon Shader Language Compiler Usage: - azslc (- | FILE) [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--namespace=] [--strip-unused-srgs] [--no-ms] [--skip-mat33-padding] [-o OUTFILE] + azslc (- | FILE) [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--namespace=] [--strip-unused-srgs] [--no-ms] [--no-alignment-validation] [-o OUTFILE] [--W0|--W1|--W2|--W3] [--Wx|--Wx1|--Wx2|--Wx3] [--min-descriptors=] [--max-spaces=] - azslc (- | FILE) --full [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--strip-unused-srgs] [--no-ms] [--skip-mat33-padding] [-o OUTFILE] + azslc (- | FILE) --full [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--strip-unused-srgs] [--no-ms] [--no-alignment-validation] [-o OUTFILE] [--W0|--W1|--W2|--W3] [--Wx|--Wx1|--Wx2|--Wx3] [--min-descriptors=] [--max-spaces=] azslc (- | FILE) --ia [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--strip-unused-srgs] [-o OUTFILE] azslc (- | FILE) --om [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--strip-unused-srgs] [-o OUTFILE] - azslc (- | FILE) --srg [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--min-descriptors=] [--max-spaces=] [--strip-unused-srgs] [--no-ms] [--skip-mat33-padding] [-o OUTFILE] + azslc (- | FILE) --srg [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--root-const=] [--pad-root-const] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--min-descriptors=] [--max-spaces=] [--strip-unused-srgs] [--no-ms] [--no-alignment-validation] [-o OUTFILE] azslc (- | FILE) --options [--use-spaces] [--unique-idx] [--cb-body] [--root-sig] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--strip-unused-srgs] [-o OUTFILE] azslc (- | FILE) --semantic [--verbose] [--W0|--W1|--W2|--W3] [--Wx|--Wx1|--Wx2|--Wx3] [--root-const=] [--pad-root-const] [--strip-unused-srgs] azslc (- | FILE) --syntax - azslc (- | FILE) --dumpsym [--strip-unused-srgs] [--no-ms] [--skip-mat33-padding] + azslc (- | FILE) --dumpsym [--strip-unused-srgs] [--no-ms] [--no-alignment-validation] azslc (- | FILE) --ast [--strip-unused-srgs] - azslc (- | FILE) --bindingdep [--use-spaces] [--unique-idx] [--cb-body] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--max-spaces=] [--strip-unused-srgs] [--no-ms] [--skip-mat33-padding] [-o OUTFILE] + azslc (- | FILE) --bindingdep [--use-spaces] [--unique-idx] [--cb-body] [--Zpc] [--Zpr] [--pack-dx12] [--pack-vulkan] [--pack-opengl] [--namespace=] [--max-spaces=] [--strip-unused-srgs] [--no-ms] [--no-alignment-validation] [-o OUTFILE] azslc (- | FILE) --visitsym MQNAME [-d] [-v] [-f] [-r] azslc --listpredefined azslc -h | --help | --version @@ -254,7 +254,9 @@ namespace AZ::ShaderCompiler::Main --no-ms Transforms usage of Texture2DMS/Texture2DMSArray and related functions and semantics into plain Texture2D/Texture2DArray equivalents. This is useful for allowing shader authors to easily write AZSL code that can be compiled into alternatives to work with both a multisample render pipeline and a non-MS render pipeline. -[ --skip-mat33-padding] Skips pre-padding word-sized primitives when preceded by a 3x3 matrix. +[ --no-alignment-validation Skips checking for potential alignment issues related with differences between dx12 and vulkan. + By default, the compiler checks for those issues and fails to compile when those issues are detected. + Use this flag to skip such validation. -d (Option of --visitsym) Visit direct references -v (Option of --visitsym) Visit overload-set -f (Option of --visitsym) Visit family @@ -513,7 +515,7 @@ int main(int argc, const char* argv[]) emitOptions.m_emitConstantBufferBody = args["--cb-body"].asBool(); emitOptions.m_emitRootSig = args["--root-sig"].asBool(); emitOptions.m_padRootConstantCB = args["--pad-root-const"].asBool(); - emitOptions.m_skipMatrix33Padding = args["--skip-mat33-padding"].asBool(); + emitOptions.m_skipAlignmentValidation = args["--no-alignment-validation"].asBool(); if (args["--root-const"]) { @@ -576,7 +578,7 @@ int main(int argc, const char* argv[]) emitOptions.m_packDataBuffers, emitOptions.m_emitRowMajor, emitOptions.m_padRootConstantCB, - emitOptions.m_skipMatrix33Padding }; + emitOptions.m_skipAlignmentValidation }; ir.MiddleEnd(middleEndConfigration); if (convertToNoMS) { diff --git a/src/AzslcUtils.h b/src/AzslcUtils.h index c00d250..9bc7c07 100644 --- a/src/AzslcUtils.h +++ b/src/AzslcUtils.h @@ -1160,7 +1160,7 @@ namespace AZ::ShaderCompiler AZ::ShaderCompiler::Packing::Layout m_packDataBuffers; bool m_isRowMajor; bool m_padRootConstantCB; - bool m_skipMatrix33Padding; + bool m_skipAlignmentValidation; }; ExtractedComposedType ExtractComposedTypeNamesFromAstContext(AstType* ctx, vector* genericDims = nullptr); diff --git a/tests/Advanced/srg-layouts.py b/tests/Advanced/srg-layouts.py index 73b56bc..ca68e00 100644 --- a/tests/Advanced/srg-layouts.py +++ b/tests/Advanced/srg-layouts.py @@ -945,7 +945,7 @@ def verifyPackingRelaxedUniqueIdxUseSpaces(thefile, compilerPath, silent): return True if ok else False def verifyPackingDirectXMatrices(thefile, compilerPath, silent): - j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-dx12", "--skip-mat33-padding", "--srg"]) + j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-dx12", "--no-alignment-validation", "--srg"]) if ok: predicates = [] @@ -1093,7 +1093,7 @@ def verifyPackingDirectXMatrices(thefile, compilerPath, silent): return True if ok else False def verifyPackingVulkanMatrices(thefile, compilerPath, silent): - j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-vulkan", "--skip-mat33-padding", "--srg"]) + j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-vulkan", "--no-alignment-validation", "--srg"]) if ok: predicates = [] @@ -1241,7 +1241,7 @@ def verifyPackingVulkanMatrices(thefile, compilerPath, silent): return True if ok else False def verifyPackingDirectXStride(thefile, compilerPath, silent): - j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-dx12", "--skip-mat33-padding", "--srg"]) + j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--pack-dx12", "--no-alignment-validation", "--srg"]) if ok: predicates = [] @@ -1370,59 +1370,6 @@ def verifyPackingMetalInlineConstants(thefile, compilerPath, silent): ok = testfuncs.verifyAllPredicates(predicates, j) return True if ok else False -def verifyPackingForPaddedMatrix33(thefile, compilerPath, silent): - j, ok = testfuncs.buildAndGetJson(thefile, compilerPath, silent, ["--srg"]) - - if ok: - predicates = [] - - predicates.append(lambda: j["ShaderResourceGroups"][0]["bufferForSRGConstants"]["count"] == 1) - predicates.append(lambda: j["ShaderResourceGroups"][0]["bufferForSRGConstants"]["index"] == 0) - predicates.append(lambda: j["ShaderResourceGroups"][0]["bufferForSRGConstants"]["space"] == 0) - predicates.append(lambda: j["ShaderResourceGroups"][0]["bufferForSRGConstants"]["id"] == "SRG1") - predicates.append(lambda: j["ShaderResourceGroups"][0]["bufferForSRGConstants"]["usage"] == "Read") - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][4]["constantByteOffset"] == 160) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][4]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][4]["qualifiedName"] == "/A/D/__mat33_pad0__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][5]["constantByteOffset"] == 168) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][5]["constantByteSize"] == 4) - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][10]["constantByteOffset"] == 224) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][10]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][10]["qualifiedName"] == "/B/__mat33_pad1__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][11]["constantByteOffset"] == 232) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][11]["constantByteSize"] == 4) - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][17]["constantByteOffset"] == 352) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][17]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][17]["qualifiedName"] == "/C/__mat33_pad2__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][18]["constantByteOffset"] == 360) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][18]["constantByteSize"] == 4) - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][23]["constantByteOffset"] == 448) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][23]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][23]["qualifiedName"] == "/C/F/__mat33_pad3__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][24]["constantByteOffset"] == 456) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][24]["constantByteSize"] == 4) - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][28]["constantByteOffset"] == 512) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][28]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][28]["qualifiedName"] == "/SRG1/__mat33_pad4__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][29]["constantByteOffset"] == 520) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][29]["constantByteSize"] == 4) - - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][31]["constantByteOffset"] == 576) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][31]["constantByteSize"] == 8) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][31]["qualifiedName"] == "/SRG1/__mat33_pad5__") - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][32]["constantByteOffset"] == 584) - predicates.append(lambda: j["ShaderResourceGroups"][0]["inputsForSRGConstants"][32]["constantByteSize"] == 4) - - if not silent: print (fg.CYAN+ style.BRIGHT+ "padded float3x3 layouts verification..."+ style.RESET_ALL) - ok = testfuncs.verifyAllPredicates(predicates, j) - - return True if ok else False - result = 0 # to define for sub-tests resultFailed = 0 @@ -1481,8 +1428,6 @@ def doTests(compiler, silent, azdxcpath): if verifyPackingMetalInlineConstants(os.path.join(workDir, "inline-constant-layouts.azsl"), compiler, silent) : result += 1 else: resultFailed += 1 - if verifyPackingForPaddedMatrix33(os.path.join(workDir, "../Emission/mat33_padding.azsl"), compiler, silent) : result += 1 - else: resultFailed += 1 if __name__ == "__main__": print ("please call from testapp.py") diff --git a/tests/Emission/mat33_padding.azsl b/tests/Emission/AsError/mat33_padding.azsl similarity index 77% rename from tests/Emission/mat33_padding.azsl rename to tests/Emission/AsError/mat33_padding.azsl index 5e3b8b8..7648b3f 100644 --- a/tests/Emission/mat33_padding.azsl +++ b/tests/Emission/AsError/mat33_padding.azsl @@ -1,6 +1,11 @@ -// This test case is used to validate proper insertion of dummy -// float2 variables after float3x3 matrices in structs/classes & ShaderResourceGroups. -// The idea is to match offsets and alignment between dx12 & vulkan. See ATOM-14595. +// This test case is used to validate that the exception code 131 is thrown. +// Exception code 131 (IR_POTENTIAL_DX12_VS_VULKAN_ALIGNMENT_ERROR) means +// that AZSLc detected data layout issues within struct, classes or SRGs +// that are known to produce different layout data by DXC when using +// the command line option: -fvk-use-dx-layout +// See https://github.com/o3de/o3de-azslc/issues/8 +// +// #EC 131 ShaderResourceGroupSemantic slot1 { diff --git a/tests/Emission/AsError/matRC_padding.azsl b/tests/Emission/AsError/matRC_padding.azsl new file mode 100644 index 0000000..c2a91b0 --- /dev/null +++ b/tests/Emission/AsError/matRC_padding.azsl @@ -0,0 +1,97 @@ +// This test case is used to validate that the exception code 131 is thrown. +// Exception code 131 (IR_POTENTIAL_DX12_VS_VULKAN_ALIGNMENT_ERROR) means +// that AZSLc detected data layout issues within struct, classes or SRGs +// that are known to produce different layout data by DXC when using +// the command line option: -fvk-use-dx-layout +// See https://github.com/o3de/o3de-azslc/issues/8 +// +// #EC 131 + +ShaderResourceGroupSemantic slot1 +{ + FrequencyId = 1; +}; + +struct A +{ + struct SD2 + { + int m_i0; + float3x3 m_mat1; + float2x2 m_mat2; + float m_f3; //<-- Dummy float3 must be inserted before this variable. + float3x2 m_mat4; + float2 m_f5; //<-- Dummy float3 must be inserted before this variable. + float4x2 m_mat6; + float2 m_f7; //<-- Dummy float3 must be inserted before this variable. + float2x3 m_mat8; + float m_f9; //<-- Dummy float2 must be inserted before this variable. + float3x3 m_mat10; + float m_f11; //<-- Dummy float2 must be inserted before this variable. + float4x3 m_mat12; + float m_f13; //<-- Dummy float2 must be inserted before this variable. + } m_d14; + + float m_f15; + float2x2 m_mat16; +}; + +struct B +{ + float m_f17; + float3x2 m_mat18; +}; + +struct C +{ + float m_f19; + float4x2 m_mat20; +}; + +struct D +{ + float m_f21; + float2x3 m_mat22; +}; + +struct E +{ + float m_f23; + float3x3 m_mat24; +}; + +struct F +{ + float m_f25; + float4x3 m_mat26; +}; + +ShaderResourceGroup SRG1 : slot1 +{ + A m_a27; + float2 m_f28; //<-- Dummy float3 must be inserted before this variable. + B m_b29; + uint2 m_u30; //<-- Dummy float3 must be inserted before this variable. + C m_c31; + bool m_b32; //<-- Dummy float3 must be inserted before this variable. + + D m_d33; + int m_i34; //<-- Dummy float2 must be inserted before this variable. + + E m_e35; + float2 m_f36; + + F m_sf37; + uint m_u38; //<-- Dummy float2 must be inserted before this variable. + + float3x3 m_srg_mat39; + float m_f40; //<-- Dummy float2 must be inserted before this variable. +}; + +float4 MainPS(float2 uv : TEXCOORD0) : SV_Target0 +{ + float3x3 anotherMatrix; + float aFloat; + float4 diffuse = float4(SRG1::m_f40, 0, 0, 0); + return diffuse; +} diff --git a/tests/Emission/mat33_padding.txt b/tests/Emission/mat33_padding.txt deleted file mode 100644 index efab410..0000000 --- a/tests/Emission/mat33_padding.txt +++ /dev/null @@ -1,72 +0,0 @@ -"struct A" -"{" -"struct D" -"{" -"int m_i0 ;" -"float3x3 m_mat1 ;" -"float3x3 m_mat2 ;" -"float2 __mat33_pad0__ ;" -"float m_f3 ;" -"} ;" -":: A :: D m_d4 ;" -"float m_f5 ;" -"float3x3 m_mat6 ;" -"} ;" -"class B" -"{" -":: A m_a7 ;" -"float2 __mat33_pad1__ ;" -"int m_f8 ;" -"struct E" -"{" -"float m_f9 ;" -"float3x3 m_mat10 ;" -"} ;" -":: B :: E m_e11 ;" -"bool IsFalse ( )" -"{" -"float3x3 someMatrix ;" -"return m_mat12 != someMatrix ;" -"}" -"float3x3 m_mat12 ;" -"} ;" -"struct C" -"{" -":: B m_b13 ;" -"float2 __mat33_pad2__ ;" -"bool m_f14 ;" -"struct F" -"{" -"float m_f15 ;" -"struct G" -"{" -"float m_f16 ;" -"float3x3 m_mat17 ;" -"} ;" -":: C :: F :: G m_sg18 ;" -"float2 __mat33_pad3__ ;" -"float m_f19 ;" -"} ;" -":: C :: F m_sf20 ;" -"float3x3 m_mat21 ;" -"} ;" -"/* Generated code from" -"ShaderResourceGroup SRG1" -"*/" -"struct SRG1_SRGConstantsStruct" -"{" -"float3x3 SRG1_m_srg_mat22 ;" -":: C SRG1_m_c23 ;" -"float2 SRG1___mat33_pad4__ ;" -"uint SRG1_m_u24 ;" -"float3x3 SRG1_m_srg_mat25 ;" -"float2 SRG1___mat33_pad5__ ;" -"int SRG1_m_i26 ;" -"} ;" -"ConstantBuffer < :: SRG1_SRGConstantsStruct > SRG1_SRGConstantBuffer : register ( b0 ) ;" -"float4 MainPS ( float2 uv : TEXCOORD0 ) : SV_Target0" -"{" -"float3x3 anotherMatrix ;" -"float4 diffuse = float4 ( :: SRG1_SRGConstantBuffer . SRG1_m_i26 , 0 , 0 , 0 ) ;" -"return diffuse ;" -"}" \ No newline at end of file diff --git a/tests/Emission/mat33_padding_run_with_skip_padding.azsl b/tests/Emission/mat33_padding_run_with_no-alignment-validation.azsl similarity index 74% rename from tests/Emission/mat33_padding_run_with_skip_padding.azsl rename to tests/Emission/mat33_padding_run_with_no-alignment-validation.azsl index 5e3b8b8..69352ac 100644 --- a/tests/Emission/mat33_padding_run_with_skip_padding.azsl +++ b/tests/Emission/mat33_padding_run_with_no-alignment-validation.azsl @@ -1,6 +1,10 @@ -// This test case is used to validate proper insertion of dummy -// float2 variables after float3x3 matrices in structs/classes & ShaderResourceGroups. -// The idea is to match offsets and alignment between dx12 & vulkan. See ATOM-14595. +// This test case is used to validate that the exception code 131 is NOT thrown +// when using the command line argument '--no-alignment-validation' +// Exception code 131 (IR_POTENTIAL_DX12_VS_VULKAN_ALIGNMENT_ERROR) means +// that AZSLc detected data layout issues within struct, classes or SRGs +// that are known to produce different layout data by DXC when using +// the command line option: -fvk-use-dx-layout +// See https://github.com/o3de/o3de-azslc/issues/8 ShaderResourceGroupSemantic slot1 { diff --git a/tests/Emission/mat33_padding_run_with_skip_padding.txt b/tests/Emission/mat33_padding_run_with_no-alignment-validation.txt similarity index 96% rename from tests/Emission/mat33_padding_run_with_skip_padding.txt rename to tests/Emission/mat33_padding_run_with_no-alignment-validation.txt index 7309c6e..c1bd1f9 100644 --- a/tests/Emission/mat33_padding_run_with_skip_padding.txt +++ b/tests/Emission/mat33_padding_run_with_no-alignment-validation.txt @@ -1,5 +1,5 @@ /* - Cmdargs: ['--skip-mat33-padding'] + Cmdargs: ['--no-alignment-validation'] */ "struct A" "{"