[OPT] Avoid assert in generatecopy (KhronosGroup#5756)

We want to be able to recover when fix storage class is not able to fix
everything, and just leave the spir-v in an invalid state. The pass
should not fail because of that.

Author: s-perron

source/opt/copy_prop_arrays.cpp

@@ -751,6 +751,8 @@ void CopyPropagateArrays::UpdateUses(Instruction* original_ptr_inst,
           uint32_t pointee_type_id =
               pointer_type->GetSingleWordInOperand(kTypePointerPointeeInIdx);
           uint32_t copy = GenerateCopy(original_ptr_inst, pointee_type_id, use);
+          assert(copy != 0 &&
+                 "Should not be updating uses unless we know it can be done.");
 
           context()->ForgetUses(use);
           use->SetInOperand(index, {copy});

source/opt/fix_storage_class.cpp

@@ -237,6 +237,9 @@ bool FixStorageClass::PropagateType(Instruction* inst, uint32_t type_id,
         }
 
         uint32_t copy_id = GenerateCopy(obj_inst, pointee_type_id, inst);
+        if (copy_id == 0) {
+          return false;
+        }
         inst->SetInOperand(1, {copy_id});
         context()->UpdateDefUse(inst);
       }

source/opt/pass.cpp

@@ -96,8 +96,10 @@ uint32_t Pass::GenerateCopy(Instruction* object_to_copy, uint32_t new_type_id,
 
   Instruction* original_type = get_def_use_mgr()->GetDef(original_type_id);
   Instruction* new_type = get_def_use_mgr()->GetDef(new_type_id);
-  assert(new_type->opcode() == original_type->opcode() &&
-         "Can't copy an aggragate type unless the type correspond.");
+
+  if (new_type->opcode() != original_type->opcode()) {
+    return 0;
+  }
 
   switch (original_type->opcode()) {
     case spv::Op::OpTypeArray: {
@@ -114,8 +116,12 @@ uint32_t Pass::GenerateCopy(Instruction* object_to_copy, uint32_t new_type_id,
       for (uint32_t i = 0; i < array_length; i++) {
         Instruction* extract = ir_builder.AddCompositeExtract(
             original_element_type_id, object_to_copy->result_id(), {i});
-        element_ids.push_back(
-            GenerateCopy(extract, new_element_type_id, insertion_position));
+        uint32_t new_id =
+            GenerateCopy(extract, new_element_type_id, insertion_position);
+        if (new_id == 0) {
+          return 0;
+        }
+        element_ids.push_back(new_id);
       }
 
       return ir_builder.AddCompositeConstruct(new_type_id, element_ids)
@@ -128,20 +134,23 @@ uint32_t Pass::GenerateCopy(Instruction* object_to_copy, uint32_t new_type_id,
         uint32_t new_member_type_id = new_type->GetSingleWordInOperand(i);
         Instruction* extract = ir_builder.AddCompositeExtract(
             orig_member_type_id, object_to_copy->result_id(), {i});
-        element_ids.push_back(
-            GenerateCopy(extract, new_member_type_id, insertion_position));
+        uint32_t new_id =
+            GenerateCopy(extract, new_member_type_id, insertion_position);
+        if (new_id == 0) {
+          return 0;
+        }
+        element_ids.push_back(new_id);
       }
       return ir_builder.AddCompositeConstruct(new_type_id, element_ids)
           ->result_id();
     }
     default:
       // If we do not have an aggregate type, then we have a problem.  Either we
       // found multiple instances of the same type, or we are copying to an
-      // incompatible type.  Either way the code is illegal.
-      assert(false &&
-             "Don't know how to copy this type.  Code is likely illegal.");
+      // incompatible type.  Either way the code is illegal. Leave the code as
+      // is and let the caller deal with it.
+      return 0;
   }
-  return 0;
 }
 
 }  // namespace opt

source/opt/pass.h

@@ -145,7 +145,8 @@ class Pass {
 
   // Returns the id whose value is the same as |object_to_copy| except its type
   // is |new_type_id|.  Any instructions needed to generate this value will be
-  // inserted before |insertion_position|.
+  // inserted before |insertion_position|. Returns 0 if a copy could not be
+  // done.
   uint32_t GenerateCopy(Instruction* object_to_copy, uint32_t new_type_id,
                         Instruction* insertion_position);
 

test/opt/fix_storage_class_test.cpp

@@ -987,6 +987,52 @@ OpFunctionEnd
   SinglePassRunAndCheck<FixStorageClass>(text, text, false);
 }
 
+// This example is generated by DXC when certain inline spiir-v is used.
+// The intention is that the function scope variable will eventually be
+// optimized away, removing the type mismatch. We want to make sure the
+// OpCopyObject is rewritten, and that the pass does not fail.
+TEST_F(FixStorageClassTest, DoNotFailWithMismatchedPointerTypes) {
+  const std::string text = R"(
+               OpCapability Shader
+               OpMemoryModel Logical GLSL450
+               OpEntryPoint GLCompute %1 "main" %38
+               OpExecutionMode %1 LocalSize 64 1 1
+               OpSource HLSL 600
+        %int = OpTypeInt 32 1
+      %int_0 = OpConstant %int 0
+      %float = OpTypeFloat 32
+       %uint = OpTypeInt 32 0
+    %uint_64 = OpConstant %uint 64
+%_arr_float_uint_64 = OpTypeArray %float %uint_64
+%_ptr_Workgroup__arr_float_uint_64 = OpTypePointer Workgroup %_arr_float_uint_64
+       %void = OpTypeVoid
+         %80 = OpTypeFunction %void
+%_ptr_Workgroup_float = OpTypePointer Workgroup %float
+%_ptr_Function__ptr_Workgroup_float = OpTypePointer Function %_ptr_Workgroup_float
+%_ptr_Workgroup_float_0 = OpTypePointer Workgroup %float
+         %38 = OpVariable %_ptr_Workgroup__arr_float_uint_64 Workgroup
+          %1 = OpFunction %void None %80
+         %98 = OpLabel
+; CHECK: [[var:%\d+]] = OpVariable %_ptr_Function__ptr_Workgroup_float Function
+        %113 = OpVariable %_ptr_Function__ptr_Workgroup_float Function
+; CHECK: [[ac:%\d+]] = OpAccessChain %_ptr_Workgroup_float_0 {{%\d+}} %int_0
+        %136 = OpAccessChain %_ptr_Workgroup_float_0 %38 %int_0
+; Verify that the type for the OpCopyObject has changed to match [[ac]].
+; CHECK: [[copy:%\d+]] = OpCopyObject %_ptr_Workgroup_float_0 [[ac]]
+        %137 = OpCopyObject %_ptr_Workgroup_float %136
+; This has a type mismatch, but this is because we do not have a way to copy
+; a pointer from one type to another, so FixStorageClass cannot do anything
+; about it. We want fix storage class to leave it as is, and the validator
+; will report an error if the store is not remove by a later optimization.
+; CHECK: OpStore [[var]] [[copy]]
+               OpStore %113 %137
+               OpReturn
+               OpFunctionEnd
+)";
+
+  SinglePassRunAndMatch<FixStorageClass>(text, false);
+}
+
 }  // namespace
 }  // namespace opt
 }  // namespace spvtools