Disallow use of OpCompositeExtract/OpCompositeInsert with no indices (#2980)
diff --git a/source/opt/folding_rules.cpp b/source/opt/folding_rules.cpp
index d2d029d..e54a0cd 100644
--- a/source/opt/folding_rules.cpp
+++ b/source/opt/folding_rules.cpp
@@ -1407,20 +1407,6 @@
};
}
-// Folds an OpCompositeExtract instruction with no indexes into an OpCopyObject.
-bool ExtractWithNoIndexes(IRContext*, Instruction* inst,
- const std::vector<const analysis::Constant*>&) {
- assert(inst->opcode() == SpvOpCompositeExtract &&
- "Wrong opcode. Should be OpCompositeExtract.");
-
- if (inst->NumInOperands() > 1) {
- return false;
- }
-
- inst->SetOpcode(SpvOpCopyObject);
- return true;
-}
-
FoldingRule InsertFeedingExtract() {
return [](IRContext* context, Instruction* inst,
const std::vector<const analysis::Constant*>&) {
@@ -2227,7 +2213,6 @@
// Take that into consideration.
rules_[SpvOpCompositeConstruct].push_back(CompositeExtractFeedingConstruct());
- rules_[SpvOpCompositeExtract].push_back(ExtractWithNoIndexes);
rules_[SpvOpCompositeExtract].push_back(InsertFeedingExtract());
rules_[SpvOpCompositeExtract].push_back(CompositeConstructFeedingExtract());
rules_[SpvOpCompositeExtract].push_back(VectorShuffleFeedingExtract());
diff --git a/source/val/validate_composites.cpp b/source/val/validate_composites.cpp
index 9b7f592..eb8a324 100644
--- a/source/val/validate_composites.cpp
+++ b/source/val/validate_composites.cpp
@@ -30,8 +30,8 @@
// OpCompositeInsert instruction. The function traverses the hierarchy of
// nested data structures (structs, arrays, vectors, matrices) as directed by
// the sequence of indices in the instruction. May return error if traversal
-// fails (encountered non-composite, out of bounds, nesting too deep).
-// Returns the type of Composite operand if the instruction has no indices.
+// fails (encountered non-composite, out of bounds, no indices, nesting too
+// deep).
spv_result_t GetExtractInsertValueType(ValidationState_t& _,
const Instruction* inst,
uint32_t* member_type) {
@@ -40,10 +40,15 @@
uint32_t word_index = opcode == SpvOpCompositeExtract ? 4 : 5;
const uint32_t num_words = static_cast<uint32_t>(inst->words().size());
const uint32_t composite_id_index = word_index - 1;
-
const uint32_t num_indices = num_words - word_index;
const uint32_t kCompositeExtractInsertMaxNumIndices = 255;
- if (num_indices > kCompositeExtractInsertMaxNumIndices) {
+
+ if (num_indices == 0) {
+ return _.diag(SPV_ERROR_INVALID_DATA, inst)
+ << "Expected at least one index to Op"
+ << spvOpcodeString(inst->opcode()) << ", zero found";
+
+ } else if (num_indices > kCompositeExtractInsertMaxNumIndices) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "The number of indexes in Op" << spvOpcodeString(opcode)
<< " may not exceed " << kCompositeExtractInsertMaxNumIndices
@@ -386,20 +391,20 @@
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Cannot extract from a composite of 8- or 16-bit types";
}
+
return SPV_SUCCESS;
}
spv_result_t ValidateCompositeInsert(ValidationState_t& _,
const Instruction* inst) {
- const SpvOp opcode = inst->opcode();
const uint32_t object_type = _.GetOperandTypeId(inst, 2);
const uint32_t composite_type = _.GetOperandTypeId(inst, 3);
const uint32_t result_type = inst->type_id();
if (result_type != composite_type) {
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "The Result Type must be the same as Composite type in Op"
- << spvOpcodeString(opcode) << " yielding Result Id " << result_type
- << ".";
+ << spvOpcodeString(inst->opcode()) << " yielding Result Id "
+ << result_type << ".";
}
uint32_t member_type = 0;
@@ -421,6 +426,7 @@
return _.diag(SPV_ERROR_INVALID_DATA, inst)
<< "Cannot insert into a composite of 8- or 16-bit types";
}
+
return SPV_SUCCESS;
}
diff --git a/test/opt/fold_test.cpp b/test/opt/fold_test.cpp
index 5c2adec..cb81f1c 100644
--- a/test/opt/fold_test.cpp
+++ b/test/opt/fold_test.cpp
@@ -3333,16 +3333,7 @@
"%3 = OpCompositeExtract %float %2 4\n" +
"OpReturn\n" +
"OpFunctionEnd",
- 3, 0),
- // Test case 14: Fold OpCompositeExtract with no indexes.
- InstructionFoldingCase<uint32_t>(
- Header() + "%main = OpFunction %void None %void_func\n" +
- "%main_lab = OpLabel\n" +
- "%2 = OpConstantComposite %v4float %float_1 %float_1 %float_1 %float_1\n" +
- "%3 = OpCompositeExtract %v4float %2\n" +
- "OpReturn\n" +
- "OpFunctionEnd",
- 3, 2)
+ 3, 0)
));
INSTANTIATE_TEST_SUITE_P(CompositeConstructFoldingTest, GeneralInstructionFoldingTest,
diff --git a/test/opt/reduce_load_size_test.cpp b/test/opt/reduce_load_size_test.cpp
index 7b850e3..50dc501 100644
--- a/test/opt/reduce_load_size_test.cpp
+++ b/test/opt/reduce_load_size_test.cpp
@@ -321,34 +321,6 @@
SinglePassRunAndCheck<ReduceLoadSize>(test, test, true, false);
}
-TEST_F(ReduceLoadSizeTest, extract_with_no_index) {
- const std::string test =
- R"(
- OpCapability ImageGatherExtended
- OpExtension ""
- OpMemoryModel Logical GLSL450
- OpEntryPoint Fragment %4 "P�Ma'" %12 %17
- OpExecutionMode %4 OriginUpperLeft
- %void = OpTypeVoid
- %3 = OpTypeFunction %void
- %float = OpTypeFloat 32
- %_struct_7 = OpTypeStruct %float %float
-%_ptr_Input__struct_7 = OpTypePointer Input %_struct_7
-%_ptr_Output__struct_7 = OpTypePointer Output %_struct_7
- %12 = OpVariable %_ptr_Input__struct_7 Input
- %17 = OpVariable %_ptr_Output__struct_7 Output
- %4 = OpFunction %void DontInline|Pure|Const %3
- %245 = OpLabel
- %13 = OpLoad %_struct_7 %12
- %33 = OpCompositeExtract %_struct_7 %13
- OpReturn
- OpFunctionEnd
- )";
-
- auto result = SinglePassRunAndDisassemble<ReduceLoadSize>(test, true, true);
- EXPECT_EQ(Pass::Status::SuccessWithoutChange, std::get<1>(result));
-}
-
} // namespace
} // namespace opt
} // namespace spvtools
diff --git a/test/opt/scalar_replacement_test.cpp b/test/opt/scalar_replacement_test.cpp
index 1228076..3cf46ca 100644
--- a/test/opt/scalar_replacement_test.cpp
+++ b/test/opt/scalar_replacement_test.cpp
@@ -1899,37 +1899,6 @@
SinglePassRunAndMatch<ScalarReplacementPass>(text, true);
}
-TEST_F(ScalarReplacementTest, ExtractWithNoIndex) {
- const std::string text = R"(
-; CHECK: [[var1:%\w+]] = OpVariable %_ptr_Function_float Function
-; CHECK: [[var2:%\w+]] = OpVariable %_ptr_Function_float Function
-; CHECK: [[ld1:%\w+]] = OpLoad %float [[var1]]
-; CHECK: [[ld2:%\w+]] = OpLoad %float [[var2]]
-; CHECK: [[constr:%\w+]] = OpCompositeConstruct {{%\w+}} [[ld2]] [[ld1]]
-; CHECK: OpCompositeExtract {{%\w+}} [[constr]]
-OpCapability Shader
-OpExtension ""
-OpMemoryModel Logical GLSL450
-OpEntryPoint Fragment %1 "main"
-OpExecutionMode %1 OriginUpperLeft
-%void = OpTypeVoid
-%3 = OpTypeFunction %void
-%float = OpTypeFloat 32
-%_struct_5 = OpTypeStruct %float %float
-%_ptr_Private__struct_5 = OpTypePointer Private %_struct_5
-%_ptr_Function__struct_5 = OpTypePointer Function %_struct_5
-%1 = OpFunction %void Inline %3
-%8 = OpLabel
-%9 = OpVariable %_ptr_Function__struct_5 Function
-%10 = OpLoad %_struct_5 %9
-%11 = OpCompositeExtract %_struct_5 %10
-OpReturn
-OpFunctionEnd
-)";
-
- SinglePassRunAndMatch<ScalarReplacementPass>(text, true);
-}
-
} // namespace
} // namespace opt
} // namespace spvtools
diff --git a/test/opt/vector_dce_test.cpp b/test/opt/vector_dce_test.cpp
index a978a07..594995c 100644
--- a/test/opt/vector_dce_test.cpp
+++ b/test/opt/vector_dce_test.cpp
@@ -1158,74 +1158,6 @@
SinglePassRunAndCheck<VectorDCE>(text, text, true, true);
}
-TEST_F(VectorDCETest, InsertWithNoIndices) {
- const std::string text = R"(
-; CHECK: OpEntryPoint Fragment {{%\w+}} "PSMain" [[in1:%\w+]] [[in2:%\w+]] [[out:%\w+]]
-; CHECK: OpFunction
-; CHECK: [[ld:%\w+]] = OpLoad %v4float [[in2]]
-; CHECK: OpStore [[out]] [[ld]]
- OpCapability Shader
- OpMemoryModel Logical GLSL450
- OpEntryPoint Fragment %1 "PSMain" %2 %14 %3
- OpExecutionMode %1 OriginUpperLeft
- %void = OpTypeVoid
- %5 = OpTypeFunction %void
- %float = OpTypeFloat 32
- %v4float = OpTypeVector %float 4
-%_ptr_Input_v4float = OpTypePointer Input %v4float
-%_ptr_Output_v4float = OpTypePointer Output %v4float
- %2 = OpVariable %_ptr_Input_v4float Input
- %14 = OpVariable %_ptr_Input_v4float Input
- %3 = OpVariable %_ptr_Output_v4float Output
- %1 = OpFunction %void None %5
- %10 = OpLabel
- %13 = OpLoad %v4float %14
- %11 = OpLoad %v4float %2
- %12 = OpCompositeInsert %v4float %13 %11
- OpStore %3 %12
- OpReturn
- OpFunctionEnd
-)";
-
- SinglePassRunAndMatch<VectorDCE>(text, true);
-}
-
-TEST_F(VectorDCETest, ExtractWithNoIndices) {
- const std::string text = R"(
-; CHECK: OpLoad %float
-; CHECK: [[ld:%\w+]] = OpLoad %v4float
-; CHECK: [[ex1:%\w+]] = OpCompositeExtract %v4float [[ld]]
-; CHECK: [[ex2:%\w+]] = OpCompositeExtract %float [[ex1]] 1
-; CHECK: OpStore {{%\w+}} [[ex2]]
- OpCapability Shader
- OpMemoryModel Logical GLSL450
- OpEntryPoint Fragment %1 "PSMain" %2 %14 %3
- OpExecutionMode %1 OriginUpperLeft
- %void = OpTypeVoid
- %5 = OpTypeFunction %void
- %float = OpTypeFloat 32
- %v4float = OpTypeVector %float 4
-%_ptr_Input_float = OpTypePointer Input %float
-%_ptr_Input_v4float = OpTypePointer Input %v4float
-%_ptr_Output_float = OpTypePointer Output %float
- %2 = OpVariable %_ptr_Input_v4float Input
- %14 = OpVariable %_ptr_Input_float Input
- %3 = OpVariable %_ptr_Output_float Output
- %1 = OpFunction %void None %5
- %10 = OpLabel
- %13 = OpLoad %float %14
- %11 = OpLoad %v4float %2
- %12 = OpCompositeInsert %v4float %13 %11 0
- %20 = OpCompositeExtract %v4float %12
- %21 = OpCompositeExtract %float %20 1
- OpStore %3 %21
- OpReturn
- OpFunctionEnd
-)";
-
- SinglePassRunAndMatch<VectorDCE>(text, true);
-}
-
} // namespace
} // namespace opt
} // namespace spvtools
diff --git a/test/val/val_composites_test.cpp b/test/val/val_composites_test.cpp
index 74b6f20..e970562 100644
--- a/test/val/val_composites_test.cpp
+++ b/test/val/val_composites_test.cpp
@@ -648,7 +648,6 @@
%val16 = OpCompositeExtract %f32 %struct 4 1000 1
%val17 = OpCompositeExtract %f32 %struct 5 0
%val18 = OpCompositeExtract %u32 %struct 5 1
-%val19 = OpCompositeExtract %big_struct %struct
)";
CompileSuccessfully(GenerateShaderCode(body));
@@ -767,6 +766,18 @@
"indexes still remain to be traversed."));
}
+TEST_F(ValidateComposites, CompositeExtractNoIndices) {
+ const std::string body = R"(
+%struct = OpLoad %big_struct %var_big_struct
+%val1 = OpCompositeExtract %big_struct %struct
+)";
+
+ CompileSuccessfully(GenerateShaderCode(body));
+ ASSERT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+ EXPECT_THAT(getDiagnosticString(),
+ HasSubstr("Expected at least one index to OpCompositeExtract"));
+}
+
TEST_F(ValidateComposites, CompositeExtractWrongType1) {
const std::string body = R"(
%struct = OpLoad %big_struct %var_big_struct
@@ -861,7 +872,6 @@
%val16 = OpCompositeInsert %big_struct %f32_3 %struct 4 1000 1
%val17 = OpCompositeInsert %big_struct %f32_3 %struct 5 0
%val18 = OpCompositeInsert %big_struct %u32_3 %struct 5 1
-%val19 = OpCompositeInsert %big_struct %struct %struct
)";
CompileSuccessfully(GenerateShaderCode(body));
@@ -1157,9 +1167,8 @@
HasSubstr("The Result Type must be the same as Composite type"));
}
-// Valid: No Indexes were passed to OpCompositeExtract, and the Result Type is
-// the same as the Base Composite type.
-TEST_F(ValidateComposites, CompositeExtractNoIndexesGood) {
+// Invalid: No Indexes were passed to OpCompositeExtract.
+TEST_F(ValidateComposites, CompositeExtractNoIndices2) {
std::ostringstream spirv;
spirv << GetHeaderForTestsFromValId() << std::endl;
spirv << "%matrix = OpLoad %mat4x3 %my_matrix" << std::endl;
@@ -1167,29 +1176,32 @@
spirv << R"(OpReturn
OpFunctionEnd)";
CompileSuccessfully(spirv.str());
- EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Expected at least one index to OpCompositeExtract, zero found"));
}
-// Invalid: No Indexes were passed to OpCompositeExtract, but the Result Type is
-// different from the Base Composite type.
-TEST_F(ValidateComposites, CompositeExtractNoIndexesBad) {
+// Invalid: No Indexes were passed to OpCompositeExtract.
+TEST_F(ValidateComposites, CompositeExtractNoIndicesWrongResultType) {
std::ostringstream spirv;
spirv << GetHeaderForTestsFromValId() << std::endl;
spirv << "%matrix = OpLoad %mat4x3 %my_matrix" << std::endl;
- spirv << "%float_entry = OpCompositeExtract %float %matrix" << std::endl;
+ spirv << "%float_entry = OpCompositeExtract %float %matrix" << std::endl;
spirv << R"(OpReturn
OpFunctionEnd)";
CompileSuccessfully(spirv.str());
EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("Result type (OpTypeFloat) does not match the type "
- "that results from indexing into the composite "
- "(OpTypeMatrix)."));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Expected at least one index to OpCompositeExtract, zero found"));
}
-// Valid: No Indexes were passed to OpCompositeInsert, and the type of the
+// Invalid: No Indices were passed to OpCompositeInsert, and the type of the
// Object<id> argument matches the Composite type.
-TEST_F(ValidateComposites, CompositeInsertMissingIndexesGood) {
+TEST_F(ValidateComposites, CompositeInsertMissingIndices) {
std::ostringstream spirv;
spirv << GetHeaderForTestsFromValId() << std::endl;
spirv << "%matrix = OpLoad %mat4x3 %my_matrix" << std::endl;
@@ -1199,12 +1211,16 @@
OpReturn
OpFunctionEnd)";
CompileSuccessfully(spirv.str());
- EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());
+ EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Expected at least one index to OpCompositeInsert, zero found"));
}
-// Invalid: No Indexes were passed to OpCompositeInsert, but the type of the
+// Invalid: No Indices were passed to OpCompositeInsert, but the type of the
// Object<id> argument does not match the Composite type.
-TEST_F(ValidateComposites, CompositeInsertMissingIndexesBad) {
+TEST_F(ValidateComposites, CompositeInsertMissingIndices2) {
std::ostringstream spirv;
spirv << GetHeaderForTestsFromValId() << std::endl;
spirv << "%matrix = OpLoad %mat4x3 %my_matrix" << std::endl;
@@ -1214,10 +1230,10 @@
OpFunctionEnd)";
CompileSuccessfully(spirv.str());
EXPECT_EQ(SPV_ERROR_INVALID_DATA, ValidateInstructions());
- EXPECT_THAT(getDiagnosticString(),
- HasSubstr("The Object type (OpTypeInt) does not match the type "
- "that results from indexing into the Composite "
- "(OpTypeMatrix)."));
+ EXPECT_THAT(
+ getDiagnosticString(),
+ HasSubstr(
+ "Expected at least one index to OpCompositeInsert, zero found"));
}
// Valid: Tests that we can index into Struct, Array, Matrix, and Vector!
