Repository: KhronosGroup/SPIRV-Tools (language: python)

Issue:
SPIRV code fails validation after optimization
Hi,

I built a shader in slang that fails validation on `-O3`, but does not on `-O1`. The slang team redirected me here as they think the problem comes from spirv-opt.

Compiling the slang code from the [slang ticket](https://github.com/shader-slang/slang/issues/3844) gives me `shader.spv` spirv file on `-O0`, find it here: [spirv_opt.zip](https://github.com/KhronosGroup/SPIRV-Tools/files/14776613/spirv_opt.zip).

Calling `spirv-val shader.spv` from the 1.3.280 vulkan sdk gives me no validation errors for this file.

Calling `spirv-opt --validate-after-all -O shader.spv -o shaderOpt.spv` results in this validation error:
```
error: line 124: Expected Constituent type to be equal to the corresponding member type of Result Type struct
  %92 = OpCompositeConstruct %Bar %88 %121

error: line 0: Validation failed after pass simplify-instructions
```

Can you fix this issue or point me to a workaround? Thanks!


Discussion / hints:
Minimal reproducible example:

```
; SPIR-V
; Version: 1.5
; Generator: Khronos; 40
; Bound: 91
; Schema: 0
               OpCapability Int64
               OpCapability VariablePointers
               OpCapability PhysicalStorageBufferAddresses
               OpCapability Shader
               OpExtension "SPV_KHR_storage_buffer_storage_class"
               OpExtension "SPV_KHR_variable_pointers"
               OpExtension "SPV_KHR_physical_storage_buffer"
               OpMemoryModel PhysicalStorageBuffer64 GLSL450
               OpEntryPoint GLCompute %2 "main"
               OpExecutionMode %2 LocalSize 1 1 1
               OpMemberDecorate %structA 0 Offset 0
               OpMemberDecorate %structB 0 Offset 0
               OpMemberDecorate %structC 0 Offset 0
       %void = OpTypeVoid
          %3 = OpTypeFunction %void
      %ulong = OpTypeInt 64 0
 %structA = OpTypeStruct %ulong
 %structB = OpTypeStruct %ulong
 %structC = OpTypeStruct %structB
         %90 = OpUndef %structA
          %2 = OpFunction %void None %3
          %4 = OpLabel
         %45 = OpCompositeExtract %ulong %90 0
         %40 = OpCompositeConstruct %structB %45
         %34 = OpCompositeConstruct %structC %40
               OpReturn
               OpFunctionEnd
```
Before and after `simplify-instructions`:

```
; IR before pass simplify-instructions
OpCapability Int64
OpCapability VariablePointers
OpCapability PhysicalStorageBufferAddresses
OpCapability Shader
OpExtension "SPV_KHR_storage_buffer_storage_class"
OpExtension "SPV_KHR_variable_pointers"
OpExtension "SPV_KHR_physical_storage_buffer"
OpMemoryModel PhysicalStorageBuffer64 GLSL450
OpEntryPoint GLCompute %1 "main"
OpExecutionMode %1 LocalSize 1 1 1
OpMemberDecorate %_struct_2 0 Offset 0
OpMemberDecorate %_struct_3 0 Offset 0
OpMemberDecorate %_struct_4 0 Offset 0
%void = OpTypeVoid
%6 = OpTypeFunction %void
%ulong = OpTypeInt 64 0
%_struct_2 = OpTypeStruct %ulong
%_struct_3 = OpTypeStruct %ulong
%_struct_4 = OpTypeStruct %_struct_3
%8 = OpUndef %_struct_2
%1 = OpFunction %void None %6
%9 = OpLabel
%10 = OpCompositeExtract %ulong %8 0
%11 = OpCompositeConstruct %_struct_3 %10
%12 = OpCompositeConstruct %_struct_4 %11
OpReturn
OpFunctionEnd

; IR before pass scalar-replacement=100
OpCapability Int64
OpCapability VariablePointers
OpCapability PhysicalStorageBufferAddresses
OpCapability Shader
OpExtension "SPV_KHR_storage_buffer_storage_class"
OpExtension "SPV_KHR_variable_pointers"
OpExtension "SPV_KHR_physical_storage_buffer"
OpMemoryModel PhysicalStorageBuffer64 GLSL450
OpEntryPoint GLCompute %1 "main"
OpExecutionMode %1 LocalSize 1 1 1
OpMemberDecorate %_struct_2 0 Offset 0
OpMemberDecorate %_struct_3 0 Offset 0
OpMemberDecorate %_struct_4 0 Offset 0
%void = OpTypeVoid
%6 = OpTypeFunction %void
%ulong = OpTypeInt 64 0
%_struct_2 = OpTypeStruct %ulong
%_struct_3 = OpTypeStruct %ulong
%_struct_4 = OpTypeStruct %_struct_3
%8 = OpUndef %_struct_2
%1 = OpFunction %void None %6
%9 = OpLabel
%10 = OpCompositeExtract %ulong %8 0
%12 = OpCompositeConstruct %_struct_4 %8
OpReturn
OpFunctionEnd
```
This look like an issues with the isomorphic structs. This confuses the type manager when the type manager is not used correctly. This is a known source of bugs, but there is not enough activity on spirv-opt to make a major change to the type manager.

I'll try fixing the code in this one place, and check if I can add an assert that might catch some of these cases. Assuming my guess at the root cause is correct.

Fix patch:
diff --git a/source/opt/folding_rules.cpp b/source/opt/folding_rules.cpp
index 5f83669940..24979671f2 100644
--- a/source/opt/folding_rules.cpp
+++ b/source/opt/folding_rules.cpp
@@ -1607,27 +1607,26 @@ bool CompositeConstructFeedingExtract(
 }
 
 // Walks the indexes chain from |start| to |end| of an OpCompositeInsert or
-// OpCompositeExtract instruction, and returns the type of the final element
-// being accessed.
-const analysis::Type* GetElementType(uint32_t type_id,
-                                     Instruction::iterator start,
-                                     Instruction::iterator end,
-                                     const analysis::TypeManager* type_mgr) {
-  const analysis::Type* type = type_mgr->GetType(type_id);
+// OpCompositeExtract instruction, and returns the type id of the final element
+// being accessed. Returns 0 if a valid type could not be found.
+uint32_t GetElementType(uint32_t type_id, Instruction::iterator start,
+                        Instruction::iterator end,
+                        const analysis::DefUseManager* def_use_manager) {
   for (auto index : make_range(std::move(start), std::move(end))) {
+    const Instruction* type_inst = def_use_manager->GetDef(type_id);
     assert(index.type == SPV_OPERAND_TYPE_LITERAL_INTEGER &&
            index.words.size() == 1);
-    if (auto* array_type = type->AsArray()) {
-      type = array_type->element_type();
-    } else if (auto* matrix_type = type->AsMatrix()) {
-      type = matrix_type->element_type();
-    } else if (auto* struct_type = type->AsStruct()) {
-      type = struct_type->element_types()[index.words[0]];
+    if (type_inst->opcode() == spv::Op::OpTypeArray) {
+      type_id = type_inst->GetSingleWordInOperand(0);
+    } else if (type_inst->opcode() == spv::Op::OpTypeMatrix) {
+      type_id = type_inst->GetSingleWordInOperand(0);
+    } else if (type_inst->opcode() == spv::Op::OpTypeStruct) {
+      type_id = type_inst->GetSingleWordInOperand(index.words[0]);
     } else {
-      type = nullptr;
+      return 0;
     }
   }
-  return type;
+  return type_id;
 }
 
 // Returns true of |inst_1| and |inst_2| have the same indexes that will be used
@@ -1712,16 +1711,11 @@ bool CompositeExtractFeedingConstruct(
   // The last check it to see that the object being extracted from is the
   // correct type.
   Instruction* original_inst = def_use_mgr->GetDef(original_id);
-  analysis::TypeManager* type_mgr = context->get_type_mgr();
-  const analysis::Type* original_type =
+  uint32_t original_type_id =
       GetElementType(original_inst->type_id(), first_element_inst->begin() + 3,
-                     first_element_inst->end() - 1, type_mgr);
-
-  if (original_type == nullptr) {
-    return false;
-  }
+                     first_element_inst->end() - 1, def_use_mgr);
 
-  if (inst->type_id() != type_mgr->GetId(original_type)) {
+  if (inst->type_id() != original_type_id) {
     return false;
   }
 
@@ -2011,13 +2005,15 @@ bool DoInsertedValuesCoverEntireObject(
   return true;
 }
 
-// Returns the type of the element that immediately contains the element being
-// inserted by the OpCompositeInsert instruction |inst|.
-const analysis::Type* GetContainerType(Instruction* inst) {
+// Returns id of the type of the element that immediately contains the element
+// being inserted by the OpCompositeInsert instruction |inst|. Returns 0 if it
+// could not be found.
+uint32_t GetContainerTypeId(Instruction* inst) {
   assert(inst->opcode() == spv::Op::OpCompositeInsert);
-  analysis::TypeManager* type_mgr = inst->context()->get_type_mgr();
-  return GetElementType(inst->type_id(), inst->begin() + 4, inst->end() - 1,
-                        type_mgr);
+  analysis::DefUseManager* def_use_manager = inst->context()->get_def_use_mgr();
+  uint32_t container_type_id = GetElementType(
+      inst->type_id(), inst->begin() + 4, inst->end() - 1, def_use_manager);
+  return container_type_id;
 }
 
 // Returns an OpCompositeConstruct instruction that build an object with
@@ -2064,18 +2060,20 @@ bool CompositeInsertToCompositeConstruct(
   if (inst->NumInOperands() < 3) return false;
 
   std::map<uint32_t, uint32_t> values_inserted = GetInsertedValues(inst);
-  const analysis::Type* container_type = GetContainerType(inst);
-  if (container_type == nullptr) {
+  uint32_t container_type_id = GetContainerTypeId(inst);
+  if (container_type_id == 0) {
     return false;
   }
 
+  analysis::TypeManager* type_mgr = context->get_type_mgr();
+  const analysis::Type* container_type = type_mgr->GetType(container_type_id);
+  assert(container_type && "GetContainerTypeId returned a bad id.");
   if (!DoInsertedValuesCoverEntireObject(container_type, values_inserted)) {
     return false;
   }
 
-  analysis::TypeManager* type_mgr = context->get_type_mgr();
-  Instruction* construct = BuildCompositeConstruct(
-      type_mgr->GetId(container_type), values_inserted, inst);
+  Instruction* construct =
+      BuildCompositeConstruct(container_type_id, values_inserted, inst);
   InsertConstructedObject(inst, construct);
   return true;
 }


Test patch:
diff --git a/test/opt/fold_test.cpp b/test/opt/fold_test.cpp
index 255449dbbf..35828ab22f 100644
--- a/test/opt/fold_test.cpp
+++ b/test/opt/fold_test.cpp
@@ -7827,7 +7827,21 @@ ::testing::Values(
             "%5 = OpCompositeInsert %int_arr_2 %int_1 %4 1\n" +
             "OpReturn\n" +
             "OpFunctionEnd",
-        5, true)
+        5, true),
+    // Test case 19: Don't fold for isomorphic structs
+    InstructionFoldingCase<bool>(
+        Header() +
+            "%structA = OpTypeStruct %ulong\n" +
+            "%structB = OpTypeStruct %ulong\n" +
+            "%structC = OpTypeStruct %structB\n" +
+            "%struct_a_undef = OpUndef %structA\n" +
+            "%main = OpFunction %void None %void_func\n" +
+            "%main_lab = OpLabel\n" +
+            "%3 = OpCompositeExtract %ulong %struct_a_undef 0\n" +
+            "%4 = OpCompositeConstruct %structB %3\n" +
+            "OpReturn\n" +
+            "OpFunctionEnd",
+        4, false)
 ));
 
 INSTANTIATE_TEST_SUITE_P(DotProductMatchingTest, MatchingInstructionFoldingTest,