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

diff --git a/CHANGES b/CHANGES
index ffe7b0006df011f77b2d7bbd7f897d1361938d2c..8347371ddd63228c5a6acb0f778940b8955bd7d6 100644 (file)
--- 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 f6dc1cdf41ac455bac8474dc0144fef139428f4c..b1d71973d46ebabd6739297cb9b29280b8a6346b 100644 (file)
--- 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 2f1f482ac29549a393f9dbf9d22ab8862f5226e4..d807d47c5fb22366c05d06c217fa5155f71e7f84 100644 (file)
--- 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 (file)
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 <gmock/gmock.h>
+
+#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<uint32_t> binary_in;
+  tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary_in);
+
+  Optimizer opt(SPV_ENV_UNIVERSAL_1_0);
+  opt.RegisterPass(CreateNullPass());
+  std::vector<uint32_t> 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<uint32_t> binary_in;
+  tools.Assemble("OpName %foo \"foo\"\n%foo = OpTypeVoid", &binary_in);
+
+  Optimizer opt(SPV_ENV_UNIVERSAL_1_0);
+  opt.RegisterPass(CreateStripDebugInfoPass());
+  std::vector<uint32_t> 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<uint32_t> 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<uint32_t> 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<uint32_t> 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