From b81d6bc68f1be52f99cc701cab8f3ec9208af182 Mon Sep 17 00:00:00 2001 From: Zichun Ye Date: Thu, 17 Mar 2022 14:21:55 +0800 Subject: [PATCH] fix code check problem update code review comments --- .../common/graph_kernel/model/op_register.cc | 6 +- .../common/graph_kernel/model/op_register.h | 2 +- .../common/graph_kernel/optimize_assign.cc | 6 +- .../graph_kernel/parallel_cost_model.cc | 4 +- .../common/graph_kernel/parallel_cost_model.h | 2 +- .../common/graph_kernel/parallel_fusion.cc | 98 +++++++++---------- .../common/graph_kernel/split_model/area.cc | 2 +- .../graph_kernel/split_model/fuse_pattern.h | 4 +- .../graph_kernel/split_model/split_model.cc | 16 +-- .../graph_kernel/split_model/split_model.h | 4 +- .../split_model/split_model_cpu.h | 4 + 11 files changed, 79 insertions(+), 69 deletions(-) diff --git a/mindspore/ccsrc/common/graph_kernel/model/op_register.cc b/mindspore/ccsrc/common/graph_kernel/model/op_register.cc index c4d363e1f9f..485a3af6631 100644 --- a/mindspore/ccsrc/common/graph_kernel/model/op_register.cc +++ b/mindspore/ccsrc/common/graph_kernel/model/op_register.cc @@ -20,12 +20,14 @@ namespace mindspore::graphkernel::inner { namespace { class OpRegister { public: - OpRegister(const std::string &name, const CreatorFunc &func) { OpRegistry::Instance().Register(name, func); } + OpRegister(const std::string &name, const CreatorFunc &func) : name_(name) { + OpRegistry::Instance().Register(name, func); + } ~OpRegister() = default; protected: // for pclint-plus - bool rev_{false}; + std::string name_; }; #define JOIN(x, y) x##y diff --git a/mindspore/ccsrc/common/graph_kernel/model/op_register.h b/mindspore/ccsrc/common/graph_kernel/model/op_register.h index 0396c4d470b..4bf22a9a716 100644 --- a/mindspore/ccsrc/common/graph_kernel/model/op_register.h +++ b/mindspore/ccsrc/common/graph_kernel/model/op_register.h @@ -30,7 +30,7 @@ class OpRegistry { static OpRegistry instance{}; return instance; } - void Register(const std::string &op_name, const CreatorFunc &func) { creators.insert({op_name, func}); } + void Register(const std::string &op_name, const CreatorFunc &func) { (void)creators.emplace(op_name, func); } PrimOpPtr NewOp(const std::string &op) { // "OpaqueOp" is registered by default. diff --git a/mindspore/ccsrc/common/graph_kernel/optimize_assign.cc b/mindspore/ccsrc/common/graph_kernel/optimize_assign.cc index 5840d668f39..8bf54770da7 100644 --- a/mindspore/ccsrc/common/graph_kernel/optimize_assign.cc +++ b/mindspore/ccsrc/common/graph_kernel/optimize_assign.cc @@ -107,7 +107,7 @@ void KeepExecOrder(const FuncGraphPtr &func_graph, const AnfNodePtr &getitem, co load_node->set_abstract(assign_to_node->abstract()); func_graph->AddNode(load_node); - mng->Replace(getitem, load_node); + (void)mng->Replace(getitem, load_node); } int64_t GetitemIndex(const AnfNodePtr &getitem) { @@ -131,7 +131,7 @@ void UpdateUsersOfGraphKernel(const FuncGraphPtr &func_graph, const AnfNodePtr & // 2. If the `cnode` has another path to the getitem_user, it's unnecessary to add update_state and load node to // keep exec_order. if (HasPathToParamUser(cnode, getitem_user, getitem)) { - mng->Replace(getitem, assign_to); + (void)mng->Replace(getitem, assign_to); continue; } KeepExecOrder(func_graph, getitem, assign_to, mng); @@ -180,7 +180,7 @@ bool ReplaceAssignByInplaceAssignInGraphkernel(const FuncGraphPtr &func_graph) { std::vector output_types = {input_types.back()}; auto graph_sel_info = BuildSelectKernelBuildInfo(input_formats, input_types, output_formats, output_types, cnode); AnfAlgo::SetSelectKernelBuildInfo(graph_sel_info, new_cnode.get()); - mng->Replace(cnode, new_cnode); + (void)mng->Replace(cnode, new_cnode); } return changed; } diff --git a/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.cc b/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.cc index 63bc8f1f4d7..fef258376b1 100644 --- a/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.cc +++ b/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.cc @@ -49,8 +49,8 @@ std::tuple, int, FusionInfoPtr> ParallelCostModel::CalFu const AnfNodePtrList &nodes) const { nlohmann::json json_desc; std::vector graphs; - std::transform(nodes.begin(), nodes.end(), std::back_inserter(graphs), - [](const AnfNodePtr &node) -> AnfNodePtrList { return {node}; }); + (void)std::transform(nodes.begin(), nodes.end(), std::back_inserter(graphs), + [](const AnfNodePtr &node) -> AnfNodePtrList { return {node}; }); DumpOption dump_option; if (!AnfToJsonDesc(graphs, dump_option, &json_desc)) { MS_LOG(EXCEPTION) << "Collect json desc failed."; diff --git a/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.h b/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.h index 39b5299f7a3..d4c656d2b94 100644 --- a/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.h +++ b/mindspore/ccsrc/common/graph_kernel/parallel_cost_model.h @@ -107,7 +107,7 @@ using ParallelCostModelPtr = std::shared_ptr; class ParellelCostModelWarehouse { public: static ParellelCostModelWarehouse &Instance() { - static ParellelCostModelWarehouse instance; + static ParellelCostModelWarehouse instance = ParellelCostModelWarehouse(); return instance; } ParallelCostModelPtr GetParallelCostModel(const std::string &target) const; diff --git a/mindspore/ccsrc/common/graph_kernel/parallel_fusion.cc b/mindspore/ccsrc/common/graph_kernel/parallel_fusion.cc index f3c3a342d49..f42d8838e8a 100644 --- a/mindspore/ccsrc/common/graph_kernel/parallel_fusion.cc +++ b/mindspore/ccsrc/common/graph_kernel/parallel_fusion.cc @@ -40,7 +40,7 @@ bool IsOneOf(const AnfNodePtr &node, const std::vector &ops_prim) } void ProcessThroughPassCNode(const std::function &pass_fn, - OrderedMap *const node_rels) { + OrderedMap *node_rels) { std::set latter_to_be_erased; for (const auto &[node, node_rel] : (*node_rels)) { if (!pass_fn(node) || latter_to_be_erased.count(node) != 0) { @@ -62,35 +62,35 @@ void ProcessThroughPassCNode(const std::function &pass continue; } - latter_to_be_erased.insert(cur_node); + (void)latter_to_be_erased.insert(cur_node); auto predecessors = (*node_rels)[cur_node].pres; if (predecessors.empty()) { continue; } for (const auto &pre_node : predecessors) { - (*node_rels)[cur_node].pres.erase(pre_node); - (*node_rels)[pre_node].nexts.erase(cur_node); + (void)(*node_rels)[cur_node].pres.erase(pre_node); + (void)(*node_rels)[pre_node].nexts.erase(cur_node); node_que.push(pre_node); } } // Modify the relation: delete node <-> next_node, add pre node <-> next_node. for (const auto &next_node : nexts) { - (*node_rels)[next_node].pres.erase(node); + (void)(*node_rels)[next_node].pres.erase(node); for (const auto &cur_node : pre_nodes) { - (*node_rels)[next_node].pres.insert(cur_node); - (*node_rels)[cur_node].nexts.insert(next_node); + (void)(*node_rels)[next_node].pres.insert(cur_node); + (void)(*node_rels)[cur_node].nexts.insert(next_node); } } } for (const auto &node : latter_to_be_erased) { - node_rels->erase(node); + (void)node_rels->erase(node); } } -void ProcessTailMakeTupleCNode(OrderedMap *const node_rels) { +void ProcessTailMakeTupleCNode(OrderedMap *node_rels) { AnfNodePtrList latter_to_be_erased; for (auto &[node, node_rel] : (*node_rels)) { if (!IsPrimitiveCNode(node, prim::kPrimMakeTuple)) { @@ -123,15 +123,15 @@ void ProcessTailMakeTupleCNode(OrderedMap *const node_ // Delete Tail MakeTuple(including its getitem nodes). for (const auto &node : latter_to_be_erased) { for (auto &pre : (*node_rels)[node].pres) { - (*node_rels)[pre].nexts.erase(node); + (void)(*node_rels)[pre].nexts.erase(node); } // Tail MakeTuple is just be consumed by nothing or invalid getitem node. for (auto &getitem : (*node_rels)[node].nexts) { - node_rels->erase(getitem); + (void)node_rels->erase(getitem); } - node_rels->erase(node); + (void)node_rels->erase(node); } } @@ -177,8 +177,8 @@ bool IsNoOutputsNode(const OrderedMap &node_rels, cons return false; } -void ProcessLocalStructure(OrderedMap *node_rels, - std::set *const virtual_noout_nodes, std::set *ignore_noin_nodes) { +void ProcessLocalStructure(OrderedMap *node_rels, std::set *virtual_noout_nodes, + std::set *ignore_noin_nodes) { // 1. Local relation // Graph as following left part, relation D->B and D->E(D is a no input node) // will make B and E to be multiply inputs node. @@ -218,16 +218,16 @@ void ProcessLocalStructure(OrderedMap *node_rels, serial_tail = cur_node; cur_node = *((*node_rels)[cur_node].nexts.begin()); } - latter_delete.emplace_back(serial_tail, cur_node); + (void)latter_delete.emplace_back(serial_tail, cur_node); } } // Delete relation. for (const auto &[serial_tail, cur_node] : latter_delete) { - virtual_noout_nodes->insert(serial_tail); - ignore_noin_nodes->insert(cur_node); - (*node_rels)[serial_tail].nexts.erase(cur_node); - (*node_rels)[cur_node].pres.erase(serial_tail); + (void)virtual_noout_nodes->insert(serial_tail); + (void)ignore_noin_nodes->insert(cur_node); + (void)(*node_rels)[serial_tail].nexts.erase(cur_node); + (void)(*node_rels)[cur_node].pres.erase(serial_tail); MS_LOG(INFO) << "Process local relation delete relation: " << serial_tail->fullname_with_scope() << " -> " << cur_node->fullname_with_scope(); } @@ -302,7 +302,7 @@ bool Parallelizable(const AnfNodePtr &node) { return WhiteOpsFilter(node) && !Un std::vector SearchFromNodes(const AnfNodePtrList &nodes, const std::function &filter_func, const OrderedMap &node_rels, bool is_backward, - std::set *const seen) { + std::set *seen) { // Start from multi-inputs node, stop on seen node or multi-inputs or multi-outputs nodes. // For backward search, the other multi-inputs node can be contained in. // For forward search, the other multi-outputs node can be contained in. @@ -319,12 +319,12 @@ std::vector SearchFromNodes(const AnfNodePtrList &nodes, iter = node_rels.find(n)) { if (filter_func(n)) { stream.push_back(n); - seen->insert(n); + (void)seen->insert(n); } if (get_contain_node_set(iter->second).size() != 1) { break; } - n = *(get_contain_node_set(iter->second).begin()); + n = *(get_contain_node_set(iter->second).cbegin()); } if (stream.size() > 0) { group.push_back(stream); @@ -333,7 +333,7 @@ std::vector SearchFromNodes(const AnfNodePtrList &nodes, if (group.size() == 1) { for (const auto &drop : group[0]) { - seen->erase(drop); + (void)seen->erase(drop); } group.clear(); } @@ -343,8 +343,7 @@ std::vector SearchFromNodes(const AnfNodePtrList &nodes, void SearchStreamFromMultiRelationNode(const AnfNodePtrList &multi_nodes, const OrderedMap &node_rels, bool is_backward, - std::vector> *groups, - std::set *const seen) { + std::vector> *groups, std::set *seen) { auto get_related_nodes = is_backward ? [](const NodeRelation &info) { return info.pres; } : [](const NodeRelation &info) { return info.nexts; }; for (const auto &node : multi_nodes) { @@ -367,8 +366,7 @@ void SearchStreamFromMultiRelationNode(const AnfNodePtrList &multi_nodes, void SearchStreamFromUnidirectionalNode(const AnfNodePtrList &ud_nodes, const OrderedMap &node_rels, bool is_backward, - std::vector> *groups, - std::set *const seen) { + std::vector> *groups, std::set *seen) { groups->push_back(SearchFromNodes(ud_nodes, Parallelizable, node_rels, is_backward, seen)); // Erase empty groups. @@ -426,18 +424,15 @@ inline bool ParameterLimit(const AnfNodePtrList &nodes) { } bool res = true; - switch (AnfAlgo::GetProcessor(nodes[0])) { - case kernel::Processor::CUDA: { - // The number of inputs and outputs for a valid kernel should be less than cuda's limit. - size_t para_count = 0; - for (const auto &node : nodes) { - para_count += common::AnfAlgo::GetInputTensorNum(node); - para_count += common::AnfAlgo::GetOutputTensorNum(node); - } - res = para_count <= CUDA_PARA_LIMIT; - } break; - default: - break; + auto processor_type = AnfAlgo::GetProcessor(nodes[0]); + if (processor_type == kernel::Processor::CUDA) { + // The number of inputs and outputs for a valid kernel should be less than cuda's limit. + size_t para_count = 0; + for (const auto &node : nodes) { + para_count += common::AnfAlgo::GetInputTensorNum(node); + para_count += common::AnfAlgo::GetOutputTensorNum(node); + } + res = para_count <= CUDA_PARA_LIMIT; } return res; @@ -467,8 +462,8 @@ OrderedMap ParallelOpFusion::GenAnalysisGraph(const An continue; } auto behind_node = get_info(input); - prior_node->pres.insert(input); - behind_node->nexts.insert(node); + (void)prior_node->pres.insert(input); + (void)behind_node->nexts.insert(node); } } @@ -550,8 +545,8 @@ std::tuple, std::vector> ParallelOpFusion::DoSea std::vector parallel_infos; std::vector origin_candidates_used(origin_size, false); std::vector sorted_candidates_used(candidates.size(), false); - - for (size_t i = 0; i < candidates.size(); ++i) { + size_t i = 0; + while (i < candidates.size()) { if (sorted_candidates_used[i]) { continue; } @@ -572,7 +567,9 @@ std::tuple, std::vector> ParallelOpFusion::DoSea while (begin <= end) { size_t mid = (begin + end) / 2; std::vector tc(mid); - std::iota(tc.begin(), tc.end(), 1); + for (size_t idx = 0; idx < mid; idx++) { + tc[idx] = idx + 1; + } AnfNodePtrList other_candidates; std::tie(other_candidates, std::ignore) = GetAvaliableNodesByOffset(SizeToInt(i), tc, sorted_candidates_used, candidates, std::set()); @@ -589,7 +586,9 @@ std::tuple, std::vector> ParallelOpFusion::DoSea if (begin > 1) { std::vector tc(begin - 1); - std::iota(tc.begin(), tc.end(), 1); + for (size_t idx = 0; idx < begin - 1; idx++) { + tc[idx] = idx + 1; + } AnfNodePtrList other_candidates; std::tie(other_candidates, std::ignore) = GetAvaliableNodesByOffset(SizeToInt(i), tc, sorted_candidates_used, candidates, std::set()); @@ -610,6 +609,7 @@ std::tuple, std::vector> ParallelOpFusion::DoSea origin_candidates_used[IntToSize(get_index(origin_indices, node))] = true; } } + i++; } // Current nodes is not suitable to fuse, so pop first node to try other fusion possibility. @@ -626,7 +626,7 @@ std::tuple, std::vector> ParallelOpFusion::Searc std::vector indices; for (size_t i = 0; i < cs.size(); ++i) { if (cs[i]) { - (void)origin_indices.emplace(cs[i], i); + origin_indices[cs[i]] = i; indices.push_back(i); } } @@ -646,7 +646,7 @@ std::tuple, std::vector> ParallelOpFusion::Searc std::map sorted_indices; for (size_t i = 0; i < candidates.size(); ++i) { - (void)sorted_indices.emplace(candidates[i], i); + sorted_indices[candidates[i]] = i; } return DoSearchInSortedCandidates(cs.size(), candidates, &origin_indices, &sorted_indices); @@ -685,8 +685,8 @@ void ParallelOpFusion::SearchFuseNodesInParallelGroup(const std::vector 1) { auto [used, fnds] = SearchFuseNodesInCandidates(candidates); - std::transform(fnds.cbegin(), fnds.cend(), std::back_insert_iterator(*parallel_infos), - [](const ParallelInfo &pi) { return pi; }); + (void)std::transform(fnds.cbegin(), fnds.cend(), std::back_insert_iterator(*parallel_infos), + [](const ParallelInfo &pi) { return pi; }); update_tails(used); candidates = get_candidates(); } diff --git a/mindspore/ccsrc/common/graph_kernel/split_model/area.cc b/mindspore/ccsrc/common/graph_kernel/split_model/area.cc index dbd519ce7a8..f8e93737ae0 100644 --- a/mindspore/ccsrc/common/graph_kernel/split_model/area.cc +++ b/mindspore/ccsrc/common/graph_kernel/split_model/area.cc @@ -128,7 +128,7 @@ void Area::FuseInput(const AreaPtr &input_area) { if (iter == inputs_with_relation_.end()) { MS_LOG(EXCEPTION) << "The area " << input_area->ToString() << " should be the input of area " << this->ToString(); } - auto input_idx = iter - inputs_with_relation_.begin(); + auto input_idx = IntToSize(iter - inputs_with_relation_.begin()); if (input_area->is_output_) { is_output_ = true; diff --git a/mindspore/ccsrc/common/graph_kernel/split_model/fuse_pattern.h b/mindspore/ccsrc/common/graph_kernel/split_model/fuse_pattern.h index dbb52b6c132..92b9da34b92 100644 --- a/mindspore/ccsrc/common/graph_kernel/split_model/fuse_pattern.h +++ b/mindspore/ccsrc/common/graph_kernel/split_model/fuse_pattern.h @@ -26,6 +26,7 @@ class CircleChecker { public: // whether it will form a circle if the two areas are fused. virtual bool HasCircle(const AreaPtr &a, const AreaPtr &b) const = 0; + virtual ~CircleChecker() = default; }; using CircleCheckerPtr = std::shared_ptr; @@ -50,7 +51,7 @@ class FusePattern { std::string name() const { return name_; } FuseDirection direction() const { return direction_; } - std::vector &fused_areas() { return fused_areas_; } + std::vector fused_areas_; protected: void Reset() { fused_areas_.clear(); } @@ -65,7 +66,6 @@ class FusePattern { } std::string name_; - std::vector fused_areas_; FuseDirection direction_{FuseDirection::FORWARD}; CircleCheckerPtr circle_checker_{nullptr}; }; diff --git a/mindspore/ccsrc/common/graph_kernel/split_model/split_model.cc b/mindspore/ccsrc/common/graph_kernel/split_model/split_model.cc index 63ba88d736f..333864a4117 100644 --- a/mindspore/ccsrc/common/graph_kernel/split_model/split_model.cc +++ b/mindspore/ccsrc/common/graph_kernel/split_model/split_model.cc @@ -59,7 +59,7 @@ void ReachTable::FuseArea(size_t target, size_t other) { } } // discard other_node. - alive_.erase(other); + (void)alive_.erase(other); } bool ReachTable::HasCircle(const AreaPtr &a, const AreaPtr &b) const { @@ -115,7 +115,7 @@ void SplitModel::AlignShape(const LiteGraphPtr &litegraph) { } if (cur_shape_size > op->shape.size()) { auto num = cur_shape_size - op->shape.size(); - op->shape.insert(op->shape.begin(), num, 1LL); + (void)op->shape.insert(op->shape.begin(), num, 1LL); } } } @@ -150,7 +150,9 @@ void SplitModel::AddPattern(const std::shared_ptr &pn, bool enable) void SplitModel::LimitAreaSize(const AreaPtr &dom, std::vector *areas, size_t max_size) { auto dom_size = dom->size(); - std::for_each(areas->begin(), areas->end(), [&dom_size](const AreaPtr &a) { dom_size += a->size(); }); + for (auto a = areas->begin(); a != areas->end(); ++a) { + dom_size += (*a)->size(); + } if (dom_size <= max_size) { return; } @@ -161,7 +163,7 @@ void SplitModel::LimitAreaSize(const AreaPtr &dom, std::vector *areas, cur_size += a->size(); return cur_size > max_size; }); - areas->erase(iter, areas->end()); + (void)areas->erase(iter, areas->end()); } void SplitModel::FuseAreas(const AreaPtr &dom, const std::vector &areas, FuseDirection direction) { @@ -195,9 +197,9 @@ bool SplitModel::RunOnePattern(const FusePatternPtr &pattern) { } if (pattern->Run(area)) { MS_LOG(DEBUG) << "Area " << area->ToString() << " matches " << pattern->ToString(); - LimitAreaSize(area, &pattern->fused_areas()); - if (!pattern->fused_areas().empty()) { - FuseAreas(area, pattern->fused_areas(), pattern->direction()); + LimitAreaSize(area, &pattern->fused_areas_); + if (!pattern->fused_areas_.empty()) { + FuseAreas(area, pattern->fused_areas_, pattern->direction()); changed = true; continue; } diff --git a/mindspore/ccsrc/common/graph_kernel/split_model/split_model.h b/mindspore/ccsrc/common/graph_kernel/split_model/split_model.h index 0d783d9e556..28ea1ae5cab 100644 --- a/mindspore/ccsrc/common/graph_kernel/split_model/split_model.h +++ b/mindspore/ccsrc/common/graph_kernel/split_model/split_model.h @@ -29,7 +29,7 @@ namespace mindspore::graphkernel::inner { class ReachTable : public CircleChecker { public: explicit ReachTable(size_t size); - ~ReachTable() = default; + virtual ~ReachTable() = default; bool HasCircle(const AreaPtr &a, const AreaPtr &b) const override; // Link area from `from` to `to`. @@ -51,6 +51,8 @@ class SplitModel { public: void Run(const LiteGraphPtr &litegraph); const std::list &areas() const { return areas_; } + SplitModel() = default; + virtual ~SplitModel() = default; protected: // transform the litegraph to areas, and initialize inner tables. diff --git a/mindspore/ccsrc/common/graph_kernel/split_model/split_model_cpu.h b/mindspore/ccsrc/common/graph_kernel/split_model/split_model_cpu.h index ce844689680..439d3795683 100644 --- a/mindspore/ccsrc/common/graph_kernel/split_model/split_model_cpu.h +++ b/mindspore/ccsrc/common/graph_kernel/split_model/split_model_cpu.h @@ -20,6 +20,10 @@ namespace mindspore::graphkernel::inner { class SplitModelCpu : public SplitModel { public: + SplitModelCpu() = default; + virtual ~SplitModelCpu() = default; + + protected: AreaMode GetDefaultAreaMode(const PrimOpPtr &) const override; void InitFusePatterns() override; };