[OPT] Avoid assert in generatecopy (#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.
diff --git a/source/opt/copy_prop_arrays.cpp b/source/opt/copy_prop_arrays.cpp
index c2bea8a..26b5ef7 100644
--- a/source/opt/copy_prop_arrays.cpp
+++ b/source/opt/copy_prop_arrays.cpp
@@ -751,6 +751,8 @@
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});
diff --git a/source/opt/fix_storage_class.cpp b/source/opt/fix_storage_class.cpp
index 367418d..b64026e 100644
--- a/source/opt/fix_storage_class.cpp
+++ b/source/opt/fix_storage_class.cpp
@@ -237,6 +237,9 @@
}
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);
}
diff --git a/source/opt/pass.cpp b/source/opt/pass.cpp
index 28f26c5..0f260e2 100644
--- a/source/opt/pass.cpp
+++ b/source/opt/pass.cpp
@@ -96,8 +96,10 @@
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 @@
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,8 +134,12 @@
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();
@@ -137,11 +147,10 @@
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
diff --git a/source/opt/pass.h b/source/opt/pass.h
index b2303e2..3e6c4d0 100644
--- a/source/opt/pass.h
+++ b/source/opt/pass.h
@@ -145,7 +145,8 @@
// 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);
diff --git a/test/opt/fix_storage_class_test.cpp b/test/opt/fix_storage_class_test.cpp
index 410f140..01a75e0 100644
--- a/test/opt/fix_storage_class_test.cpp
+++ b/test/opt/fix_storage_class_test.cpp
@@ -987,6 +987,52 @@
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
