Pass manager recomputes Id bound automatically.

Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/371
in the sense that the id bound is correct after all the passes
have been run.  But it might be inconsistent between passes.

diff --git a/source/opt/pass_manager.h b/source/opt/pass_manager.h
index 7ebd6f8..a4ca586 100644 (file)
--- a/source/opt/pass_manager.h
+++ b/source/opt/pass_manager.h
@@ -65,12 +65,12 @@ class PassManager {
 
   // Runs all passes on the given |module|.
   void Run(ir::Module* module) {
+    bool modified = false;
     for (const auto& pass : passes_) {
-      // TODO(antiagainst): Currently we ignore the return value of the pass,
-      // which indicates whether the module has been modified, since there is
-      // nothing shared between passes right now.
-      pass->Process(module);
+      modified |= pass->Process(module);
     }
+    // Set the Id bound in the header in case a pass forgot to do so.
+    if (modified) module->SetIdBound(module->ComputeIdBound());
   }
 
  private:

diff --git a/test/opt/CMakeLists.txt b/test/opt/CMakeLists.txt
index ea253b8..84c4505 100644 (file)
--- a/test/opt/CMakeLists.txt
+++ b/test/opt/CMakeLists.txt
@@ -35,7 +35,8 @@ add_spvtools_unittest(TARGET ir_loader
 )
 
 add_spvtools_unittest(TARGET pass_manager
-  SRCS test_pass_manager.cpp
+  SRCS module_utils.h
+       test_pass_manager.cpp
   LIBS SPIRV-Tools-opt ${SPIRV_TOOLS}
 )
 
@@ -85,6 +86,7 @@ add_spvtools_unittest(TARGET iterator
 )
 
 add_spvtools_unittest(TARGET module
-  SRCS test_module.cpp
+  SRCS module_utils.h
+       test_module.cpp
   LIBS SPIRV-Tools-opt ${SPIRV_TOOLS}
 )

diff --git a/test/opt/module_utils.h b/test/opt/module_utils.h
new file mode 100644 (file)
index 0000000..e05e961
--- /dev/null
+++ b/test/opt/module_utils.h
@@ -0,0 +1,46 @@
+// Copyright (c) 2016 Google 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.
+
+#ifndef LIBSPIRV_TEST_OPT_MODULE_UTILS_H_
+#define LIBSPIRV_TEST_OPT_MODULE_UTILS_H_
+
+#include <vector>
+#include "opt/module.h"
+
+namespace spvtest {
+
+inline uint32_t GetIdBound(const spvtools::ir::Module& m) {
+  std::vector<uint32_t> binary;
+  m.ToBinary(&binary, false);
+  // The 5-word header must always exist.
+  EXPECT_LE(5u, binary.size());
+  // The bound is the fourth word.
+  return binary[3];
+}
+
+}  // namespace spvtest
+
+#endif // LIBSPIRV_TEST_OPT_MODULE_UTILS_H_

diff --git a/test/opt/test_module.cpp b/test/opt/test_module.cpp
index 7260ccb..95b6e9d 100644 (file)
--- a/test/opt/test_module.cpp
+++ b/test/opt/test_module.cpp
@@ -32,20 +32,14 @@
 #include "opt/libspirv.hpp"
 #include "opt/module.h"
 
+#include "module_utils.h"
+
 namespace {
 
 using spvtools::ir::Module;
+using spvtest::GetIdBound;
 using ::testing::Eq;
 
-uint32_t GetIdBound(const Module& m) {
-  std::vector<uint32_t> binary;
-  m.ToBinary(&binary, false);
-  // The 5-word header must always exist.
-  EXPECT_GE(5u, binary.size());
-  // The bound is the fourth word.
-  return binary[3];
-}
-
 TEST(ModuleTest, SetIdBound) {
   Module m;
   // It's initialized to 0.

diff --git a/test/opt/test_pass_manager.cpp b/test/opt/test_pass_manager.cpp
index 0e8c45a..8b93fc5 100644 (file)
--- a/test/opt/test_pass_manager.cpp
+++ b/test/opt/test_pass_manager.cpp
@@ -24,12 +24,17 @@
 // TORT OR OTHERWISE, ARISING FROM, OUT OF OR IN CONNECTION WITH THE
 // MATERIALS OR THE USE OR OTHER DEALINGS IN THE MATERIALS.
 
-#include "pass_fixture.h"
+#include "gmock/gmock.h"
+
+#include "module_utils.h"
 #include "opt/make_unique.h"
+#include "pass_fixture.h"
 
 namespace {
 
 using namespace spvtools;
+using spvtest::GetIdBound;
+using ::testing::Eq;
 
 TEST(PassManager, Interface) {
   opt::PassManager manager;
@@ -91,4 +96,50 @@ TEST_F(PassManagerTest, Run) {
   RunAndCheck(text.c_str(), (text + "OpSource ESSL 310\nOpNop\n").c_str());
 }
 
+// A pass that appends an OpTypeVoid instruction that uses a given id.
+class AppendTypeVoidInstPass : public opt::Pass {
+ public:
+  AppendTypeVoidInstPass(uint32_t result_id) : result_id_(result_id) {}
+  const char* name() const override { return "AppendTypeVoidInstPass"; }
+  bool Process(ir::Module* module) override {
+    auto inst = MakeUnique<ir::Instruction>(SpvOpTypeVoid, 0, result_id_,
+                                            std::vector<ir::Operand>{});
+    module->AddType(std::move(inst));
+    return true;
+  }
+
+ private:
+  uint32_t result_id_;
+};
+
+TEST(PassManager, RecomputeIdBoundAutomatically) {
+  ir::Module module;
+  EXPECT_THAT(GetIdBound(module), Eq(0u));
+
+  opt::PassManager manager;
+  manager.Run(&module);
+  manager.AddPass<AppendOpNopPass>();
+  // With no ID changes, the ID bound does not change.
+  EXPECT_THAT(GetIdBound(module), Eq(0u));
+
+  // Now we force an Id of 100 to be used.
+  manager.AddPass(MakeUnique<AppendTypeVoidInstPass>(100));
+  EXPECT_THAT(GetIdBound(module), Eq(0u));
+  manager.Run(&module);
+  // The Id has been updated automatically, even though the pass
+  // did not update it.
+  EXPECT_THAT(GetIdBound(module), Eq(101u));
+
+  // Try one more time!
+  manager.AddPass(MakeUnique<AppendTypeVoidInstPass>(200));
+  manager.Run(&module);
+  EXPECT_THAT(GetIdBound(module), Eq(201u));
+
+  // Add another pass, but which uses a lower Id.
+  manager.AddPass(MakeUnique<AppendTypeVoidInstPass>(10));
+  manager.Run(&module);
+  // The Id stays high.
+  EXPECT_THAT(GetIdBound(module), Eq(201u));
+}
+
 }  // anonymous namespace