Merge pull request #77 from o3de/fix-option-min

Fix for "option range not reconstructed"

src/AzslcEmitter.cpp

@@ -685,13 +685,13 @@ namespace AZ::ShaderCompiler
         else if (attrInfo.m_attribute == "partial")
         {
             // Reserved for ShaderResourceGroup use. Do not re-emit
-            outstream << "// Attribute not re-emitted by design - [[" << attrInfo << "]]\n ";
+            outstream << "// original attribute: [[" << attrInfo << "]]\n ";
         }
 
         else if (attrInfo.m_attribute == "range")
         {
             // Reserved for integer type option variables. Do not re-emit
-            outstream << "// Attribute not re-emitted by design - [[" << attrInfo << "]]\n ";
+            outstream << "// original attribute: [[" << attrInfo << "]]\n ";
         }
 
         else
@@ -1140,14 +1140,68 @@ namespace AZ::ShaderCompiler
         EmitGetShaderKeyFunctionDeclaration(getterUid, returnType);
 
         m_out << "\n{\n";
-        if (keySizeInBits > 0)
+        if (keySizeInBits > 0)  // Emit the option getter function body using CB access and bit decoding.
         {
-            m_out << "    uint shaderKey = (" << GetTranslatedName(shaderKeyUid, UsageContext::ReferenceSite) << "[" << arraySlot << "]." << suffix[swizzle] << " >> " << keyOffsetBits << ") & " << mask << ";\n";
+            m_out << "    uint shaderKey = ((" << GetTranslatedName(shaderKeyUid, UsageContext::ReferenceSite)
+                  << "[" << arraySlot << "]." << suffix[swizzle] << " >> " << keyOffsetBits << ") & " << mask << ")";
+            // Also we need to reproduce the "range minimal" value as an addition post big-field extraction.
+            // because the CB compact storage into bits of an uint4 does not incorporate the offset of the range
+            // because they are "useless" (compressable) entropy bits that would take space in the key.
+            if (returnType.m_typeId.m_name.find("int") != string::npos)  // if type int/uint/int16_t... then it's an integer range.
+            {
+                if (auto attrInfo = m_ir->m_symbols.GetAttribute(getterUid, "range"))
+                {
+                    // Presence of the correct arglist has already been verified in Backend::AppendOptionRange
+                    m_out << " + " << ExtractValueAsInt64(get<ConstNumericVal>(attrInfo->m_argList[0]));
+                }
+            }
+            else if (returnType.m_typeClass == TypeClass::Enum)
+            {
+                // In the case of an enumeration,
+                // recovering the correct value will depend if there are jumps in the enumerators.
+                // To discover that, we will use the presence of initializers.
+                // one initializer on the first enumerator is a special case because it still allows
+                // us to reconstruct the value from an integer cast and a +first enumerator.
+                // In a case of disparate enumerators, we'll need to reconstruct the values
+                // by emitting a switch case.
+                // We don't want that switch case as a generic case, that could simplify azslc's source
+                // but it risks slowing down the shader runtime. Sort of a pay-what-you-use principle.
+                auto& enumClassInfo = *m_ir->GetSymbolSubAs<ClassInfo>(returnType.m_typeId.GetName());
+                auto& enumerators = enumClassInfo.GetOrderedMembers();
+                bool otherInitializers = std::any_of(enumerators.begin() + 1, enumerators.end(),
+                                                     [&](const IdentifierUID& e)
+                                                     {
+                                                         auto& var = *m_ir->GetSymbolSubAs<VarInfo>(e.GetName());
+                                                         return !!var.m_declNodeEnum->Value;
+                                                     });
+                if (otherInitializers)
+                {
+                    m_out << ";\n    switch (shaderKey)\n    {\n";
+                    for (int i = 0; i < enumerators.size(); ++i)
+                    {
+                        m_out << "        case " << i << ": shaderKey = (" << GetTranslatedName(returnType, UsageContext::ReferenceSite) << ")"
+                              << GetTranslatedName(enumerators[i], UsageContext::ReferenceSite) << "; break;\n";
+                    }
+                    m_out << "    }";
+                }
+                else
+                {
+                    // add the value of the first enumerator (by emitting its name)
+                    m_out << " + (uint)" << GetTranslatedName(enumerators.front(), UsageContext::ReferenceSite);  // we can access front with no defense because keySize would be 0 if there are no enumerators.
+                }
+            }
+            m_out << ";\n";
             m_out << "    return (" << GetTranslatedName(returnType, UsageContext::ReferenceSite) << ") shaderKey;\n";
         }
         else
         {
-            m_out << "    " << GetTranslatedName(returnType, UsageContext::ReferenceSite) << " val = " << defaultValue << ";\n";
+            // In the case of an empty enumeration type (no enumerators) or empty range, the keySize will be 0
+            // and this if-branch will be taken. if the programmer specified no initializer clause,
+            // this would produce unbuildable HLSL and error in generatored code. We really don't want that
+            // as it would be hard to diagnose for users. As a reasonable behavior, we can emit val = 0.
+            string typeAsStr = GetTranslatedName(returnType, UsageContext::ReferenceSite);
+            // "<type> val = (cast expr to <type>) 0 or default;"
+            m_out << "    " << typeAsStr << " val = (" << typeAsStr << ")" << (defaultValue.empty() ? "0" : defaultValue) << ";\n";
             m_out << "    return val;\n";
         }
         m_out << "}\n\n";

src/AzslcKindInfo.h

@@ -392,6 +392,7 @@ namespace AZ::ShaderCompiler
         inline size_t              GetOriginalLineNumber () const;
 
         AstUnnamedVarDecl*         m_declNode = nullptr;
+        AstEnumeratorDecl*         m_declNodeEnum = nullptr;
         UnqualifiedName            m_identifier;
         bool                       m_srgMember = false;
         bool                       m_isPublic = true;

src/AzslcMain.cpp

@@ -23,7 +23,8 @@ namespace StdFs = std::filesystem;
 // For large features or milestones. Minor version allows for breaking changes. Existing tests can change.
 #define AZSLC_MINOR "8"   // last change: introduction of class inheritance
 // For small features or bug fixes. They cannot introduce breaking changes. Existing tests shouldn't change.
-#define AZSLC_REVISION "14"  // last change: [5a1b711] Fixing order of multiple unbounded arrays when unique indices are used.
+#define AZSLC_REVISION "15"  // last change: add min in option value key-extracter's function for range & enum
+                    // "14"          change: [5a1b711] Fixing order of multiple unbounded arrays when unique indices are used.
 
 namespace AZ::ShaderCompiler
 {

src/AzslcSemanticOrchestrator.cpp

@@ -386,16 +386,16 @@ namespace AZ::ShaderCompiler
         auto& [enumId, parentKindInfo] = *m_symbols->GetIdAndKindInfo(enumQn);
         auto& enumInfo = parentKindInfo.GetSubRefAs<ClassInfo>();
 
-        size_t line           = ctx->Name->getLine();
-        auto enumeratorName   = UnqualifiedName{ctx->Name->getText()};
-        auto& [uid, var]      = AddIdentifier(enumeratorName, Kind::Variable, line);
-        auto& varInfo         = var.GetSubAfterInitAs<Kind::Variable>();
+        size_t line            = ctx->Name->getLine();
+        auto enumeratorName    = UnqualifiedName{ctx->Name->getText()};
+        auto& [uid, var]       = AddIdentifier(enumeratorName, Kind::Variable, line);
+        auto& varInfo          = var.GetSubAfterInitAs<Kind::Variable>();
         if (ctx->Value)
         {
             varInfo.m_constVal = FoldEvalStaticConstExprNumericValue(ctx->Value);
         }
-        varInfo.m_declNode    = nullptr;
-        Modifiers modifiers = Modifiers{StorageFlag::Static} |
+        varInfo.m_declNodeEnum = ctx;
+        Modifiers modifiers    = Modifiers{StorageFlag::Static} |
             StorageFlag::Const | StorageFlag::Enumerator;
 
         varInfo.m_typeInfoExt = ExtendedTypeInfo{CreateTypeRefInfo(UnqualifiedNameView{enumQn}, OnNotFoundOrWrongKind::Diagnose), {},

src/AzslcUtils.h

@@ -44,6 +44,7 @@ namespace AZ::ShaderCompiler
     using AstClassDeclNode              = azslParser::ClassDefinitionContext;
     using AstStructDeclNode             = azslParser::StructDefinitionContext;
     using AstEnumDeclNode               = azslParser::EnumDefinitionContext;
+    using AstEnumeratorDecl             = azslParser::EnumeratorDeclaratorContext;
     using AstInterfaceDeclNode          = azslParser::InterfaceDefinitionContext;
     using AstSRGSemanticDeclNode        = azslParser::SrgSemanticContext;
     using AstSRGSemanticMemberDeclNode  = azslParser::SrgSemanticMemberDeclarationContext;

tests/Emission/Variants.txt

@@ -1,6 +1,6 @@
 # list lines expected to have tokens matching the patterns in the strings. each space is a separation.
 
 "enum class QualityT { Low , Medium , High , } ;"
-"enum ColorT { Red , Green , Blue , } ;"
 "static const :: QualityT Quality = GetShaderVariantKey_Quality ( ) ;"
+"enum ColorT { Red , Green , Blue , } ;"
 "static const :: ColorT Color = GetShaderVariantKey_Color ( )  ;"

tests/Emission/option-range.azsl

@@ -0,0 +1,25 @@
+ShaderResourceGroupSemantic slot1
+{
+    FrequencyId = 1;
+    ShaderVariantFallback = 128;
+};
+
+ShaderResourceGroup SRG : slot1
+{
+};
+
+[[range(10,11)]]
+option int16_t o_stuff = 10;
+
+option enum E { v = 10, v2 } o_s2;
+
+option enum class F { v = 10, v2 } o_s3;
+
+option enum G { gv, gv2 = 4, gv3 } o_s4;
+
+option enum Empty { } o_y = 1;
+
+float4 MainPS(float2 uv : TEXCOORD0) : SV_Target0
+{
+    return o_stuff;
+}

tests/Emission/option-range.txt

@@ -0,0 +1,31 @@
+"struct SRG_SRGConstantsStruct"
+"uint4 SRG_m_SHADER_VARIANT_KEY_NAME_ [1] ;"
+"ConstantBuffer < :: SRG_SRGConstantsStruct >"
+"int16_t GetShaderVariantKey_o_stuff ( ) ;"
+"# if defined ( o_stuff_OPTION_DEF )"
+"static const int16_t o_stuff = o_stuff_OPTION_DEF ;"
+"# else"
+"static const int16_t o_stuff = GetShaderVariantKey_o_stuff ( ) ;"
+"enum E"
+"v = 10"
+":: E GetShaderVariantKey_o_s2 ( ) ;"
+"enum class F"
+":: F GetShaderVariantKey_o_s3 ( ) ;"
+"enum G"
+"gv2 = 4"
+"int16_t GetShaderVariantKey_o_stuff ( )"
+"uint shaderKey = ( ( :: SRG_SRGConstantBuffer . SRG_m_SHADER_VARIANT_KEY_NAME_ [0] . x > > 0 ) & 1 ) + 10 ;"
+"return ( int16_t ) shaderKey ;"
+":: E GetShaderVariantKey_o_s2 ( )"
+"SRG_m_SHADER_VARIANT_KEY_NAME_ [0] . x > > 1 ) & 1 ) + ( uint ) :: v"
+"GetShaderVariantKey_o_s3"
+"x > > 2 ) & 1 ) + ( uint ) :: F :: v ;"
+"GetShaderVariantKey_o_s4"
+"x > > 3 ) & 3 ) ;"
+"switch ( shaderKey )"
+"case 0: shaderKey = ( :: G ) :: gv ; break ;"
+"case 1: shaderKey = ( :: G ) :: gv2 ; break ;"
+"case 2: shaderKey = ( :: G ) :: gv3 ; break ;"
+"} ;"
+"return ( :: G ) shaderKey ;"
+":: Empty val = ( :: Empty ) 1 ;"

tests/testhelper.py

@@ -37,10 +37,15 @@ def found(needle, haystack, negative):
     pttrn = "\s+".join(words) # free number of spaces between each needle.
     p = re.compile(pttrn)
     consumed = haystack[lastEnd:]  # we search on the rest only. not the whole string at each time.
+    #print("consumed is", fg.YELLOW, f"{consumed[:80]}...{consumed[-20:]}", fg.RESET)
     matchObject = p.search(consumed)
     if matchObject:
-        lastEnd = matchObject.end()
+        lastEnd += matchObject.end()
+        #print(f"found {fg.MAGENTA} {matchObject} {fg.RESET} last end updated to {lastEnd}")
+        #print(f"verification: consumed[span]={fg.GREEN}{consumed[matchObject.start():matchObject.end()]}{fg.RESET}\n")
         return not negative
+    #else:
+        #print(f"searched pattern was {fg.RED}{pttrn}{fg.RESET}\n")
     return negative
 
 # parse the argument mentioned in the shader source file Ex : Cmdargs: --namespace=vk   or Cmdargs: ['--unique-idx', '--root-sig', '--root-const', '0']