From afc60bbebfeb8cc4d04cc904e562f4a53f58613f Mon Sep 17 00:00:00 2001 From: David Neto Date: Sun, 2 Apr 2017 02:17:41 -0400 Subject: [PATCH] Fix optimizer on when to write the binary The spvtools::Optimizer::Run method should also write the output binary if optimization succeeds without changes but the output binary vector does not have exactly the same contents as the input binary. We have to check both the base pointer of the storage and the size of the vector Added a test for this too. Fixes https://github.com/KhronosGroup/SPIRV-Tools/issues/611 --- CHANGES | 2 + source/opt/optimizer.cpp | 5 +- test/opt/CMakeLists.txt | 5 ++ test/opt/optimizer_test.cpp | 109 ++++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 120 insertions(+), 1 deletion(-) create mode 100644 test/opt/optimizer_test.cpp diff --git a/CHANGES b/CHANGES index ffe7b00..8347371 100644 --- a/CHANGES +++ b/CHANGES @@ -16,6 +16,8 @@ v2016.7-dev 2017-01-06 header. #548: Validator: Error when the reserved OpImageSparseSampleProj* opcodes are used. + #611: spvtools::Optimizer was failing to save the module to the output + binary vector when all passes succeded without changes. v2016.6 2016-12-13 - Published the C++ interface for assembling, disassembling, validation, and diff --git a/source/opt/optimizer.cpp b/source/opt/optimizer.cpp index f6dc1cd..b1d7197 100644 --- a/source/opt/optimizer.cpp +++ b/source/opt/optimizer.cpp @@ -75,7 +75,10 @@ bool Optimizer::Run(const uint32_t* original_binary, if (module == nullptr) return false; auto status = impl_->pass_manager.Run(module.get()); - if (status == opt::Pass::Status::SuccessWithChange) { + if (status == opt::Pass::Status::SuccessWithChange || + (status == opt::Pass::Status::SuccessWithoutChange && + (optimized_binary->data() != original_binary || + optimized_binary->size() != original_binary_size))) { optimized_binary->clear(); module->ToBinary(optimized_binary, /* skip_nop = */ true); } diff --git a/test/opt/CMakeLists.txt b/test/opt/CMakeLists.txt index 2f1f482..d807d47 100644 --- a/test/opt/CMakeLists.txt +++ b/test/opt/CMakeLists.txt @@ -28,6 +28,11 @@ add_spvtools_unittest(TARGET pass_manager LIBS SPIRV-Tools-opt ) +add_spvtools_unittest(TARGET optimizer + SRCS optimizer_test.cpp + LIBS SPIRV-Tools-opt +) + add_spvtools_unittest(TARGET pass_strip_debug_info SRCS strip_debug_info_test.cpp pass_utils.cpp LIBS SPIRV-Tools-opt diff --git a/test/opt/optimizer_test.cpp b/test/opt/optimizer_test.cpp new file mode 100644 index 0000000..ef0e992 --- /dev/null +++ b/test/opt/optimizer_test.cpp @@ -0,0 +1,109 @@ +// Copyright (c) 2017 Google Inc. +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +#include + +#include "spirv-tools/libspirv.hpp" +#include "spirv-tools/optimizer.hpp" + +#include "pass_fixture.h" + +namespace { + +using spvtools::CreateNullPass; +using spvtools::CreateStripDebugInfoPass; +using spvtools::Optimizer; +using spvtools::SpirvTools; +using ::testing::Eq; + +TEST(Optimizer, CanRunNullPassWithDistinctInputOutputVectors) { + SpirvTools tools(SPV_ENV_UNIVERSAL_1_0); + std::vector binary_in; + tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary_in); + + Optimizer opt(SPV_ENV_UNIVERSAL_1_0); + opt.RegisterPass(CreateNullPass()); + std::vector binary_out; + opt.Run(binary_in.data(), binary_in.size(), &binary_out); + + std::string disassembly; + tools.Disassemble(binary_out.data(), binary_out.size(), &disassembly); + EXPECT_THAT(disassembly, Eq("OpName %foo \"foo\"\n%foo = OpTypeVoid\n")); +} + +TEST(Optimizer, CanRunTransformingPassWithDistinctInputOutputVectors) { + SpirvTools tools(SPV_ENV_UNIVERSAL_1_0); + std::vector binary_in; + tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary_in); + + Optimizer opt(SPV_ENV_UNIVERSAL_1_0); + opt.RegisterPass(CreateStripDebugInfoPass()); + std::vector binary_out; + opt.Run(binary_in.data(), binary_in.size(), &binary_out); + + std::string disassembly; + tools.Disassemble(binary_out.data(), binary_out.size(), &disassembly); + EXPECT_THAT(disassembly, Eq("%void = OpTypeVoid\n")); +} + +TEST(Optimizer, CanRunNullPassWithAliasedVectors) { + SpirvTools tools(SPV_ENV_UNIVERSAL_1_0); + std::vector binary; + tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary); + + Optimizer opt(SPV_ENV_UNIVERSAL_1_0); + opt.RegisterPass(CreateNullPass()); + opt.Run(binary.data(), binary.size(), &binary); // This is the key. + + std::string disassembly; + tools.Disassemble(binary.data(), binary.size(), &disassembly); + EXPECT_THAT(disassembly, Eq("OpName %foo \"foo\"\n%foo = OpTypeVoid\n")); +} + +TEST(Optimizer, CanRunNullPassWithAliasedVectorDataButDifferentSize) { + SpirvTools tools(SPV_ENV_UNIVERSAL_1_0); + std::vector binary; + tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary); + + Optimizer opt(SPV_ENV_UNIVERSAL_1_0); + opt.RegisterPass(CreateNullPass()); + auto orig_size = binary.size(); + // Now change the size. Add a word that will be ignored + // by the optimizer. + binary.push_back(42); + EXPECT_THAT(orig_size + 1, Eq(binary.size())); + opt.Run(binary.data(), orig_size, &binary); // This is the key. + // The binary vector should have been rewritten. + EXPECT_THAT(binary.size(), Eq(orig_size)); + + std::string disassembly; + tools.Disassemble(binary.data(), binary.size(), &disassembly); + EXPECT_THAT(disassembly, Eq("OpName %foo \"foo\"\n%foo = OpTypeVoid\n")); +} + +TEST(Optimizer, CanRunTransformingPassWithAliasedVectors) { + SpirvTools tools(SPV_ENV_UNIVERSAL_1_0); + std::vector binary; + tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary); + + Optimizer opt(SPV_ENV_UNIVERSAL_1_0); + opt.RegisterPass(CreateStripDebugInfoPass()); + opt.Run(binary.data(), binary.size(), &binary); // This is the key + + std::string disassembly; + tools.Disassemble(binary.data(), binary.size(), &disassembly); + EXPECT_THAT(disassembly, Eq("%void = OpTypeVoid\n")); +} + +} // namespace -- 2.7.4