Fix handling of OpExtInstImport

The assembler tracks mapping of extended instruction import Id
to extended instruction type.

Adds a few new ways to fail.

CMakeLists.txt

@@ -215,6 +215,7 @@ if (NOT ${SPIRV_SKIP_EXECUTABLES})
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Debug.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.DeviceSideEnqueue.cpp
+      ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Extension.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Function.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Group.cpp
       ${CMAKE_CURRENT_SOURCE_DIR}/test/TextToBinary.Image.cpp

include/libspirv/libspirv.h

@@ -287,7 +287,7 @@ typedef enum spv_operand_type_t {
 } spv_operand_type_t;
 
 typedef enum spv_ext_inst_type_t {
-  SPV_EXT_INST_TYPE_NONE,
+  SPV_EXT_INST_TYPE_NONE = 0,
   SPV_EXT_INST_TYPE_GLSL_STD_450,
   SPV_EXT_INST_TYPE_OPENCL_STD,
 

source/text.cpp

@@ -233,6 +233,19 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar,
       const uint32_t id = context->spvNamedIdAssignOrGet(textValue);
       if (type == SPV_OPERAND_TYPE_TYPE_ID) pInst->resultTypeId = id;
       spvInstructionAddWord(pInst, id);
+
+      // Set the extended instruction type.
+      // The import set id is the 3rd operand of OpExtInst.
+      if (pInst->opcode == SpvOpExtInst && pInst->words.size() == 4) {
+        auto ext_inst_type = context->getExtInstTypeForId(pInst->words[3]);
+        if (ext_inst_type == SPV_EXT_INST_TYPE_NONE) {
+          return context->diagnostic()
+                 << "Invalid extended instruction import Id "
+                 << pInst->words[2];
+        }
+        pInst->extInstType = ext_inst_type;
+      }
+
     } break;
 
     case SPV_OPERAND_TYPE_EXTENSION_INSTRUCTION_NUMBER: {
@@ -328,7 +341,15 @@ spv_result_t spvTextEncodeOperand(const libspirv::AssemblyGrammar& grammar,
 
       // NOTE: Special case for extended instruction library import
       if (SpvOpExtInstImport == pInst->opcode) {
-        pInst->extInstType = spvExtInstImportTypeGet(literal.value.str);
+        const spv_ext_inst_type_t ext_inst_type =
+            spvExtInstImportTypeGet(literal.value.str);
+        if (SPV_EXT_INST_TYPE_NONE == ext_inst_type) {
+          return context->diagnostic()
+                 << "Invalid extended instruction import '" << literal.value.str
+                 << "'";
+        }
+        if (auto error = context->recordIdAsExtInstImport(pInst->words[1], ext_inst_type))
+          return error;
       }
 
       if (context->binaryEncodeString(literal.value.str, pInst))
@@ -651,16 +672,13 @@ spv_result_t spvTextToBinaryInternal(const libspirv::AssemblyGrammar& grammar,
   // Skip past whitespace and comments.
   context.advance();
 
-  spv_ext_inst_type_t extInstType = SPV_EXT_INST_TYPE_NONE;
   while (context.hasText()) {
     instructions.push_back({});
     spv_instruction_t& inst = instructions.back();
-    inst.extInstType = extInstType;
 
     if (spvTextEncodeOpcode(grammar, &context, &inst)) {
       return SPV_ERROR_INVALID_TEXT;
     }
-    extInstType = inst.extInstType;
 
     if (context.advance()) break;
   }

source/text_handler.cpp

@@ -364,6 +364,24 @@ spv_result_t AssemblyContext::recordTypeIdForValue(uint32_t value,
   return SPV_SUCCESS;
 }
 
+spv_result_t AssemblyContext::recordIdAsExtInstImport(
+    uint32_t id, spv_ext_inst_type_t type) {
+  bool successfully_inserted = false;
+  std::tie(std::ignore, successfully_inserted) =
+      import_id_to_ext_inst_type_.insert(std::make_pair(id, type));
+  if (!successfully_inserted)
+    return diagnostic() << "Import Id is being defined a second time";
+  return SPV_SUCCESS;
+}
+
+spv_ext_inst_type_t AssemblyContext::getExtInstTypeForId(uint32_t id) const {
+  auto type = import_id_to_ext_inst_type_.find(id);
+  if (type == import_id_to_ext_inst_type_.end()) {
+    return SPV_EXT_INST_TYPE_NONE;
+  }
+  return std::get<1>(*type);
+}
+
 spv_result_t AssemblyContext::binaryEncodeFloatingPointLiteral(
     const char* val, spv_result_t error_code, const IdType& type,
     spv_instruction_t* pInst) {

source/text_handler.h

@@ -234,6 +234,15 @@ class AssemblyContext {
   // Tracks the relationship between the value and its type.
   spv_result_t recordTypeIdForValue(uint32_t value, uint32_t type);
 
+  // Records the given Id as being the import of the given extended instruction
+  // type.
+  spv_result_t recordIdAsExtInstImport(uint32_t id, spv_ext_inst_type_t type);
+
+  // Returns the extended instruction type corresponding to the import with
+  // the given Id, if it exists.  Returns SPV_EXT_INST_TYPE_NONE if the
+  // id is not the id for an extended instruction type.
+  spv_ext_inst_type_t getExtInstTypeForId(uint32_t id) const;
+
   // Parses a numeric value of a given type from the given text.  The number
   // should take up the entire string, and should be within bounds for the
   // target type.  On success, returns SPV_SUCCESS and populates the object
@@ -320,6 +329,8 @@ class AssemblyContext {
   spv_named_id_table named_ids_;
   spv_id_to_type_map types_;
   spv_id_to_type_id value_types_;
+  // Maps an extended instruction import Id to the extended instruction type.
+  std::unordered_map<uint32_t, spv_ext_inst_type_t> import_id_to_ext_inst_type_;
   spv_position_t current_position_;
   spv_diagnostic* pDiagnostic_;
   spv_text text_;

test/ExtInst.OpenCL.std.cpp

@@ -383,25 +383,4 @@ INSTANTIATE_TEST_CASE_P(
 #undef CASE2Lit
 #undef CASE3Round
 
-// TODO(dneto): Fix this functionality.
-TEST_F(TextToBinaryTest, DISABLED_ExtInstFromTwoDifferentImports) {
-  const std::string input =
-      R"(%1 = OpExtInstImport "OpenCL.std"
-%2 = OpExtInstImport "GLSL.std.450"
-%4 = OpExtInst %3 %1 native_sqrt %5
-%7 = OpExtInst %6 %2 MatrixInverse %8
-)";
-  EXPECT_THAT(
-      CompiledInstructions(input),
-      Eq(Concatenate({
-          MakeInstruction(SpvOpExtInstImport, {1}, MakeVector("OpenCL.std")),
-          MakeInstruction(SpvOpExtInstImport, {2}, MakeVector("GLSL.std.450")),
-          MakeInstruction(
-              SpvOpExtInst,
-              {3, 4, 1, uint32_t(OpenCLLIB::Entrypoints::Native_sqrt), 5}),
-          MakeInstruction(SpvOpExtInst,
-                          {6, 7, 2, uint32_t(GLSLstd450MatrixInverse), 8}),
-      })));
-}
-
 }  // anonymous namespace

test/TextToBinary.Extension.cpp

@@ -0,0 +1,93 @@
+// Copyright (c) 2015 The Khronos Group Inc.
+//
+// Permission is hereby granted, free of charge, to any person obtaining a
+// copy of this software and/or associated documentation files (the
+// "Materials"), to deal in the Materials without restriction, including
+// without limitation the rights to use, copy, modify, merge, publish,
+// distribute, sublicense, and/or sell copies of the Materials, and to
+// permit persons to whom the Materials are furnished to do so, subject to
+// the following conditions:
+//
+// The above copyright notice and this permission notice shall be included
+// in all copies or substantial portions of the Materials.
+//
+// MODIFICATIONS TO THIS FILE MAY MEAN IT NO LONGER ACCURATELY REFLECTS
+// KHRONOS STANDARDS. THE UNMODIFIED, NORMATIVE VERSIONS OF KHRONOS
+// SPECIFICATIONS AND HEADER INFORMATION ARE LOCATED AT
+//    https://www.khronos.org/registry/
+//
+// THE MATERIALS ARE PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND,
+// EXPRESS OR IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF
+// MERCHANTABILITY, FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.
+// IN NO EVENT SHALL THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY
+// CLAIM, DAMAGES OR OTHER LIABILITY, WHETHER IN AN ACTION OF CONTRACT,
+// TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
+// MATERIALS OR THE USE OR OTHER DEALINGS IN THE MATERIALS.
+
+// Assembler tests for instructions in the "Extension Instruction" section
+// of the SPIR-V spec.
+
+#include "UnitSPIRV.h"
+
+#include "TestFixture.h"
+#include "gmock/gmock.h"
+
+namespace {
+
+using spvtest::Concatenate;
+using spvtest::MakeInstruction;
+using spvtest::MakeVector;
+using spvtest::TextToBinaryTest;
+using ::testing::Eq;
+
+TEST_F(TextToBinaryTest, InvalidExtInstImportName) {
+  EXPECT_THAT(CompileFailure("%1 = OpExtInstImport \"Haskell.std\""),
+              Eq("Invalid extended instruction import 'Haskell.std'"));
+}
+
+TEST_F(TextToBinaryTest, InvalidImportId) {
+  EXPECT_THAT(CompileFailure("%1 = OpTypeVoid\n"
+                             "%2 = OpExtInst %1 %1"),
+              Eq("Invalid extended instruction import Id 2"));
+}
+
+TEST_F(TextToBinaryTest, InvalidImportInstruction) {
+  const std::string input = R"(%1 = OpTypeVoid
+                               %2 = OpExtInstImport "OpenCL.std"
+                               %3 = OpExtInst %1 %2 not_in_the_opencl)";
+  EXPECT_THAT(CompileFailure(input),
+              Eq("Invalid extended instruction name 'not_in_the_opencl'."));
+}
+
+TEST_F(TextToBinaryTest, MultiImport) {
+  const std::string input = R"(%2 = OpExtInstImport "OpenCL.std"
+                               %2 = OpExtInstImport "OpenCL.std")";
+  EXPECT_THAT(CompileFailure(input),
+              Eq("Import Id is being defined a second time"));
+}
+
+TEST_F(TextToBinaryTest, ExtInstFromTwoDifferentImports) {
+  const std::string input = R"(%1 = OpExtInstImport "OpenCL.std"
+%2 = OpExtInstImport "GLSL.std.450"
+%4 = OpExtInst %3 %1 native_sqrt %5
+%7 = OpExtInst %6 %2 MatrixInverse %8
+)";
+
+  // Make sure it assembles correctly.
+  EXPECT_THAT(
+      CompiledInstructions(input),
+      Eq(Concatenate({
+          MakeInstruction(SpvOpExtInstImport, {1}, MakeVector("OpenCL.std")),
+          MakeInstruction(SpvOpExtInstImport, {2}, MakeVector("GLSL.std.450")),
+          MakeInstruction(
+              SpvOpExtInst,
+              {3, 4, 1, uint32_t(OpenCLLIB::Entrypoints::Native_sqrt), 5}),
+          MakeInstruction(SpvOpExtInst,
+                          {6, 7, 2, uint32_t(GLSLstd450MatrixInverse), 8}),
+      })));
+
+  // Make sure it disassembles correctly.
+  EXPECT_THAT(EncodeAndDecodeSuccessfully(input), Eq(input));
+}
+
+}  // anonymous namespace