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

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.

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

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;

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
   )";

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

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"

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
 )";

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"
       },
     }));

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"
       },
     }));

test/val/val_adjacency_test.cpp

@@ -144,7 +144,7 @@ OpBranch %end_label
 %false_label = OpLabel
 OpBranch %end_label
 %end_label = OpLabel
-%line = OpLine %string 0 0
+OpLine %string 0 0
 %result = OpPhi %bool %true %true_label %false %false_label
 )";
 
@@ -178,7 +178,7 @@ OpBranch %end_label
 %false_label = OpLabel
 OpBranch %end_label
 %end_label = OpLabel
-%line = OpLine %string 0 0
+OpLine %string 0 0
 %result = OpPhi %bool %true %true_label %false %false_label
 )";
 

test/val/val_atomics_test.cpp

@@ -708,7 +708,7 @@ OpAtomicStore %f32_var %device %relaxed %u32_1
 
 TEST_F(ValidateAtomics, AtomicExchangeShaderSuccess) {
   const std::string body = R"(
-%val1 = OpAtomicStore %u32_var %device %relaxed %u32_1
+OpAtomicStore %u32_var %device %relaxed %u32_1
 %val2 = OpAtomicExchange %u32 %u32_var %device %relaxed %u32_0
 )";
 
@@ -720,7 +720,7 @@ TEST_F(ValidateAtomics, AtomicExchangeKernelSuccess) {
   const std::string body = R"(
 OpAtomicStore %f32_var %device %relaxed %f32_1
 %val2 = OpAtomicExchange %f32 %f32_var %device %relaxed %f32_0
-%val3 = OpAtomicStore %u32_var %device %relaxed %u32_1
+OpAtomicStore %u32_var %device %relaxed %u32_1
 %val4 = OpAtomicExchange %u32 %u32_var %device %relaxed %u32_0
 )";
 
@@ -743,7 +743,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
 TEST_F(ValidateAtomics, AtomicExchangeWrongResultType) {
   const std::string body = R"(
-%val1 = OpStore %f32vec4_var %f32vec4_0000
+OpStore %f32vec4_var %f32vec4_0000
 %val2 = OpAtomicExchange %f32vec4 %f32vec4_var %device %relaxed %f32vec4_0000
 )";
 
@@ -768,7 +768,7 @@ TEST_F(ValidateAtomics, AtomicExchangeWrongPointerType) {
 
 TEST_F(ValidateAtomics, AtomicExchangeWrongPointerDataType) {
   const std::string body = R"(
-%val1 = OpStore %f32vec4_var %f32vec4_0000
+OpStore %f32vec4_var %f32vec4_0000
 %val2 = OpAtomicExchange %f32 %f32vec4_var %device %relaxed %f32vec4_0000
 )";
 
@@ -822,7 +822,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeShaderSuccess) {
   const std::string body = R"(
-%val1 = OpAtomicStore %u32_var %device %relaxed %u32_1
+OpAtomicStore %u32_var %device %relaxed %u32_1
 %val2 = OpAtomicCompareExchange %u32 %u32_var %device %relaxed %relaxed %u32_0 %u32_0
 )";
 
@@ -834,7 +834,7 @@ TEST_F(ValidateAtomics, AtomicCompareExchangeKernelSuccess) {
   const std::string body = R"(
 OpAtomicStore %f32_var %device %relaxed %f32_1
 %val2 = OpAtomicCompareExchange %f32 %f32_var %device %relaxed %relaxed %f32_0 %f32_1
-%val3 = OpAtomicStore %u32_var %device %relaxed %u32_1
+OpAtomicStore %u32_var %device %relaxed %u32_1
 %val4 = OpAtomicCompareExchange %u32 %u32_var %device %relaxed %relaxed %u32_0 %u32_0
 )";
 
@@ -857,7 +857,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeWrongResultType) {
   const std::string body = R"(
-%val1 = OpStore %f32vec4_var %f32vec4_0000
+OpStore %f32vec4_var %f32vec4_0000
 %val2 = OpAtomicCompareExchange %f32vec4 %f32vec4_var %device %relaxed %relaxed %f32vec4_0000 %f32vec4_0000
 )";
 
@@ -882,7 +882,7 @@ TEST_F(ValidateAtomics, AtomicCompareExchangeWrongPointerType) {
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeWrongPointerDataType) {
   const std::string body = R"(
-%val1 = OpStore %f32vec4_var %f32vec4_0000
+OpStore %f32vec4_var %f32vec4_0000
 %val2 = OpAtomicCompareExchange %f32 %f32vec4_var %device %relaxed %relaxed %f32_0 %f32_1
 )";
 
@@ -975,7 +975,7 @@ OpAtomicStore %f32_var %device %relaxed %f32_1
 
 TEST_F(ValidateAtomics, AtomicCompareExchangeWeakSuccess) {
   const std::string body = R"(
-%val3 = OpAtomicStore %u32_var %device %relaxed %u32_1
+OpAtomicStore %u32_var %device %relaxed %u32_1
 %val4 = OpAtomicCompareExchangeWeak %u32 %u32_var %device %relaxed %relaxed %u32_0 %u32_0
 )";
 

test/val/val_id_test.cpp

@@ -2317,8 +2317,8 @@ TEST_F(ValidateIdWithMessage, OpLoadGood) {
  %6 = OpFunction %1 None %4
  %7 = OpLabel
  %8 = OpLoad %2 %5
- %9 = OpReturn
-%10 = OpFunctionEnd
+      OpReturn
+      OpFunctionEnd
 )";
   CompileSuccessfully(spirv.c_str());
   EXPECT_EQ(SPV_SUCCESS, ValidateInstructions());