Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[onert] Configure context_data_map using the backend info of lgraph #12653

Merged
merged 1 commit into from
Feb 22, 2024

Conversation

jyoungyun
Copy link
Contributor

@jyoungyun jyoungyun commented Feb 19, 2024

This commit configures the context_data_map using the backend
information of lgraph.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun [email protected]
Co-authored-by: Hyeongseok Oh [email protected]

@jyoungyun jyoungyun requested review from YongseopKim and a team February 19, 2024 11:13
// Generate partial graphs for each backend
for (auto &&backend : backend_manager.getAll())
for (auto &&backend : backends)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As i understood correctly, the difference is here :

Before

create backend from backend_manager.getAll()

After

create backend from CompilerOptions config

Is there a difference between backend_manager.getAll() and config lists?

Copy link
Contributor Author

@jyoungyun jyoungyun Feb 21, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it can be different.
A scenario like this is as follows:

flowchart TD
   create_session --> id0["set_available_backends('train')"] -- "BACKENDS config: ['train']\nbackend_manager: ['train']" --> train_prepare --> train --> id1["set_available_backends('cpu')"] -- "BACKENDS config: ['cpu']\nbackend_manager: ['train', 'cpu']" --> prepare --> run --> close_session
Loading

@hseok-oh
Copy link
Contributor

hseok-oh commented Feb 20, 2024

I think backend configuration has already been reflected in the lowered graph. So I suggest to use lgraph iteration instead of getting backend list

@@ -160,18 +160,17 @@ createBackendContexts(compiler::ILoweredGraph &lgraph, bool linear_executor,
                       std::shared_ptr<backend::custom::IKernelBuilder> custom_kernel_builder)
 {
   backend::BackendContexts contexts;
-  auto &backend_manager = compiler::BackendManager::get();
-
   std::unordered_map<const backend::Backend *, backend::ContextData> context_data_map;
 
-  // Generate partial graphs for each backend
-  for (auto &&backend : backend_manager.getAll())
-  {
+  auto init_context_data = [&](const backend::Backend *backend) {
     auto &data = context_data_map[backend];
     auto graph = std::make_unique<ir::Graph>();
     graph->setLayout(lgraph.graph().layout());
     data.graph = std::move(graph);
-  }
+  };
 
   auto &whole_graph = lgraph.graph();
   // Separate operands into partial graphs
@@ -182,6 +181,9 @@ createBackendContexts(compiler::ILoweredGraph &lgraph, bool linear_executor,
       return;
     const auto &def_factor = def_factors.getOnlyElement();
     const auto backend = def_factor.backend();
+    if (context_data_map.find(backend) == context_data_map.end())
+      init_context_data(backend);
+
     auto &partial_graph = *context_data_map[backend].graph;
     auto &operand_layouts = context_data_map[backend].operand_layouts;
     assert(operand_layouts.find(operand_ind) == operand_layouts.end());
@@ -200,6 +202,9 @@ createBackendContexts(compiler::ILoweredGraph &lgraph, bool linear_executor,
     [&](const ir::OperationIndex &op_ind, const ir::IOperation &operation) {
       auto &op_li = lgraph.lower_info().operation;
       auto backend = op_li.at(op_ind).backend();
+      if (context_data_map.find(backend) == context_data_map.end())
+        init_context_data(backend);
+
       auto &partial_graph = *context_data_map[backend].graph;
       auto &external_operands = context_data_map[backend].external_operands;
       auto &operand_layouts = context_data_map[backend].operand_layouts;

@YongseopKim
Copy link
Contributor

I'll review after @jyoungyun responds to @hseok-oh 's comment.

@jyoungyun
Copy link
Contributor Author

@hseok-oh Your suggestion looks good. The backend of each operation and operand has already been specified. So we can use the backend information of each operation and operand. One concern is that each operation and operand must find a maching backend each time. This is done in the preparation stage, so it is not time-sensitive. However, if possible, I'd like to define the context_data_map in advance. I will take a closer look. :)

@jyoungyun
Copy link
Contributor Author

The context_data_map is an unordered_map. And the time complexity of finding key is just O(1) on average. So I don't think this will affect the performance! I will fix this PR. :)

This commit configures the `context_data_map` using the backend
information of lgraph.

ONE-DCO-1.0-Signed-off-by: Jiyoung Yun <[email protected]>
Co-authored-by: Hyeongseok Oh <[email protected]>
@jyoungyun jyoungyun force-pushed the onert/check_backends branch from 4100ef6 to af3d7be Compare February 21, 2024 05:43
@jyoungyun jyoungyun changed the title [onert] Create a backend only for the ones in BACKENDS config [onert] Configure context_data_map using the backend info of lgraph Feb 21, 2024
@jyoungyun jyoungyun requested a review from a team February 21, 2024 05:44
Copy link
Contributor

@YongseopKim YongseopKim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! It works well on my draft.

Copy link
Contributor

@hseok-oh hseok-oh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@jyoungyun
Copy link
Contributor Author

/cc @zetwhite

@jyoungyun jyoungyun requested review from zetwhite and removed request for zetwhite February 21, 2024 08:51
@hseok-oh hseok-oh merged commit 2103774 into Samsung:master Feb 22, 2024
9 checks passed
@jyoungyun jyoungyun deleted the onert/check_backends branch February 22, 2024 01:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants