From 76dfa74756d37289e6eb14ce20cb3d3da5e4577c Mon Sep 17 00:00:00 2001 From: Nikolai Tillmann Date: Thu, 9 Jan 2025 11:25:42 -0800 Subject: [PATCH] Upgrade to editable cfg Summary: This avoids convertions to/from cfg for every time the pass runs, plus within where we used to create a CFG temporarily for further cleanup. This is a behavior-preserving change (except for some minor changes that come due to cfg construction itself). Reviewed By: thezhangwei Differential Revision: D67921413 fbshipit-source-id: 6404f451fdfa7a62fcdd56063dfdea18a589599f --- opt/singleimpl/SingleImpl.h | 1 - opt/singleimpl/SingleImplAnalyze.cpp | 41 +++++++----- opt/singleimpl/SingleImplDefs.h | 8 +-- opt/singleimpl/SingleImplOptimize.cpp | 95 ++++++++++++++++++--------- 4 files changed, 91 insertions(+), 54 deletions(-) diff --git a/opt/singleimpl/SingleImpl.h b/opt/singleimpl/SingleImpl.h index ab0762295f..b218deb38b 100644 --- a/opt/singleimpl/SingleImpl.h +++ b/opt/singleimpl/SingleImpl.h @@ -50,7 +50,6 @@ class SingleImplPass : public Pass { m_pass_config.filter_proguard_special_interfaces); } - bool is_cfg_legacy() override { return true; } void run_pass(DexStoresVector&, ConfigFiles&, PassManager&) override; // count of removed interfaces diff --git a/opt/singleimpl/SingleImplAnalyze.cpp b/opt/singleimpl/SingleImplAnalyze.cpp index 49718b7bab..1b4b250d54 100644 --- a/opt/singleimpl/SingleImplAnalyze.cpp +++ b/opt/singleimpl/SingleImplAnalyze.cpp @@ -359,9 +359,16 @@ void AnalysisImpl::collect_method_defs() { * fieldref or methodref. */ void AnalysisImpl::analyze_opcodes() { + auto register_reference = [](SingleImplData& si, DexMethod* referrer, + IRInstruction* insn, + const cfg::InstructionIterator& insn_it) { + auto& map = si.referencing_methods[referrer]; + auto [it, emplaced] = map.emplace(insn, insn_it); + always_assert(emplaced || it->second == insn_it); + }; auto check_arg = [&](DexMethod* referrer, - const IRList::iterator& insn_it, + const cfg::InstructionIterator& insn_it, DexType* type, DexMethodRef* meth, IRInstruction* insn) { @@ -369,13 +376,13 @@ void AnalysisImpl::analyze_opcodes() { if (intf) { auto& si = single_impls.at(intf); std::lock_guard lock(si.mutex); - si.referencing_methods[referrer][insn] = insn_it; + register_reference(si, referrer, insn, insn_it); si.methodrefs[meth].insert(insn); } }; auto check_sig = [&](DexMethod* referrer, - const IRList::iterator& insn_it, + const cfg::InstructionIterator& insn_it, DexMethodRef* meth, IRInstruction* insn) { // check the sig for single implemented interface @@ -387,7 +394,7 @@ void AnalysisImpl::analyze_opcodes() { }; auto check_field = [&](DexMethod* referrer, - const IRList::iterator& insn_it, + const cfg::InstructionIterator& insn_it, DexFieldRef* field, IRInstruction* insn) { auto cls = field->get_class(); @@ -400,27 +407,27 @@ void AnalysisImpl::analyze_opcodes() { if (intf) { auto& si = single_impls.at(intf); std::lock_guard lock(si.mutex); - si.referencing_methods[referrer][insn] = insn_it; + register_reference(si, referrer, insn, insn_it); si.fieldrefs[field].push_back(insn); } }; auto check_return = [&](DexMethod* referrer, - const IRList::iterator& insn_it, + const cfg::InstructionIterator& insn_it, IRInstruction* insn) { auto rtype = referrer->get_proto()->get_rtype(); auto intf = get_and_check_single_impl(rtype); if (intf) { auto& si = single_impls.at(intf); std::lock_guard lock(si.mutex); - si.referencing_methods[referrer][insn] = insn_it; + register_reference(si, referrer, insn, insn_it); } }; walk::parallel::code(scope, [&](DexMethod* method, IRCode& code) { - redex_assert(!code.editable_cfg_built()); // Need *one* way to - auto ii = ir_list::InstructionIterable(code); - const auto& end = ii.end(); + redex_assert(code.editable_cfg_built()); + auto ii = InstructionIterable(code.cfg()); + auto end = ii.end(); for (auto it = ii.begin(); it != end; ++it) { auto insn = it->insn; auto op = insn->opcode(); @@ -436,7 +443,7 @@ void AnalysisImpl::analyze_opcodes() { if (intf) { auto& si = single_impls.at(intf); std::lock_guard lock(si.mutex); - si.referencing_methods[method][insn] = it.unwrap(); + register_reference(si, method, insn, it); si.typerefs.push_back(insn); } break; @@ -453,7 +460,7 @@ void AnalysisImpl::analyze_opcodes() { if (field == nullptr) { field = insn->get_field(); } - check_field(method, it.unwrap(), field, insn); + check_field(method, it, field, insn); break; } case OPCODE_SGET: @@ -467,7 +474,7 @@ void AnalysisImpl::analyze_opcodes() { if (field == nullptr) { field = insn->get_field(); } - check_field(method, it.unwrap(), field, insn); + check_field(method, it, field, insn); break; } // method ref @@ -485,11 +492,11 @@ void AnalysisImpl::analyze_opcodes() { } else { auto& si = single_impls.at(intf); std::lock_guard lock(si.mutex); - si.referencing_methods[method][insn] = it.unwrap(); + register_reference(si, method, insn, it); si.intf_methodrefs[meth].insert(insn); } } - check_sig(method, it.unwrap(), meth, insn); + check_sig(method, it, meth, insn); break; } @@ -498,11 +505,11 @@ void AnalysisImpl::analyze_opcodes() { case OPCODE_INVOKE_VIRTUAL: case OPCODE_INVOKE_SUPER: { const auto meth = insn->get_method(); - check_sig(method, it.unwrap(), meth, insn); + check_sig(method, it, meth, insn); break; } case OPCODE_RETURN_OBJECT: { - check_return(method, it.unwrap(), insn); + check_return(method, it, insn); break; } default: diff --git a/opt/singleimpl/SingleImplDefs.h b/opt/singleimpl/SingleImplDefs.h index 4d48677e7f..550ff37748 100644 --- a/opt/singleimpl/SingleImplDefs.h +++ b/opt/singleimpl/SingleImplDefs.h @@ -15,9 +15,8 @@ #include "CheckCastTransform.h" #include "ClassHierarchy.h" +#include "ControlFlow.h" #include "DexClass.h" -#include "IRInstruction.h" -#include "IRList.h" #include "SingleImpl.h" namespace api { @@ -128,8 +127,9 @@ struct SingleImplData { // opcodes to a methodref with the single impl interface in the signature MethodToOpcodes methodrefs; - std::unordered_map> + std::unordered_map< + DexMethod*, + std::unordered_map> referencing_methods; std::mutex mutex; diff --git a/opt/singleimpl/SingleImplOptimize.cpp b/opt/singleimpl/SingleImplOptimize.cpp index 333639a91f..b6f2d4ebc7 100644 --- a/opt/singleimpl/SingleImplOptimize.cpp +++ b/opt/singleimpl/SingleImplOptimize.cpp @@ -222,7 +222,10 @@ struct OptimizationImpl { EscapeReason can_optimize(const DexType* intf, const SingleImplData& data, bool rename_on_collision); - CheckCastSet do_optimize(const DexType* intf, const SingleImplData& data); + CheckCastSet do_optimize( + const DexType* intf, + const SingleImplData& data, + std::unordered_map& method_mutations); EscapeReason check_field_collision(const DexType* intf, const SingleImplData& data); EscapeReason check_method_collision(const DexType* intf, @@ -232,8 +235,10 @@ struct OptimizationImpl { DexMethod* method); void set_field_defs(const DexType* intf, const SingleImplData& data); void set_field_refs(const DexType* intf, const SingleImplData& data); - CheckCastSet fix_instructions(const DexType* intf, - const SingleImplData& data); + CheckCastSet fix_instructions( + const DexType* intf, + const SingleImplData& data, + std::unordered_map& method_mutations); void set_method_defs(const DexType* intf, const SingleImplData& data); void set_method_refs(const DexType* intf, const SingleImplData& data); void rewrite_interface_methods(const DexType* intf, @@ -243,7 +248,7 @@ struct OptimizationImpl { const SingleImplData& data); check_casts::impl::Stats post_process( - const std::unordered_set& methods, + const std::vector& methods, std::vector* removed_instruction); std::unique_ptr single_impls; @@ -350,8 +355,10 @@ void for_all_methods(const T& methods, Fn fn, bool parallel = true) { // field value. Expectation is that unnecessary insertions (e.g., // duplicate check-casts) will be eliminated, for example, in // `post_process`. -CheckCastSet OptimizationImpl::fix_instructions(const DexType* intf, - const SingleImplData& data) { +CheckCastSet OptimizationImpl::fix_instructions( + const DexType* intf, + const SingleImplData& data, + std::unordered_map& method_mutations) { if (data.referencing_methods.empty()) { return {}; } @@ -372,19 +379,23 @@ CheckCastSet OptimizationImpl::fix_instructions(const DexType* intf, [&](const DexMethod* caller_const) { auto caller = const_cast(caller_const); std::vector temps; // Cached temps. - auto code = caller->get_code(); - redex_assert(!code->editable_cfg_built()); - + auto* code = caller->get_code(); + always_assert(code->editable_cfg_built()); + auto& cfg = code->cfg(); + auto& mutation = method_mutations.at(caller); for (const auto& insn_it_pair : data.referencing_methods.at(caller)) { auto insn_it = insn_it_pair.second; auto insn = insn_it_pair.first; + always_assert(&insn_it.cfg() == &cfg); auto temp_it = temps.begin(); auto add_check_cast = [&](reg_t reg) { + std::vector new_insns; + new_insns.reserve(2); auto check_cast = new IRInstruction(OPCODE_CHECK_CAST); check_cast->set_src(0, reg); check_cast->set_type(data.cls); - code->insert_before(insn_it, *new MethodItemEntry(check_cast)); + new_insns.push_back(check_cast); if (PARALLEL) { std::unique_lock lock(ret_lock); @@ -396,7 +407,7 @@ CheckCastSet OptimizationImpl::fix_instructions(const DexType* intf, // See if we need a new temp. reg_t out; if (temp_it == temps.end()) { - reg_t new_temp = code->allocate_temp(); + reg_t new_temp = cfg.allocate_temp(); temps.push_back(new_temp); temp_it = temps.end(); out = new_temp; @@ -408,9 +419,8 @@ CheckCastSet OptimizationImpl::fix_instructions(const DexType* intf, auto pseudo_move_result = new IRInstruction(IOPCODE_MOVE_RESULT_PSEUDO_OBJECT); pseudo_move_result->set_dest(out); - code->insert_before(insn_it, - *new MethodItemEntry(pseudo_move_result)); - + new_insns.push_back(pseudo_move_result); + mutation.insert_before(insn_it, std::move(new_insns)); return out; }; @@ -809,9 +819,11 @@ void OptimizationImpl::rename_possible_collisions(const DexType* intf, /** * Perform the optimization. */ -CheckCastSet OptimizationImpl::do_optimize(const DexType* intf, - const SingleImplData& data) { - CheckCastSet ret = fix_instructions(intf, data); +CheckCastSet OptimizationImpl::do_optimize( + const DexType* intf, + const SingleImplData& data, + std::unordered_map& method_mutations) { + CheckCastSet ret = fix_instructions(intf, data, method_mutations); set_type_refs(intf, data); set_field_defs(intf, data); set_field_refs(intf, data); @@ -830,7 +842,23 @@ OptimizeStats OptimizationImpl::optimize(Scope& scope, TypeList to_optimize; single_impls->get_interfaces(to_optimize); std::sort(to_optimize.begin(), to_optimize.end(), compare_dextypes); - std::unordered_set for_post_processing; + + std::unordered_map method_mutations; + std::vector mutated_methods; + for (auto intf : to_optimize) { + auto& intf_data = single_impls->get_single_impl_data(intf); + for (auto&& [method, _] : intf_data.referencing_methods) { + auto* code = method->get_code(); + always_assert(code->editable_cfg_built()); + auto emplaced = + method_mutations.emplace(method, cfg::CFGMutation(code->cfg())) + .second; + if (emplaced) { + mutated_methods.push_back(method); + } + } + } + CheckCastSet inserted_check_casts; for (auto intf : to_optimize) { auto& intf_data = single_impls->get_single_impl_data(intf); @@ -841,14 +869,15 @@ OptimizeStats OptimizationImpl::optimize(Scope& scope, single_impls->escape_interface(intf, escape); continue; } - auto check_casts = do_optimize(intf, intf_data); + auto check_casts = do_optimize(intf, intf_data, method_mutations); inserted_check_casts.insert(check_casts.begin(), check_casts.end()); - for (auto& p : intf_data.referencing_methods) { - for_post_processing.insert(p.first); - } optimized.insert(intf); } + for (auto&& [_, mutation] : method_mutations) { + mutation.flush(); + } + // make a new scope deleting all single impl interfaces Scope new_scope; for (auto cls : scope) { @@ -863,13 +892,15 @@ OptimizeStats OptimizationImpl::optimize(Scope& scope, std::vector removed_instructions; auto post_process_stats = - post_process(for_post_processing, &removed_instructions); + post_process(mutated_methods, &removed_instructions); std::atomic retained{0}; { - for_all_methods(for_post_processing, [&](const DexMethod* m) { - auto code = m->get_code(); + for_all_methods(mutated_methods, [&](const DexMethod* m) { + auto* code = m->get_code(); + always_assert(code->editable_cfg_built()); + auto& cfg = code->cfg(); size_t found = 0; - for (const auto& mie : ir_list::ConstInstructionIterable(*code)) { + for (const auto& mie : InstructionIterable(cfg)) { if (inserted_check_casts.count(mie.insn) != 0) { ++found; } @@ -892,7 +923,7 @@ OptimizeStats OptimizationImpl::optimize(Scope& scope, } check_casts::impl::Stats OptimizationImpl::post_process( - const std::unordered_set& methods, + const std::vector& methods, std::vector* removed_instructions) { // The analysis times the number of methods is easily expensive, run in // parallel. @@ -904,22 +935,22 @@ check_casts::impl::Stats OptimizationImpl::post_process( [&stats, &mutex, removed_instructions, &api](const DexMethod* m_const) { auto m = const_cast(m_const); auto code = m->get_code(); - always_assert(!code->editable_cfg_built()); - cfg::ScopedCFG cfg(code); + always_assert(code->editable_cfg_built()); + auto& cfg = code->cfg(); // T131253060 If enable weaken, we are hitting an assertion in // CheckCastAnalysis where a definition of a value is unknown. This only // occurs here within SingleImplPass, but not in subsequent // CheckCastRemovals where weaken is enabled by default. check_casts::CheckCastConfig config{.weaken = false}; check_casts::impl::CheckCastAnalysis analysis( - config, *cfg, is_static(m), m->get_class(), + config, cfg, is_static(m), m->get_class(), m->get_proto()->get_args(), m->get_proto()->get_rtype(), m->get_param_anno(), api); auto casts = analysis.collect_redundant_checks_replacement(); - auto local_stats = check_casts::impl::apply(*cfg, casts); + auto local_stats = check_casts::impl::apply(cfg, casts); + auto insns = cfg.release_removed_instructions(); std::lock_guard lock_guard(mutex); stats += local_stats; - auto insns = cfg->release_removed_instructions(); removed_instructions->insert(removed_instructions->end(), insns.begin(), insns.end()); },