From 1bc0b275dd74f509259555e99f4e54f235532a8a Mon Sep 17 00:00:00 2001 From: Lei Zhang Date: Tue, 20 Sep 2016 16:48:00 -0400 Subject: [PATCH] Allow changing MessageConsumer in Pass & PassManager. Default-constructed Pass/PassManager will have a MessageConsumer which ignores all messages. SetMessageConsumer() should be called to supply a meaningful MessageConsumer. --- source/opt/eliminate_dead_constant_pass.h | 2 -- .../fold_spec_constant_op_and_composite_pass.cpp | 6 ++-- .../opt/fold_spec_constant_op_and_composite_pass.h | 2 +- source/opt/freeze_spec_constant_value_pass.h | 2 -- source/opt/null_pass.h | 2 -- source/opt/pass.h | 16 +++++++---- source/opt/pass_manager.h | 14 ++++++++-- source/opt/set_spec_constant_default_value_pass.h | 11 ++++---- source/opt/strip_debug_info_pass.h | 2 -- source/opt/unify_const_pass.h | 2 -- test/opt/pass_fixture.h | 10 +++++-- test/opt/test_line_debug_info.cpp | 2 -- test/opt/test_pass_manager.cpp | 32 ++++++++-------------- tools/opt/opt.cpp | 7 +++-- 14 files changed, 52 insertions(+), 58 deletions(-) diff --git a/source/opt/eliminate_dead_constant_pass.h b/source/opt/eliminate_dead_constant_pass.h index 80d8a9a..6e9a278 100644 --- a/source/opt/eliminate_dead_constant_pass.h +++ b/source/opt/eliminate_dead_constant_pass.h @@ -28,8 +28,6 @@ namespace opt { // OpSpecConstantOp. class EliminateDeadConstantPass : public Pass { public: - explicit EliminateDeadConstantPass(const MessageConsumer& c) : Pass(c) {} - const char* name() const override { return "eliminate-dead-const"; } Status Process(ir::Module*) override; }; diff --git a/source/opt/fold_spec_constant_op_and_composite_pass.cpp b/source/opt/fold_spec_constant_op_and_composite_pass.cpp index 1922992..0170dcb 100644 --- a/source/opt/fold_spec_constant_op_and_composite_pass.cpp +++ b/source/opt/fold_spec_constant_op_and_composite_pass.cpp @@ -242,10 +242,8 @@ std::vector OperateVectors( } } // anonymous namespace -FoldSpecConstantOpAndCompositePass::FoldSpecConstantOpAndCompositePass( - const MessageConsumer& c) - : Pass(c), - max_id_(0), +FoldSpecConstantOpAndCompositePass::FoldSpecConstantOpAndCompositePass() + : max_id_(0), module_(nullptr), def_use_mgr_(nullptr), type_mgr_(nullptr), diff --git a/source/opt/fold_spec_constant_op_and_composite_pass.h b/source/opt/fold_spec_constant_op_and_composite_pass.h index aff7af6..7b07fbf 100644 --- a/source/opt/fold_spec_constant_op_and_composite_pass.h +++ b/source/opt/fold_spec_constant_op_and_composite_pass.h @@ -50,7 +50,7 @@ namespace opt { // TODO(qining): Add support for the operations listed above. class FoldSpecConstantOpAndCompositePass : public Pass { public: - explicit FoldSpecConstantOpAndCompositePass(const MessageConsumer& c); + FoldSpecConstantOpAndCompositePass(); const char* name() const override { return "fold-spec-const-op-composite"; } Status Process(ir::Module* module) override { diff --git a/source/opt/freeze_spec_constant_value_pass.h b/source/opt/freeze_spec_constant_value_pass.h index 7dd31b4..5e74a42 100644 --- a/source/opt/freeze_spec_constant_value_pass.h +++ b/source/opt/freeze_spec_constant_value_pass.h @@ -31,8 +31,6 @@ namespace opt { // other spec constants defined by OpSpecConstantComposite or OpSpecConstantOp. class FreezeSpecConstantValuePass : public Pass { public: - explicit FreezeSpecConstantValuePass(const MessageConsumer& c) : Pass(c) {} - const char* name() const override { return "freeze-spec-const"; } Status Process(ir::Module*) override; }; diff --git a/source/opt/null_pass.h b/source/opt/null_pass.h index 5b65771..a3306b5 100644 --- a/source/opt/null_pass.h +++ b/source/opt/null_pass.h @@ -24,8 +24,6 @@ namespace opt { // A null pass that does nothing. class NullPass : public Pass { public: - explicit NullPass(const MessageConsumer& c) : Pass(c) {} - const char* name() const override { return "null"; } Status Process(ir::Module*) override { return Status::SuccessWithoutChange; } }; diff --git a/source/opt/pass.h b/source/opt/pass.h index cedb7c8..3d5aac8 100644 --- a/source/opt/pass.h +++ b/source/opt/pass.h @@ -37,15 +37,19 @@ class Pass { SuccessWithoutChange = 0x11, }; - // Constructs a new pass with the given message consumer, which will be - // invoked every time there is a message to be communicated to the outside. + // Constructs a new pass. // - // This pass just keeps a reference to the message consumer; so the message - // consumer should outlive this pass. - explicit Pass(const MessageConsumer& c) : consumer_(c) {} + // The constructed instance will have an empty message consumer, which just + // ignores all messages from the library. Use SetMessageConsumer() to supply + // one if messages are of concern. + Pass() : consumer_(IgnoreMessage) {} // Returns a descriptive name for this pass. virtual const char* name() const = 0; + + // Sets the message consumer to the given |consumer|. |consumer| which will be + // invoked every time there is a message to be communicated to the outside. + void SetMessageConsumer(MessageConsumer c) { consumer_ = std::move(c); } // Returns the reference to the message consumer for this pass. const MessageConsumer& consumer() const { return consumer_; } @@ -55,7 +59,7 @@ class Pass { virtual Status Process(ir::Module* module) = 0; private: - const MessageConsumer& consumer_; // Message consumer. + MessageConsumer consumer_; // Message consumer. }; } // namespace opt diff --git a/source/opt/pass_manager.h b/source/opt/pass_manager.h index 61dc4b5..40c16ba 100644 --- a/source/opt/pass_manager.h +++ b/source/opt/pass_manager.h @@ -31,8 +31,15 @@ namespace opt { // to run on a module. Passes are executed in the exact order of addition. class PassManager { public: - // Constructs a pass manager with the given message consumer. - explicit PassManager(MessageConsumer c) : consumer_(std::move(c)) {} + // Constructs a pass manager. + // + // The constructed instance will have an empty message consumer, which just + // ignores all messages from the library. Use SetMessageConsumer() to supply + // one if messages are of concern. + PassManager() : consumer_(IgnoreMessage) {} + + // Sets the message consumer to the given |consumer|. + void SetMessageConsumer(MessageConsumer c) { consumer_ = std::move(c); } // Adds an externally constructed pass. void AddPass(std::unique_ptr pass); @@ -70,7 +77,8 @@ inline void PassManager::AddPass(std::unique_ptr pass) { template inline void PassManager::AddPass(Args&&... args) { - passes_.emplace_back(new T(consumer_, std::forward(args)...)); + passes_.emplace_back(new T(std::forward(args)...)); + passes_.back()->SetMessageConsumer(consumer_); } inline uint32_t PassManager::NumPasses() const { diff --git a/source/opt/set_spec_constant_default_value_pass.h b/source/opt/set_spec_constant_default_value_pass.h index e1c6012..89e6b28 100644 --- a/source/opt/set_spec_constant_default_value_pass.h +++ b/source/opt/set_spec_constant_default_value_pass.h @@ -34,12 +34,11 @@ class SetSpecConstantDefaultValuePass : public Pass { using SpecIdToInstMap = std::unordered_map; // Constructs a pass instance with a map from spec ids to default values. - SetSpecConstantDefaultValuePass(const MessageConsumer& c, - const SpecIdToValueStrMap& default_values) - : Pass(c), spec_id_to_value_(default_values) {} - SetSpecConstantDefaultValuePass(const MessageConsumer& c, - SpecIdToValueStrMap&& default_values) - : Pass(c), spec_id_to_value_(std::move(default_values)) {} + explicit SetSpecConstantDefaultValuePass( + const SpecIdToValueStrMap& default_values) + : spec_id_to_value_(default_values) {} + explicit SetSpecConstantDefaultValuePass(SpecIdToValueStrMap&& default_values) + : spec_id_to_value_(std::move(default_values)) {} const char* name() const override { return "set-spec-const-default-value"; } Status Process(ir::Module*) override; diff --git a/source/opt/strip_debug_info_pass.h b/source/opt/strip_debug_info_pass.h index c2bb1fd..0c3216d 100644 --- a/source/opt/strip_debug_info_pass.h +++ b/source/opt/strip_debug_info_pass.h @@ -25,8 +25,6 @@ namespace opt { // Section 3.32.2 of the SPIR-V spec). class StripDebugInfoPass : public Pass { public: - explicit StripDebugInfoPass(const MessageConsumer& c) : Pass(c) {} - const char* name() const override { return "strip-debug"; } Status Process(ir::Module* module) override; }; diff --git a/source/opt/unify_const_pass.h b/source/opt/unify_const_pass.h index 0edb10d..1a45089 100644 --- a/source/opt/unify_const_pass.h +++ b/source/opt/unify_const_pass.h @@ -36,8 +36,6 @@ namespace opt { // 3) NaN in float point format with different bit patterns are not unified. class UnifyConstantPass : public Pass { public: - explicit UnifyConstantPass(const MessageConsumer& c) : Pass(c) {} - const char* name() const override { return "unify-const"; } Status Process(ir::Module*) override; }; diff --git a/test/opt/pass_fixture.h b/test/opt/pass_fixture.h index 9ce5fc9..08a78dc 100644 --- a/test/opt/pass_fixture.h +++ b/test/opt/pass_fixture.h @@ -43,7 +43,7 @@ class PassTest : public TestT { PassTest() : consumer_(IgnoreMessage), tools_(SPV_ENV_UNIVERSAL_1_1), - manager_(new opt::PassManager(consumer_)) {} + manager_(new opt::PassManager()) {} // Runs the given |pass| on the binary assembled from the |assembly|, and // disassebles the optimized binary. Returns a tuple of disassembly string @@ -75,7 +75,8 @@ class PassTest : public TestT { template std::tuple SinglePassRunAndDisassemble( const std::string& assembly, bool skip_nop, Args&&... args) { - auto pass = MakeUnique(consumer_, std::forward(args)...); + auto pass = MakeUnique(std::forward(args)...); + pass->SetMessageConsumer(consumer_); return OptimizeAndDisassemble(pass.get(), assembly, skip_nop); } @@ -105,7 +106,10 @@ class PassTest : public TestT { } // Renews the pass manager, including clearing all previously added passes. - void RenewPassManger() { manager_.reset(new opt::PassManager(consumer_)); } + void RenewPassManger() { + manager_.reset(new opt::PassManager()); + manager_->SetMessageConsumer(consumer_); + } // Runs the passes added thus far using a pass manager on the binary assembled // from the |original| assembly, and checks whether the optimized binary can diff --git a/test/opt/test_line_debug_info.cpp b/test/opt/test_line_debug_info.cpp index a4af497..43edc94 100644 --- a/test/opt/test_line_debug_info.cpp +++ b/test/opt/test_line_debug_info.cpp @@ -22,8 +22,6 @@ using namespace spvtools; // A pass turning all none debug line instructions into Nop. class NopifyPass : public opt::Pass { public: - explicit NopifyPass(const MessageConsumer& c) : opt::Pass(c) {} - const char* name() const override { return "NopifyPass"; } Status Process(ir::Module* module) override { bool modified = false; diff --git a/test/opt/test_pass_manager.cpp b/test/opt/test_pass_manager.cpp index 71ca3ac..704aa53 100644 --- a/test/opt/test_pass_manager.cpp +++ b/test/opt/test_pass_manager.cpp @@ -29,25 +29,23 @@ using ::testing::Eq; // A null pass whose construtors accept arguments class NullPassWithArgs : public opt::NullPass { public: - NullPassWithArgs(const MessageConsumer& c, uint32_t) : NullPass(c) {} - NullPassWithArgs(const MessageConsumer& c, std::string) : NullPass(c) {} - NullPassWithArgs(const MessageConsumer& c, const std::vector&) - : NullPass(c) {} - NullPassWithArgs(const MessageConsumer& c, const std::vector&, uint32_t) - : NullPass(c) {} + NullPassWithArgs(uint32_t) {} + NullPassWithArgs(std::string) {} + NullPassWithArgs(const std::vector&) {} + NullPassWithArgs(const std::vector&, uint32_t) {} const char* name() const override { return "null-with-args"; } }; TEST(PassManager, Interface) { - opt::PassManager manager(IgnoreMessage); + opt::PassManager manager; EXPECT_EQ(0u, manager.NumPasses()); manager.AddPass(); EXPECT_EQ(1u, manager.NumPasses()); EXPECT_STREQ("strip-debug", manager.GetPass(0)->name()); - manager.AddPass(MakeUnique(IgnoreMessage)); + manager.AddPass(MakeUnique()); EXPECT_EQ(2u, manager.NumPasses()); EXPECT_STREQ("strip-debug", manager.GetPass(0)->name()); EXPECT_STREQ("null", manager.GetPass(1)->name()); @@ -75,8 +73,6 @@ TEST(PassManager, Interface) { // A pass that appends an OpNop instruction to the debug section. class AppendOpNopPass : public opt::Pass { public: - explicit AppendOpNopPass(const MessageConsumer& c) : opt::Pass(c) {} - const char* name() const override { return "AppendOpNop"; } Status Process(ir::Module* module) override { module->AddDebugInst(MakeUnique()); @@ -88,8 +84,7 @@ class AppendOpNopPass : public opt::Pass { // section. class AppendMultipleOpNopPass : public opt::Pass { public: - AppendMultipleOpNopPass(const MessageConsumer& c, uint32_t num_nop) - : opt::Pass(c), num_nop_(num_nop) {} + explicit AppendMultipleOpNopPass(uint32_t num_nop) : num_nop_(num_nop) {} const char* name() const override { return "AppendOpNop"; } Status Process(ir::Module* module) override { @@ -106,8 +101,6 @@ class AppendMultipleOpNopPass : public opt::Pass { // A pass that duplicates the last instruction in the debug section. class DuplicateInstPass : public opt::Pass { public: - explicit DuplicateInstPass(const MessageConsumer& c) : opt::Pass(c) {} - const char* name() const override { return "DuplicateInst"; } Status Process(ir::Module* module) override { auto inst = MakeUnique(*(--module->debug_end())); @@ -143,8 +136,7 @@ TEST_F(PassManagerTest, Run) { // A pass that appends an OpTypeVoid instruction that uses a given id. class AppendTypeVoidInstPass : public opt::Pass { public: - AppendTypeVoidInstPass(const MessageConsumer& c, uint32_t result_id) - : opt::Pass(c), result_id_(result_id) {} + explicit AppendTypeVoidInstPass(uint32_t result_id) : result_id_(result_id) {} const char* name() const override { return "AppendTypeVoidInstPass"; } Status Process(ir::Module* module) override { @@ -162,14 +154,14 @@ TEST(PassManager, RecomputeIdBoundAutomatically) { ir::Module module; EXPECT_THAT(GetIdBound(module), Eq(0u)); - opt::PassManager manager(IgnoreMessage); + 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(IgnoreMessage, 100)); + manager.AddPass(MakeUnique(100)); EXPECT_THAT(GetIdBound(module), Eq(0u)); manager.Run(&module); // The Id has been updated automatically, even though the pass @@ -177,12 +169,12 @@ TEST(PassManager, RecomputeIdBoundAutomatically) { EXPECT_THAT(GetIdBound(module), Eq(101u)); // Try one more time! - manager.AddPass(MakeUnique(IgnoreMessage, 200)); + 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(IgnoreMessage, 10)); + manager.AddPass(MakeUnique(10)); manager.Run(&module); // The Id stays high. EXPECT_THAT(GetIdBound(module), Eq(201u)); diff --git a/tools/opt/opt.cpp b/tools/opt/opt.cpp index cc10dfc..558563a 100644 --- a/tools/opt/opt.cpp +++ b/tools/opt/opt.cpp @@ -64,9 +64,10 @@ int main(int argc, char** argv) { spv_target_env target_env = SPV_ENV_UNIVERSAL_1_1; - opt::PassManager pass_manager([](MessageLevel level, const char* source, - const spv_position_t& position, - const char* message) { + opt::PassManager pass_manager; + pass_manager.SetMessageConsumer([](MessageLevel level, const char* source, + const spv_position_t& position, + const char* message) { std::cerr << StringifyMessage(level, source, position, message); }); -- 2.7.4