From: David Neto Date: Thu, 25 Aug 2016 22:36:03 +0000 (-0400) Subject: Pass manager recomputes Id bound automatically. X-Git-Tag: upstream/2018.6~1091 X-Git-Url: http://review.tizen.org/git/?a=commitdiff_plain;h=1d59aa07776ee6c95804e282cee1e79709b7313b;p=platform%2Fupstream%2FSPIRV-Tools.git 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 --- 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 --- 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 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 +#include "opt/module.h" + +namespace spvtest { + +inline uint32_t GetIdBound(const spvtools::ir::Module& m) { + std::vector 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 --- 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 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 --- 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(SpvOpTypeVoid, 0, result_id_, + std::vector{}); + 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(); + // 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(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(200)); + manager.Run(&module); + EXPECT_THAT(GetIdBound(module), Eq(201u)); + + // Add another pass, but which uses a lower Id. + manager.AddPass(MakeUnique(10)); + manager.Run(&module); + // The Id stays high. + EXPECT_THAT(GetIdBound(module), Eq(201u)); +} + } // anonymous namespace