Hotfix/improve glsl semantic conversion review (#968)

* Small changes based on review

* Remove the explicit 'nominal' tests
* Made isValueEqual and isEqual on on IRConstant take a pointer
* Small improvements to comments, and clarity of using 'nominal'

* Simplify comparison by just using isTypeOperandEqual as basis for isTypeEqual

* Use cross compile to test half-texture.slang on glsl

* Don't need half-texture.slang.expected

* Fix handling of nominal comparison based on review, ensuring that for nominal insts, they can only be compared by pointer.

6 files changed, 99 insertions, 276 deletions

diff --git a/source/slang/ir-insts.h b/source/slang/ir-insts.h
index 0dc4dbbac..20279e3aa 100644
--- a/source/slang/ir-insts.h
+++ b/source/slang/ir-insts.h
@@ -671,7 +671,7 @@ struct IRConstantKey
 {
     IRConstant* inst;
 
-    bool operator==(const IRConstantKey& rhs) const { return inst->equal(*rhs.inst); }
+    bool operator==(const IRConstantKey& rhs) const { return inst->equal(rhs.inst); }
     int GetHashCode() const { return inst->getHashCode(); }
 };
 
diff --git a/source/slang/ir.cpp b/source/slang/ir.cpp
index 00a70a2b4..1045b4884 100644
--- a/source/slang/ir.cpp
+++ b/source/slang/ir.cpp
@@ -1308,15 +1308,15 @@ namespace Slang
         }
     }
 
-    bool IRConstant::isValueEqual(IRConstant& rhs)
+    bool IRConstant::isValueEqual(IRConstant* rhs)
     {
         // If they are literally the same thing.. 
-        if (this == &rhs)
+        if (this == rhs)
         {
             return true;
         }
         // Check the type and they are the same op & same type
-        if (op != rhs.op)
+        if (op != rhs->op)
         {
             return false;
         }
@@ -1329,15 +1329,15 @@ namespace Slang
             {
                 SLANG_COMPILE_TIME_ASSERT(sizeof(IRFloatingPointValue) == sizeof(IRIntegerValue));
                 // ... we can just compare as bits
-                return value.intVal == rhs.value.intVal;
+                return value.intVal == rhs->value.intVal;
             }
             case kIROp_PtrLit:
             {
-                return value.ptrVal == rhs.value.ptrVal;
+                return value.ptrVal == rhs->value.ptrVal;
             }
             case kIROp_StringLit:
             {
-                return getStringSlice() == rhs.getStringSlice();
+                return getStringSlice() == rhs->getStringSlice();
             }
             default: break;
         }
@@ -1347,10 +1347,10 @@ namespace Slang
     }
 
     /// True if constants are equal
-    bool IRConstant::equal(IRConstant& rhs)
+    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();
+        return isValueEqual(rhs) && getFullType() == rhs->getFullType();
     }
 
     int IRConstant::getHashCode()
@@ -3853,9 +3853,9 @@ namespace Slang
         return true;
     }
 
-    static bool _hasNominalEquality(IROp op)
+    static bool _isNominalOp(IROp op)
     {
-        // True if the type should be handled 'nominally' for equality
+        // True if the op identity is 'nominal'
         switch (op)
         {
             case kIROp_StructType:
@@ -3869,14 +3869,8 @@ namespace Slang
         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. 
+    // True if a type operand is equal. Operands are 'IRInst' - but it's only a restricted set that
+    // can be operands of IRType instructions
     static bool _isTypeOperandEqual(IRInst* a, IRInst* b)
     {
         if (a == b)
@@ -3889,33 +3883,58 @@ namespace Slang
             return false;
         }
 
-        IROp opA = IROp(a->op & kIROpMeta_PseudoOpMask);
-        IROp opB = IROp(b->op & kIROpMeta_PseudoOpMask);
+        const IROp opA = IROp(a->op & kIROpMeta_PseudoOpMask);
+        const IROp opB = IROp(b->op & kIROpMeta_PseudoOpMask);
 
         if (opA != opB)
         {
             return false;
         }
 
-        // If it's a constant...
-        if (IRConstant::isaImpl(opA))
+        // If the type is nominal - it can only be the same if the pointer is the same.
+        if (_isNominalOp(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());
+            // The pointer isn't the same (as that was already tested), so cannot be equal
+            return false;
         }
 
-        // If it's a type
+        // Both are types
         if (IRType::isaImpl(opA))
         {
-            return isTypeEqual(static_cast<IRType*>(a), static_cast<IRType*>(b));
-        }
+            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 it's a resource type - special case the handling of the resource flavor 
+            if (IRResourceTypeBase::isaImpl(opA) &&
+                static_cast<const IRResourceTypeBase*>(a)->getFlavor() != static_cast<const IRResourceTypeBase*>(b)->getFlavor())
+            {
+                return false;
+            }
 
-        if (_hasNominalEquality(opA))
+            // 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.
+
+            // All the operands of the types must be equal
+            return _areTypeOperandsEqual(a, b);
+        }
+       
+        // If it's a constant...
+        if (IRConstant::isaImpl(opA))
         {
-            return _isNominallyEqual(a, b);
+            // 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());
         }
 
         SLANG_ASSERT(!"Unhandled comparison");
@@ -3924,68 +3943,12 @@ namespace Slang
         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;
+        // _isTypeOperandEqual handles comparison of types so can defer to it
+        return _isTypeOperandEqual(a, b);
     }
-
+     
     void findAllInstsBreadthFirst(IRInst* inst, List<IRInst*>& outInsts)
     {
         Index index = outInsts.getCount();
diff --git a/source/slang/ir.h b/source/slang/ir.h
index 47c672a99..e7f9dff75 100644
--- a/source/slang/ir.h
+++ b/source/slang/ir.h
@@ -546,10 +546,10 @@ struct IRConstant : IRInst
     UnownedStringSlice getStringSlice();
 
         /// True if constants are equal
-    bool equal(IRConstant& rhs);
+    bool equal(IRConstant* rhs);
         /// True if the value is equal.
         /// Does *NOT* compare if the type is equal. 
-    bool isValueEqual(IRConstant& rhs);
+    bool isValueEqual(IRConstant* rhs);
 
         /// Get the hash 
     int getHashCode();
diff --git a/tests/compute/half-texture.slang b/tests/compute/half-texture.slang
index 635336c66..3f2c5500e 100644
--- a/tests/compute/half-texture.slang
+++ b/tests/compute/half-texture.slang
@@ -1,4 +1,4 @@
-//TEST:SIMPLE: -target spirv -entry computeMain -profile cs_6_2
+//TEST:CROSS_COMPILE: -target spirv -entry computeMain -profile cs_6_2
 //TEST:SIMPLE: -target hlsl -entry computeMain -profile cs_6_2 
 
 //TEST_INPUT:ubuffer(data=[0 0 0 0 0 0 0 0 0 0 0 0 0 0 0 0], stride=16):dxbinding(0),glbinding(0),out
diff --git a/tests/compute/half-texture.slang.expected b/tests/compute/half-texture.slang.expected
deleted file mode 100644
index eb1ee705f..000000000
--- a/tests/compute/half-texture.slang.expected
+++ /dev/null
@@ -1,181 +0,0 @@
-result code = 0
-standard error = {
-}
-standard output = {
-// Module Version 10000
-// Generated by (magic number): 80007
-// Id's are bound by 125
-
-                              Capability Shader
-                              Capability Float16
-                              Capability StorageImageExtendedFormats
-               1:             ExtInstImport  "GLSL.std.450"
-                              MemoryModel Logical GLSL450
-                              EntryPoint GLCompute 4  "main" 13
-                              ExecutionMode 4 LocalSize 4 4 1
-                              Source GLSL 450
-                              SourceExtension  "GL_EXT_shader_16bit_storage"
-                              SourceExtension  "GL_EXT_shader_explicit_arithmetic_types"
-                              SourceExtension  "GL_GOOGLE_cpp_style_line_directive"
-                              Name 4  "main"
-                              Name 9  "pos_0"
-                              Name 13  "gl_GlobalInvocationID"
-                              Name 18  "pos2_0"
-                              Name 32  "h_0"
-                              Name 36  "halfTexture_0"
-                              Name 47  "h2_0"
-                              Name 50  "halfTexture2_0"
-                              Name 61  "h4_0"
-                              Name 64  "halfTexture4_0"
-                              Name 104  "index_0"
-                              Name 113  "_S1"
-                              MemberName 113(_S1) 0  "_data"
-                              Name 115  "outputBuffer_0"
-                              Decorate 13(gl_GlobalInvocationID) BuiltIn GlobalInvocationId
-                              Decorate 36(halfTexture_0) DescriptorSet 0
-                              Decorate 36(halfTexture_0) Binding 1
-                              Decorate 50(halfTexture2_0) DescriptorSet 0
-                              Decorate 50(halfTexture2_0) Binding 2
-                              Decorate 64(halfTexture4_0) DescriptorSet 0
-                              Decorate 64(halfTexture4_0) Binding 3
-                              Decorate 112 ArrayStride 4
-                              MemberDecorate 113(_S1) 0 Offset 0
-                              Decorate 113(_S1) BufferBlock
-                              Decorate 115(outputBuffer_0) DescriptorSet 0
-                              Decorate 115(outputBuffer_0) Binding 0
-                              Decorate 124 BuiltIn WorkgroupSize
-               2:             TypeVoid
-               3:             TypeFunction 2
-               6:             TypeInt 32 1
-               7:             TypeVector 6(int) 2
-               8:             TypePointer Function 7(ivec2)
-              10:             TypeInt 32 0
-              11:             TypeVector 10(int) 3
-              12:             TypePointer Input 11(ivec3)
-13(gl_GlobalInvocationID):     12(ptr) Variable Input
-              14:             TypeVector 10(int) 2
-              19:      6(int) Constant 3
-              20:     10(int) Constant 1
-              21:             TypePointer Function 6(int)
-              25:     10(int) Constant 0
-              30:             TypeFloat 16
-              31:             TypePointer Function 30(float16_t)
-              33:             TypeFloat 32
-              34:             TypeImage 33(float) 2D nonsampled format:R16f
-              35:             TypePointer UniformConstant 34
-36(halfTexture_0):     35(ptr) Variable UniformConstant
-              41:             TypeVector 33(float) 4
-              45:             TypeVector 30(float16_t) 2
-              46:             TypePointer Function 45(f16vec2)
-              48:             TypeImage 33(float) 2D nonsampled format:Rg16f
-              49:             TypePointer UniformConstant 48
-50(halfTexture2_0):     49(ptr) Variable UniformConstant
-              56:             TypeVector 33(float) 2
-              59:             TypeVector 30(float16_t) 4
-              60:             TypePointer Function 59(f16vec4)
-              62:             TypeImage 33(float) 2D nonsampled format:Rgba16f
-              63:             TypePointer UniformConstant 62
-64(halfTexture4_0):     63(ptr) Variable UniformConstant
-              80:30(float16_t) Constant 0
-             109:      6(int) Constant 4
-             112:             TypeRuntimeArray 6(int)
-        113(_S1):             TypeStruct 112
-             114:             TypePointer Uniform 113(_S1)
-115(outputBuffer_0):    114(ptr) Variable Uniform
-             116:      6(int) Constant 0
-             120:             TypePointer Uniform 6(int)
-             123:     10(int) Constant 4
-             124:   11(ivec3) ConstantComposite 123 123 20
-         4(main):           2 Function None 3
-               5:             Label
-        9(pos_0):      8(ptr) Variable Function
-      18(pos2_0):      8(ptr) Variable Function
-         32(h_0):     31(ptr) Variable Function
-        47(h2_0):     46(ptr) Variable Function
-        61(h4_0):     60(ptr) Variable Function
-    104(index_0):     21(ptr) Variable Function
-              15:   11(ivec3) Load 13(gl_GlobalInvocationID)
-              16:   14(ivec2) VectorShuffle 15 15 0 1
-              17:    7(ivec2) Bitcast 16
-                              Store 9(pos_0) 17
-              22:     21(ptr) AccessChain 9(pos_0) 20
-              23:      6(int) Load 22
-              24:      6(int) ISub 19 23
-              26:     21(ptr) AccessChain 9(pos_0) 25
-              27:      6(int) Load 26
-              28:      6(int) ISub 19 27
-              29:    7(ivec2) CompositeConstruct 24 28
-                              Store 18(pos2_0) 29
-              37:          34 Load 36(halfTexture_0)
-              38:    7(ivec2) Load 18(pos2_0)
-              39:   14(ivec2) Bitcast 38
-              40:    7(ivec2) Bitcast 39
-              42:   41(fvec4) ImageRead 37 40
-              43:   33(float) CompositeExtract 42 0
-              44:30(float16_t) FConvert 43
-                              Store 32(h_0) 44
-              51:          48 Load 50(halfTexture2_0)
-              52:    7(ivec2) Load 18(pos2_0)
-              53:   14(ivec2) Bitcast 52
-              54:    7(ivec2) Bitcast 53
-              55:   41(fvec4) ImageRead 51 54
-              57:   56(fvec2) VectorShuffle 55 55 0 1
-              58: 45(f16vec2) FConvert 57
-                              Store 47(h2_0) 58
-              65:          62 Load 64(halfTexture4_0)
-              66:    7(ivec2) Load 18(pos2_0)
-              67:   14(ivec2) Bitcast 66
-              68:    7(ivec2) Bitcast 67
-              69:   41(fvec4) ImageRead 65 68
-              70: 59(f16vec4) FConvert 69
-                              Store 61(h4_0) 70
-              71:          34 Load 36(halfTexture_0)
-              72:    7(ivec2) Load 9(pos_0)
-              73:   14(ivec2) Bitcast 72
-              74:    7(ivec2) Bitcast 73
-              75:     31(ptr) AccessChain 47(h2_0) 25
-              76:30(float16_t) Load 75
-              77:     31(ptr) AccessChain 47(h2_0) 20
-              78:30(float16_t) Load 77
-              79:30(float16_t) FAdd 76 78
-              81: 59(f16vec4) CompositeConstruct 79 80 80 80
-              82:   41(fvec4) FConvert 81
-                              ImageWrite 71 74 82
-              83:          48 Load 50(halfTexture2_0)
-              84:    7(ivec2) Load 9(pos_0)
-              85:   14(ivec2) Bitcast 84
-              86:    7(ivec2) Bitcast 85
-              87: 59(f16vec4) Load 61(h4_0)
-              88: 45(f16vec2) VectorShuffle 87 87 0 1
-              89:30(float16_t) CompositeExtract 88 0
-              90:30(float16_t) CompositeExtract 88 1
-              91: 59(f16vec4) CompositeConstruct 89 90 80 80
-              92:   41(fvec4) FConvert 91
-                              ImageWrite 83 86 92
-              93:          62 Load 64(halfTexture4_0)
-              94:    7(ivec2) Load 9(pos_0)
-              95:   14(ivec2) Bitcast 94
-              96:    7(ivec2) Bitcast 95
-              97: 45(f16vec2) Load 47(h2_0)
-              98:30(float16_t) Load 32(h_0)
-              99:30(float16_t) Load 32(h_0)
-             100:30(float16_t) CompositeExtract 97 0
-             101:30(float16_t) CompositeExtract 97 1
-             102: 59(f16vec4) CompositeConstruct 100 101 98 99
-             103:   41(fvec4) FConvert 102
-                              ImageWrite 93 96 103
-             105:     21(ptr) AccessChain 9(pos_0) 25
-             106:      6(int) Load 105
-             107:     21(ptr) AccessChain 9(pos_0) 20
-             108:      6(int) Load 107
-             110:      6(int) IMul 108 109
-             111:      6(int) IAdd 106 110
-                              Store 104(index_0) 111
-             117:      6(int) Load 104(index_0)
-             118:     10(int) Bitcast 117
-             119:      6(int) Load 104(index_0)
-             121:    120(ptr) AccessChain 115(outputBuffer_0) 116 118
-                              Store 121 119
-                              Return
-                              FunctionEnd
-}
diff --git a/tests/compute/half-texture.slang.glsl b/tests/compute/half-texture.slang.glsl
new file mode 100644
index 000000000..88f585378
--- /dev/null
+++ b/tests/compute/half-texture.slang.glsl
@@ -0,0 +1,41 @@
+//TEST_IGNORE_FILE:
+#version 450
+layout(row_major) uniform;
+layout(row_major) buffer;
+#extension GL_EXT_shader_16bit_storage : require
+#extension GL_EXT_shader_explicit_arithmetic_types : require
+
+layout(r16f)
+layout(binding = 1)
+uniform image2D halfTexture_0;
+
+layout(rg16f)
+layout(binding = 2)
+uniform image2D halfTexture2_0;
+
+layout(rgba16f)
+layout(binding = 3)
+uniform image2D halfTexture4_0;
+
+layout(std430, binding = 0) buffer _S1 {
+    int _data[];
+} outputBuffer_0;
+
+layout(local_size_x = 4, local_size_y = 4, local_size_z = 1) in;void main()
+{
+    ivec2 pos_0 = ivec2(gl_GlobalInvocationID.xy);
+    const float _S2 = 1.00000000000000000000 / 3.00000000000000000000;
+    ivec2 pos2_0 = ivec2(3 - pos_0.y, 3 - pos_0.x);
+
+    float16_t h_0 = (float16_t(imageLoad((halfTexture_0), ivec2((uvec2(pos2_0)))).x));
+    f16vec2 h2_0 = (f16vec2(imageLoad((halfTexture2_0), ivec2((uvec2(pos2_0)))).xy));
+    f16vec4 h4_0 = (f16vec4(imageLoad((halfTexture4_0), ivec2((uvec2(pos2_0))))));
+	imageStore((halfTexture_0), ivec2((uvec2(pos_0))), f16vec4(h2_0.x + h2_0.y, float16_t(0), float16_t(0), float16_t(0)));
+	imageStore((halfTexture2_0), ivec2((uvec2(pos_0))), f16vec4(h4_0.xy, float16_t(0), float16_t(0)));
+	imageStore((halfTexture4_0), ivec2((uvec2(pos_0))), f16vec4(h2_0, h_0, h_0));
+
+    int index_0 = pos_0.x + pos_0.y * 4;
+    ((outputBuffer_0)._data[(uint(index_0))]) = index_0;
+
+    return;
+}
\ No newline at end of file