Assembler: Can't set an ID in instruction without result ID

Fix tests that violated this rule. Fixes KhronosGroup#2257
dneto0 · Sep 11, 2019 · 0ef3f16 · 0ef3f16
1 parent c0e9807
commit 0ef3f16
Show file tree

Hide file tree

Showing 9 changed files with 45 additions and 19 deletions.
diff --git a/source/text.cpp b/source/text.cpp
@@ -546,6 +546,11 @@ spv_result_t spvTextEncodeOpcode(const spvtools::AssemblyGrammar& grammar,
            << "Expected <result-id> at the beginning of an instruction, found '"
            << firstWord << "'.";
   }
+  if (!opcodeEntry->hasResult && !result_id.empty()) {
+    return context->diagnostic()
+           << "Cannot set ID " << result_id << " because " << opcodeName
+           << " does not produce a result ID.";
+  }
   pInst->opcode = opcodeEntry->opcode;
   context->setPosition(nextPosition);
   // Reserve the first word for the instruction.

diff --git a/test/assembly_format_test.cpp b/test/assembly_format_test.cpp
@@ -20,7 +20,7 @@ namespace {
 using spvtest::ScopedContext;
 using spvtest::TextToBinaryTest;
 
-TEST_F(TextToBinaryTest, NotPlacingResultIDAtTheBeginning) {
+TEST_F(TextToBinaryTest, InstOpcodeProducesResultIDButNoIDDefinedFails) {
   SetText("OpTypeMatrix %1 %2 1000");
   EXPECT_EQ(SPV_ERROR_INVALID_TEXT,
             spvTextToBinary(ScopedContext().context, text.str, text.length,
@@ -33,5 +33,18 @@ TEST_F(TextToBinaryTest, NotPlacingResultIDAtTheBeginning) {
   EXPECT_EQ(0u, diagnostic->position.line);
 }
 
+TEST_F(TextToBinaryTest,
+       InstDefinesResultIDButOpcodeDoesNotProduceAResultFails) {
+  SetText("\n\n%foo = OpName %1 \"bar\"");
+  EXPECT_EQ(SPV_ERROR_INVALID_TEXT,
+            spvTextToBinary(ScopedContext().context, text.str, text.length,
+                            &binary, &diagnostic));
+  ASSERT_NE(nullptr, diagnostic);
+  EXPECT_STREQ(
+      "Cannot set ID %foo because OpName does not produce a result ID.",
+      diagnostic->error);
+  EXPECT_EQ(2u, diagnostic->position.line);
+}
+
 }  // namespace
 }  // namespace svptools
diff --git a/test/c_interface_test.cpp b/test/c_interface_test.cpp
@@ -107,7 +107,7 @@ TEST(CInterface, DefaultConsumerNullDiagnosticForInvalidValidating) {
 }
 
 TEST(CInterface, SpecifyConsumerNullDiagnosticForAssembling) {
-  const char input_text[] = "%1 = OpName\n";
+  const char input_text[] = "     OpName\n";
 
   auto context = spvContextCreate(SPV_ENV_UNIVERSAL_1_1);
   int invocation = 0;
@@ -213,7 +213,7 @@ TEST(CInterface, SpecifyConsumerNullDiagnosticForValidating) {
 // When having both a consumer and an diagnostic object, the diagnostic object
 // should take priority.
 TEST(CInterface, SpecifyConsumerSpecifyDiagnosticForAssembling) {
-  const char input_text[] = "%1 = OpName";
+  const char input_text[] = "     OpName";
 
   auto context = spvContextCreate(SPV_ENV_UNIVERSAL_1_1);
   int invocation = 0;

diff --git a/test/opt/ccp_test.cpp b/test/opt/ccp_test.cpp
@@ -570,10 +570,10 @@ TEST_F(CCPTest, SkipSpecConstantInstrucitons) {
          %10 = OpSpecConstantFalse %bool
        %main = OpFunction %void None %4
          %11 = OpLabel
-         %12 = OpBranchConditional %10 %l1 %l2
-         %l1 = OpLabel
+               OpBranchConditional %10 %L1 %L2
+         %L1 = OpLabel
                OpReturn
-         %l2 = OpLabel
+         %L2 = OpLabel
                OpReturn
                OpFunctionEnd
   )";

diff --git a/test/opt/copy_prop_array_test.cpp b/test/opt/copy_prop_array_test.cpp
@@ -1157,7 +1157,7 @@ OpDecorate %MyCBuffer Binding 0
 OpStore %23 %35
 %36 = OpAccessChain %_ptr_Function_v4float %23 %24
 %37 = OpLoad %v4float %36
-%39 = OpStore %36 %v4const
+      OpStore %36 %v4const
 OpStore %out_var_SV_Target %37
 OpReturn
 OpFunctionEnd

diff --git a/test/opt/optimizer_test.cpp b/test/opt/optimizer_test.cpp
@@ -273,7 +273,8 @@ TEST_P(VulkanToWebGPUPassTest, Ran) {
   std::vector<uint32_t> optimized;
   class ValidatorOptions validator_options;
   ASSERT_TRUE(opt.Run(binary.data(), binary.size(), &optimized,
-                      validator_options, true));
+                      validator_options, true))
+      << GetParam().input << "\n";
   std::string disassembly;
   {
     SpirvTools tools(SPV_ENV_WEBGPU_0);
@@ -412,7 +413,7 @@ INSTANTIATE_TEST_SUITE_P(
          "%void_f = OpTypeFunction %void\n"
          "%func = OpFunction %void None %void_f\n"
          "%label = OpLabel\n"
-         "%val0 = OpAtomicStore %u32_var %cross_device "
+         "        OpAtomicStore %u32_var %cross_device "
          "%acquire_release_atomic_counter_workgroup %u32_1\n"
          "%val1 = OpAtomicIIncrement %u32 %u32_var %cross_device "
          "%acquire_release_atomic_counter_workgroup\n"

diff --git a/test/opt/upgrade_memory_model_test.cpp b/test/opt/upgrade_memory_model_test.cpp
@@ -92,7 +92,7 @@ OpMemoryModel Logical GLSL450
 %func = OpFunction %void None %func_ty
 %1 = OpLabel
 %ld = OpLoad %int %var
-%st = OpStore %var %ld
+OpStore %var %ld
 OpReturn
 OpFunctionEnd
 )";
@@ -116,7 +116,7 @@ OpMemoryModel Logical GLSL450
 %param = OpFunctionParameter %ptr_int_Workgroup
 %1 = OpLabel
 %ld = OpLoad %int %param
-%st = OpStore %param %ld
+OpStore %param %ld
 OpReturn
 OpFunctionEnd
 )";

diff --git a/test/text_to_binary.constant_test.cpp b/test/text_to_binary.constant_test.cpp
@@ -328,7 +328,6 @@ INSTANTIATE_TEST_SUITE_P(
 
 using OpConstantInvalidTypeTest =
     spvtest::TextToBinaryTestBase<::testing::TestWithParam<std::string>>;
-
 TEST_P(OpConstantInvalidTypeTest, InvalidTypes) {
   const std::string input = "%1 = " + GetParam() +
                             "\n"
@@ -360,8 +359,11 @@ INSTANTIATE_TEST_SUITE_P(
        "OpTypeReserveId",
        "OpTypeQueue",
        "OpTypePipe ReadOnly",
-       "OpTypeForwardPointer %a UniformConstant",
-        // At least one thing that isn't a type at all
+
+       // Skip OpTypeForwardPointer doesn't even produce a result ID.
+       // The assembler errors out if we try to check it in this scenario.
+
+       // Try at least one thing that isn't a type at all
        "OpNot %a %b"
       },
     }));
@@ -470,8 +472,10 @@ INSTANTIATE_TEST_SUITE_P(
        "OpTypeReserveId",
        "OpTypeQueue",
        "OpTypePipe ReadOnly",
-       "OpTypeForwardPointer %a UniformConstant",
-        // At least one thing that isn't a type at all
+
+       // Skip testing OpTypeForwardPointer because it doesn't even produce a result ID.
+
+       // Try at least one thing that isn't a type at all
        "OpNot %a %b"
       },
     }));

diff --git a/test/text_to_binary.control_flow_test.cpp b/test/text_to_binary.control_flow_test.cpp
@@ -342,7 +342,7 @@ TEST_P(OpSwitchInvalidTypeTestCase, InvalidTypes) {
       "%1 = " + GetParam() +
       "\n"
       "%3 = OpCopyObject %1 %2\n"  // We only care the type of the expression
-      "%4 = OpSwitch %3 %default 32 %c\n";
+      "     OpSwitch %3 %default 32 %c\n";
   EXPECT_THAT(CompileFailure(input),
               Eq("The selector operand for OpSwitch must be the result of an "
                  "instruction that generates an integer scalar"));
@@ -371,8 +371,11 @@ INSTANTIATE_TEST_SUITE_P(
        "OpTypeReserveId",
        "OpTypeQueue",
        "OpTypePipe ReadOnly",
-       "OpTypeForwardPointer %a UniformConstant",
-           // At least one thing that isn't a type at all
+
+       // Skip OpTypeForwardPointer becasuse it doesn't even produce a result
+       // ID.
+
+       // At least one thing that isn't a type at all
        "OpNot %a %b"
       },
     }));