Hotfix/improve glsl semantic conversion (#965)

* Specify glsl semantic format - such that conversions are possible from hlsl sematics.

* Comment improvements. Give appropriate type in glsl for sv_tessfactor. Note that sv_tessfactor is not functional though.

* Work in progress for comparison of types.

* * Fix type comparison issues around the hash.
* Fix tests whos output changed with use of isTypeEqual

5 files changed, 228 insertions, 11 deletions

diff --git a/source/slang/emit.cpp b/source/slang/emit.cpp
index 675d459e9..0d5e51747 100644
--- a/source/slang/emit.cpp
+++ b/source/slang/emit.cpp
@@ -7280,6 +7280,7 @@ String emitEntryPoint(
 #endif
         validateIRModuleIfEnabled(compileRequest, irModule);
 
+
         // For GLSL only, we will need to perform "legalization" of
         // the entry point and any entry-point parameters.
         //
diff --git a/source/slang/ir-glsl-legalize.cpp b/source/slang/ir-glsl-legalize.cpp
index db23161f4..6a50d32cd 100644
--- a/source/slang/ir-glsl-legalize.cpp
+++ b/source/slang/ir-glsl-legalize.cpp
@@ -231,6 +231,8 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
 
     // HLSL semantic types can be found here
     // https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/dx-graphics-hlsl-semantics
+    /// NOTE! While there might be an "official" type for most of these in HLSL, in practice the user is allowed to declare almost anything
+    /// that the HLSL compiler can implicitly convert to/from the correct type
 
     auto builder = context->getBuilder();
     IRType* requiredType = nullptr;
@@ -267,6 +269,8 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
         {
             name = "gl_Position";
         }
+
+        requiredType = builder->getVectorType(builder->getBasicType(BaseType::Float), builder->getIntValue(builder->getIntType(), 4));
     }
     else if(semanticName == "sv_target")
     {
@@ -286,6 +290,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
         // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_ClipDistance.xhtml
 
         name = "gl_ClipDistance";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_culldistance")
     {
@@ -296,6 +301,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
 
         // TODO: type conversion is required here.
         name = "gl_CullDistance";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_coverage")
     {
@@ -315,6 +321,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
         // Float in hlsl & glsl
         // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_FragDepth.xhtml
         name = "gl_FragDepth";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_depthgreaterequal")
     {
@@ -322,6 +329,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
 
         // Type is 'unknown' in hlsl 
         name = "gl_FragDepth";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_depthlessequal")
     {
@@ -329,6 +337,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
 
         // 'unknown' in hlsl, float in glsl 
         name = "gl_FragDepth";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_dispatchthreadid")
     {
@@ -340,7 +349,6 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
     }
     else if(semanticName == "sv_domainlocation")
     {
-        // TODO: Not 100% confident that say float2 will convert into float3 glsl?
         // float2|3 in hlsl, vec3 in glsl
         // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_TessCoord.xhtml
 
@@ -360,6 +368,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
     {
         // uint in hlsl & in glsl
         name = "gl_LocalInvocationIndex";
+        requiredType = builder->getBasicType(BaseType::UInt);
     }
     else if(semanticName == "sv_groupthreadid")
     {
@@ -389,6 +398,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
         // bool in hlsl & glsl
         // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_FrontFacing.xhtml
         name = "gl_FrontFacing";
+        requiredType = builder->getBasicType(BaseType::Bool);
     }
     else if(semanticName == "sv_outputcontrolpointid")
     {
@@ -403,6 +413,7 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
     {
         // float in hlsl & glsl
         name = "gl_PointSize";
+        requiredType = builder->getBasicType(BaseType::Float);
     }
     else if(semanticName == "sv_primitiveid")
     {
@@ -456,11 +467,21 @@ GLSLSystemValueInfo* getGLSLSystemValueInfo(
     }
     else if (semanticName == "sv_tessfactor")
     {
-        // TODO(JS): Need to ensure the adjustType can handle such a scenario. 
+        // TODO(JS): Adjust type does *not* handle the conversion correctly. More specifically a float array hlsl
+        // parameter goes through code to make SOA in createGLSLGlobalVaryingsImpl.  
+        // 
+        // Can be input and output.
+        // 
+        // https://docs.microsoft.com/en-us/windows/desktop/direct3dhlsl/sv-tessfactor
+        // "Tessellation factors must be declared as an array; they cannot be packed into a single vector."
+        //
         // float[2|3|4] in hlsl, float[4] on glsl (ie both are arrays but might be different size)
         // https://www.khronos.org/registry/OpenGL-Refpages/gl4/html/gl_TessLevelOuter.xhtml
 
         name = "gl_TessLevelOuter";
+
+        // float[4] on glsl
+        requiredType = builder->getArrayType(builder->getBasicType(BaseType::Float), builder->getIntValue(builder->getIntType(), 4));
     }
     else if (semanticName == "sv_vertexid")
     {
@@ -637,7 +658,7 @@ ScalarizedVal createSimpleGLSLGlobalVarying(
             // the actual type of the GLSL global.
             auto toType = inType;
 
-            if( fromType != toType )
+            if( !isTypeEqual(fromType, toType ))
             {
                 RefPtr<ScalarizedTypeAdapterValImpl> typeAdapter = new ScalarizedTypeAdapterValImpl;
                 typeAdapter->actualType = systemValueInfo->requiredType;
diff --git a/source/slang/ir.cpp b/source/slang/ir.cpp
index 5015fda9d..00a70a2b4 100644
--- a/source/slang/ir.cpp
+++ b/source/slang/ir.cpp
@@ -1308,20 +1308,19 @@ namespace Slang
         }
     }
 
-    /// True if constants are equal
-    bool IRConstant::equal(IRConstant& rhs)
+    bool IRConstant::isValueEqual(IRConstant& rhs)
     {
         // If they are literally the same thing.. 
         if (this == &rhs)
         {
             return true;
         }
-        // Check the type and they are the same op
-        if (op != rhs.op || 
-           getFullType() != rhs.getFullType())
+        // Check the type and they are the same op & same type
+        if (op != rhs.op)
         {
             return false;
         }
+
         switch (op)
         {
             case kIROp_BoolLit:
@@ -1347,6 +1346,13 @@ namespace Slang
         return false;
     }
 
+    /// True if constants are equal
+    bool IRConstant::equal(IRConstant& rhs)
+    {
+        // TODO(JS): Only equal if pointer types are identical (to match how getHashCode works below)
+        return isValueEqual(rhs) && getFullType() == rhs.getFullType();
+    }
+
     int IRConstant::getHashCode()
     {
         auto code = Slang::GetHashCode(op);
@@ -3820,9 +3826,183 @@ namespace Slang
         writer->flush();
     }
 
-    //
-    //
-    //
+    // Pre-declare
+    static bool _isTypeOperandEqual(IRInst* a, IRInst* b);
+
+    static bool _areTypeOperandsEqual(IRInst* a, IRInst* b)
+    {
+        // Must have same number of operands
+        const auto operandCountA = Index(a->getOperandCount());
+        if (operandCountA != Index(b->getOperandCount()))
+        {
+            return false;
+        }
+
+        // All the operands must be equal
+        for (Index i = 0; i < operandCountA; ++i)
+        {
+            IRInst* operandA = a->getOperand(i);
+            IRInst* operandB = b->getOperand(i);
+
+            if (!_isTypeOperandEqual(operandA, operandB))
+            {
+                return false;
+            }
+        }
+
+        return true;
+    }
+
+    static bool _hasNominalEquality(IROp op)
+    {
+        // True if the type should be handled 'nominally' for equality
+        switch (op)
+        {
+            case kIROp_StructType:
+            case kIROp_InterfaceType:
+            case kIROp_Generic:
+            case kIROp_Param:
+            {
+                return true;
+            }
+        }
+        return false;
+    }
+
+    static bool _isNominallyEqual(IRInst* a, IRInst* b)
+    {
+        // Two instruction are nominally equal if their instruction pointer is equal
+        return a == b;
+    }
+
+    // True if a type operand is equal. Operands are 'IRInst' - but it's only a restricted set
+    // if equality of nominal types is by names alone. 
+    static bool _isTypeOperandEqual(IRInst* a, IRInst* b)
+    {
+        if (a == b)
+        {
+            return true;
+        }
+
+        if (a == nullptr || b == nullptr)
+        {
+            return false;
+        }
+
+        IROp opA = IROp(a->op & kIROpMeta_PseudoOpMask);
+        IROp opB = IROp(b->op & kIROpMeta_PseudoOpMask);
+
+        if (opA != opB)
+        {
+            return false;
+        }
+
+        // If it's a constant...
+        if (IRConstant::isaImpl(opA))
+        {
+            // TODO: This is contrived in that we want two types that are the same, but are different
+            // pointers to match here.
+            // If we make GetHashCode for IRType* compatible with isTypeEqual, then we should probably use that.
+            return static_cast<IRConstant*>(a)->isValueEqual(*static_cast<IRConstant*>(b)) &&
+                isTypeEqual(a->getFullType(), b->getFullType());
+        }
+
+        // If it's a type
+        if (IRType::isaImpl(opA))
+        {
+            return isTypeEqual(static_cast<IRType*>(a), static_cast<IRType*>(b));
+        }
+
+        if (_hasNominalEquality(opA))
+        {
+            return _isNominallyEqual(a, b);
+        }
+
+        SLANG_ASSERT(!"Unhandled comparison");
+
+        // We can't equate any other type..
+        return false;
+    }
+
+
+    bool isTypeEqual(IRType* a, IRType* b)
+    {
+        if (a == b)
+        {
+            return true;
+        }
+
+        if (a == nullptr || b == nullptr)
+        {
+            return false;
+        }
+
+        const IROp opA = IROp(a->op & kIROpMeta_PseudoOpMask);
+        const IROp opB = IROp(b->op & kIROpMeta_PseudoOpMask);
+        if (opA != opB)
+        {
+            return false;
+        }
+
+        if (IRBasicType::isaImpl(opA))
+        {
+            // If it's a basic type, then their op being the same means we are done
+            return true;
+        }
+
+        // We don't care about the parent or positioning
+        // We also don't care about 'type' - because these instructions are defining the type.
+        // 
+        // We may want to care about decorations.
+        //
+
+        if (_hasNominalEquality(opA))
+        {
+            return _isNominallyEqual(a, b);
+        }
+
+        // IRTextureType contains IRParam
+        // If it's a resource type - handle the resource flavor 
+        if (IRResourceTypeBase::isaImpl(opA))
+        {
+            auto resourceA = static_cast<const IRResourceTypeBase*>(a);
+            auto resourceB = static_cast<const IRResourceTypeBase*>(b);
+
+            if (resourceA->getFlavor() != resourceB->getFlavor())
+            {
+                return false;
+            }
+        }
+
+        if (!_areTypeOperandsEqual(a, b))
+        {
+            return false;
+        }
+
+        // TODO(JS): There is a question here about what to do about decorations.
+        // For now we ignore decorations. Are two types potentially different if there decorations different?
+        // If decorations play a part in difference in types - the order of decorations presumably is not important.
+
+        return true;
+    }
+
+    void findAllInstsBreadthFirst(IRInst* inst, List<IRInst*>& outInsts)
+    {
+        Index index = outInsts.getCount();
+
+        outInsts.add(inst);
+
+        while (index < outInsts.getCount())
+        {
+            IRInst* cur = outInsts[index++];
+
+            IRInstListBase childrenList = cur->getDecorationsAndChildren();
+            for (IRInst* child : childrenList)
+            {
+                outInsts.add(child);
+            }
+        }
+    }
 
     IRDecoration* IRInst::getFirstDecoration()
     {
diff --git a/source/slang/ir.h b/source/slang/ir.h
index 61cae5847..47c672a99 100644
--- a/source/slang/ir.h
+++ b/source/slang/ir.h
@@ -504,6 +504,13 @@ struct IRBoolType : IRBasicType
 
 SIMPLE_IR_TYPE(StringType, Type)
 
+
+// True if types are equal
+// Note compares nominal types by name alone 
+bool isTypeEqual(IRType* a, IRType* b);
+
+void findAllInstsBreadthFirst(IRInst* inst, List<IRInst*>& outInsts);
+
 // Constant Instructions
 
 typedef int64_t IRIntegerValue;
@@ -540,6 +547,10 @@ struct IRConstant : IRInst
 
         /// True if constants are equal
     bool equal(IRConstant& rhs);
+        /// True if the value is equal.
+        /// Does *NOT* compare if the type is equal. 
+    bool isValueEqual(IRConstant& rhs);
+
         /// Get the hash 
     int getHashCode();
 
diff --git a/source/slang/type-system-shared.h b/source/slang/type-system-shared.h
index 183cacb4b..95840e701 100644
--- a/source/slang/type-system-shared.h
+++ b/source/slang/type-system-shared.h
@@ -32,6 +32,7 @@ FOREACH_BASE_TYPE(DEFINE_BASE_TYPE)
 
     struct TextureFlavor
     {
+        typedef TextureFlavor ThisType;
         enum
         {
             // Mask for the overall "shape" of the texture
@@ -78,6 +79,9 @@ FOREACH_BASE_TYPE(DEFINE_BASE_TYPE)
         bool isMultisample() const { return (flavor & MultisampleFlag) != 0; }
         //            bool isShadow() const { return (flavor & ShadowFlag) != 0; }
 
+        SLANG_FORCE_INLINE bool operator==(const ThisType& rhs) const { return flavor == rhs.flavor; }
+        SLANG_FORCE_INLINE bool operator!=(const ThisType& rhs) const { return !(*this == rhs); }
+
         SlangResourceShape getShape() const { return flavor & 0xFF; }
         SlangResourceAccess getAccess() const { return (flavor >> 8) & 0xFF; }