From 6836e17f243eebfc4a2950faee49ed3a0015b20b Mon Sep 17 00:00:00 2001 From: David Neto Date: Tue, 29 Mar 2016 14:49:05 -0400 Subject: [PATCH] OpExecutionMode only takes a single ExecutionMode Previously, the grammar allowed many execution modes for a single OpExecutionMode instruction. Removes the variable- and optional- execution mode operand type enum values. Issue found by antiagainst@ --- include/spirv-tools/libspirv.h | 6 +----- source/binary.cpp | 3 --- source/opcode.cpp | 9 ++++++++- source/operand.cpp | 5 ----- test/OperandPattern.cpp | 6 +----- test/TextToBinary.ModeSetting.cpp | 6 ++++++ 6 files changed, 16 insertions(+), 19 deletions(-) diff --git a/include/spirv-tools/libspirv.h b/include/spirv-tools/libspirv.h index ac44ae8..9bd8a3b 100644 --- a/include/spirv-tools/libspirv.h +++ b/include/spirv-tools/libspirv.h @@ -205,8 +205,6 @@ typedef enum spv_operand_type_t { SPV_OPERAND_TYPE_OPTIONAL_TYPED_LITERAL_INTEGER, // An optional literal string. SPV_OPERAND_TYPE_OPTIONAL_LITERAL_STRING, - // An optional execution mode. - SPV_OPERAND_TYPE_OPTIONAL_EXECUTION_MODE, // An optional access qualifier SPV_OPERAND_TYPE_OPTIONAL_ACCESS_QUALIFIER, // An optional context-independent value, or CIV. CIVs are tokens that we can @@ -225,9 +223,7 @@ typedef enum spv_operand_type_t { // where the literal number must always be an integer of some sort. SPV_OPERAND_TYPE_VARIABLE_LITERAL_INTEGER_ID, // A sequence of zero or more pairs of (Id, Literal integer) - SPV_OPERAND_TYPE_VARIABLE_ID_LITERAL_INTEGER, - // A sequence of zero or more execution modes - LAST_VARIABLE(SPV_OPERAND_TYPE_VARIABLE_EXECUTION_MODE), + LAST_VARIABLE(SPV_OPERAND_TYPE_VARIABLE_ID_LITERAL_INTEGER), // This is a sentinel value, and does not represent an operand type. // It should come last. diff --git a/source/binary.cpp b/source/binary.cpp index 5c6aa8f..281038f 100644 --- a/source/binary.cpp +++ b/source/binary.cpp @@ -599,9 +599,6 @@ spv_result_t Parser::parseOperand(size_t inst_offset, } } break; - case SPV_OPERAND_TYPE_OPTIONAL_EXECUTION_MODE: - parsed_operand.type = SPV_OPERAND_TYPE_EXECUTION_MODE; - // Fall through case SPV_OPERAND_TYPE_CAPABILITY: case SPV_OPERAND_TYPE_SOURCE_LANGUAGE: case SPV_OPERAND_TYPE_EXECUTION_MODEL: diff --git a/source/opcode.cpp b/source/opcode.cpp index 1a37a6a..2421610 100644 --- a/source/opcode.cpp +++ b/source/opcode.cpp @@ -87,7 +87,14 @@ spv_operand_type_t convertOperandClassToType(SpvOp opcode, if (operandClass == OperandOptionalLiteral) { switch (opcode) { case SpvOpExecutionMode: - return SPV_OPERAND_TYPE_VARIABLE_EXECUTION_MODE; + // An OpExecutionMode only takes a single ExecutionMode operand and the + // operands for that execution mode. The OperandOptionalLiteral in the + // grammar from the spec is only used to generate the text "Optional + // literal(s)". But we've already recorded the + // SPV_OPERAND_TYPE_EXECUTION_MODE which will absorb those extra + // literals. Use a NONE operand type here to terminate the operands + // to the instruction. + return SPV_OPERAND_TYPE_NONE; default: break; } diff --git a/source/operand.cpp b/source/operand.cpp index f4a575e..3754654 100644 --- a/source/operand.cpp +++ b/source/operand.cpp @@ -1178,7 +1178,6 @@ const char* spvOperandTypeStr(spv_operand_type_t type) { case SPV_OPERAND_TYPE_MEMORY_MODEL: return "memory model"; case SPV_OPERAND_TYPE_EXECUTION_MODE: - case SPV_OPERAND_TYPE_OPTIONAL_EXECUTION_MODE: return "execution mode"; case SPV_OPERAND_TYPE_STORAGE_CLASS: return "storage class"; @@ -1312,10 +1311,6 @@ bool spvExpandOperandSequenceOnce(spv_operand_type_t type, {SPV_OPERAND_TYPE_OPTIONAL_ID, SPV_OPERAND_TYPE_LITERAL_INTEGER, type}); return true; - case SPV_OPERAND_TYPE_VARIABLE_EXECUTION_MODE: - pattern->insert(pattern->begin(), - {SPV_OPERAND_TYPE_OPTIONAL_EXECUTION_MODE, type}); - return true; default: break; } diff --git a/test/OperandPattern.cpp b/test/OperandPattern.cpp index a67c35f..f377469 100644 --- a/test/OperandPattern.cpp +++ b/test/OperandPattern.cpp @@ -189,9 +189,6 @@ TEST(AlternatePatternFollowingImmediate, SingleElement) { Eq(spv_operand_pattern_t{SPV_OPERAND_TYPE_OPTIONAL_CIV})); EXPECT_THAT(spvAlternatePatternFollowingImmediate({SPV_OPERAND_TYPE_ID}), Eq(spv_operand_pattern_t{SPV_OPERAND_TYPE_OPTIONAL_CIV})); - EXPECT_THAT(spvAlternatePatternFollowingImmediate( - {SPV_OPERAND_TYPE_VARIABLE_EXECUTION_MODE}), - Eq(spv_operand_pattern_t{SPV_OPERAND_TYPE_OPTIONAL_CIV})); } TEST(AlternatePatternFollowingImmediate, SingleResultId) { @@ -206,8 +203,7 @@ TEST(AlternatePatternFollowingImmediate, MultipleNonResultIds) { spvAlternatePatternFollowingImmediate( {SPV_OPERAND_TYPE_VARIABLE_ID_LITERAL_INTEGER, SPV_OPERAND_TYPE_CAPABILITY, SPV_OPERAND_TYPE_LOOP_CONTROL, - SPV_OPERAND_TYPE_OPTIONAL_LITERAL_INTEGER, SPV_OPERAND_TYPE_ID, - SPV_OPERAND_TYPE_VARIABLE_EXECUTION_MODE}), + SPV_OPERAND_TYPE_OPTIONAL_LITERAL_INTEGER, SPV_OPERAND_TYPE_ID}), Eq(spv_operand_pattern_t{SPV_OPERAND_TYPE_OPTIONAL_CIV})); } diff --git a/test/TextToBinary.ModeSetting.cpp b/test/TextToBinary.ModeSetting.cpp index b61644d..36a5bae 100644 --- a/test/TextToBinary.ModeSetting.cpp +++ b/test/TextToBinary.ModeSetting.cpp @@ -198,6 +198,12 @@ TEST_F(OpExecutionModeTest, WrongMode) { Eq("Invalid execution mode 'xxyyzz'.")); } +TEST_F(OpExecutionModeTest, TooManyModes) { + EXPECT_THAT(CompileFailure("OpExecutionMode %1 Xfb PointMode"), + Eq("Expected or at the beginning of an " + "instruction, found 'PointMode'.")); +} + // Test OpCapability using OpCapabilityTest = spvtest::TextToBinaryTestBase< -- 2.7.4