From b14a727a30f69f47df96b2ade8859cbb0683f95b Mon Sep 17 00:00:00 2001 From: David Neto Date: Fri, 25 Sep 2015 13:56:09 -0400 Subject: [PATCH] Execution scope, memory semantics operands are IDs They shouldn't be parsed or printed as masks. --- include/libspirv/libspirv.h | 3 ++- readme.md | 4 +++- source/binary.cpp | 8 +++---- source/text.cpp | 7 +++--- test/TextToBinary.Barrier.cpp | 43 ++++------------------------------- 5 files changed, 18 insertions(+), 47 deletions(-) diff --git a/include/libspirv/libspirv.h b/include/libspirv/libspirv.h index 60e1f2cda..718a9bdc4 100644 --- a/include/libspirv/libspirv.h +++ b/include/libspirv/libspirv.h @@ -172,8 +172,9 @@ typedef enum spv_operand_type_t { SPV_OPERAND_TYPE_SELECTION_CONTROL, SPV_OPERAND_TYPE_LOOP_CONTROL, SPV_OPERAND_TYPE_FUNCTION_CONTROL, - SPV_OPERAND_TYPE_MEMORY_SEMANTICS, + // The ID for a memory semantics value. + SPV_OPERAND_TYPE_MEMORY_SEMANTICS, // The ID for an execution scope value. SPV_OPERAND_TYPE_EXECUTION_SCOPE, diff --git a/readme.md b/readme.md index f2d6ecb1d..9f3270858 100644 --- a/readme.md +++ b/readme.md @@ -28,7 +28,7 @@ The validator is incomplete. See the Future Work section for more information. ## CHANGES (for tools hackers) -2015-09-24 +2015-09-25 * Updated to Rev32 headers * Core instructions and enumerants from Rev 32 are supported by the assembler. @@ -44,6 +44,8 @@ The validator is incomplete. See the Future Work section for more information. map to that number. E.g. `%2` doesn't necessarily map to ID 2. * Disassembler emits mask expressions for mask combinations instead of erroring out. +* Fixed parsing and printing of Execution Scope and Memory Semantics + operands. They are supplied as IDs, not as literals. 2015-09-18 * MILESTONE: This version of the assembler supports all of SPIR-V Rev31, diff --git a/source/binary.cpp b/source/binary.cpp index 5c21e7fed..0b75e6cef 100644 --- a/source/binary.cpp +++ b/source/binary.cpp @@ -222,10 +222,12 @@ spv_result_t spvBinaryDecodeOperand( print && spvIsInBitfield(SPV_BINARY_TO_TEXT_OPTION_COLOR, options); switch (type) { + case SPV_OPERAND_TYPE_EXECUTION_SCOPE: case SPV_OPERAND_TYPE_ID: - case SPV_OPERAND_TYPE_RESULT_ID: + case SPV_OPERAND_TYPE_ID_IN_OPTIONAL_TUPLE: case SPV_OPERAND_TYPE_OPTIONAL_ID: - case SPV_OPERAND_TYPE_ID_IN_OPTIONAL_TUPLE: { + case SPV_OPERAND_TYPE_MEMORY_SEMANTICS: + case SPV_OPERAND_TYPE_RESULT_ID: { if (color) { if (type == SPV_OPERAND_TYPE_RESULT_ID) { stream.get() << clr::blue(); @@ -311,8 +313,6 @@ spv_result_t spvBinaryDecodeOperand( case SPV_OPERAND_TYPE_FUNCTION_PARAMETER_ATTRIBUTE: case SPV_OPERAND_TYPE_DECORATION: case SPV_OPERAND_TYPE_BUILT_IN: - case SPV_OPERAND_TYPE_MEMORY_SEMANTICS: - case SPV_OPERAND_TYPE_EXECUTION_SCOPE: case SPV_OPERAND_TYPE_GROUP_OPERATION: case SPV_OPERAND_TYPE_KERNEL_ENQ_FLAGS: case SPV_OPERAND_TYPE_KERNEL_PROFILING_INFO: { diff --git a/source/text.cpp b/source/text.cpp index 23c0b1141..e83049ed4 100644 --- a/source/text.cpp +++ b/source/text.cpp @@ -351,6 +351,7 @@ bool isIdType(spv_operand_type_t type) { case SPV_OPERAND_TYPE_EXECUTION_SCOPE: case SPV_OPERAND_TYPE_ID: case SPV_OPERAND_TYPE_ID_IN_OPTIONAL_TUPLE: + case SPV_OPERAND_TYPE_MEMORY_SEMANTICS: case SPV_OPERAND_TYPE_OPTIONAL_ID: case SPV_OPERAND_TYPE_RESULT_ID: return true; @@ -437,11 +438,12 @@ spv_result_t spvTextEncodeOperand( } switch (type) { + case SPV_OPERAND_TYPE_EXECUTION_SCOPE: case SPV_OPERAND_TYPE_ID: case SPV_OPERAND_TYPE_ID_IN_OPTIONAL_TUPLE: case SPV_OPERAND_TYPE_OPTIONAL_ID: - case SPV_OPERAND_TYPE_RESULT_ID: - case SPV_OPERAND_TYPE_EXECUTION_SCOPE: { + case SPV_OPERAND_TYPE_MEMORY_SEMANTICS: + case SPV_OPERAND_TYPE_RESULT_ID: { if ('%' == textValue[0]) { textValue++; } else { @@ -559,7 +561,6 @@ spv_result_t spvTextEncodeOperand( case SPV_OPERAND_TYPE_FP_FAST_MATH_MODE: case SPV_OPERAND_TYPE_FUNCTION_CONTROL: case SPV_OPERAND_TYPE_LOOP_CONTROL: - case SPV_OPERAND_TYPE_MEMORY_SEMANTICS: case SPV_OPERAND_TYPE_OPTIONAL_IMAGE: case SPV_OPERAND_TYPE_OPTIONAL_MEMORY_ACCESS: case SPV_OPERAND_TYPE_SELECTION_CONTROL: { diff --git a/test/TextToBinary.Barrier.cpp b/test/TextToBinary.Barrier.cpp index 256a769ac..4449b77fa 100644 --- a/test/TextToBinary.Barrier.cpp +++ b/test/TextToBinary.Barrier.cpp @@ -40,46 +40,13 @@ using ::testing::Eq; // Test OpMemoryBarrier -using MemorySemanticsTest = spvtest::TextToBinaryTestBase< - ::testing::TestWithParam>>; +using OpMemoryBarrier = spvtest::TextToBinaryTest; -TEST_P(MemorySemanticsTest, AnySingleMemorySemanticsMask) { - std::string input = "OpMemoryBarrier %1 " + GetParam().name(); - EXPECT_THAT( - CompiledInstructions(input), - Eq(MakeInstruction(spv::OpMemoryBarrier, {1, GetParam().value()}))); -} - -#define CASE(NAME) \ - { \ - spv::MemorySemantics##NAME##Mask, #NAME, {} \ - } -INSTANTIATE_TEST_CASE_P( - TextToBinaryMemorySemanticsTest, MemorySemanticsTest, - ::testing::ValuesIn(std::vector>{ - {spv::MemorySemanticsMaskNone, "None", {}}, - // Relaxed is a synonym for None. - {spv::MemorySemanticsMaskNone, "Relaxed", {}}, - CASE(Acquire), - CASE(Release), - CASE(SequentiallyConsistent), - CASE(UniformMemory), - CASE(SubgroupMemory), - CASE(WorkgroupLocalMemory), - CASE(WorkgroupGlobalMemory), - CASE(AtomicCounterMemory), - CASE(ImageMemory), - })); -#undef CASE - -TEST_F(TextToBinaryTest, CombinedMemorySemanticsMask) { - // Sample a single combination. This ensures we've integrated - // the instruction parsing logic with spvTextParseMask. - const std::string input = "OpMemoryBarrier %1 Acquire|WorkgroupLocalMemory"; - const uint32_t expected_mask = spv::MemorySemanticsAcquireMask | - spv::MemorySemanticsWorkgroupLocalMemoryMask; +TEST_F(OpMemoryBarrier, Sample) { + std::string input = "OpMemoryBarrier %1 %2\n"; EXPECT_THAT(CompiledInstructions(input), - Eq(MakeInstruction(spv::OpMemoryBarrier, {1, expected_mask}))); + Eq(MakeInstruction(spv::OpMemoryBarrier, {1, 2}))); + EXPECT_THAT(EncodeAndDecodeSuccessfully(input), Eq(input)); } // TODO(dneto): OpControlBarrier