[Relay] Remove memory planing from LowerTEPass #8974

mikepapadim · 2021-09-09T08:05:55Z

This PR is on top of #8914 and removes the memory planing from LowerTEPass.

Add attrs to IRModule equal and hash Make LowerTEPass opt_level 0 Copy IRModule attrs to per_target_modules Add ShallowCopy to IRmodule

electriclilies

One small nit but otherwise LGTM! Thanks :)

electriclilies · 2021-09-09T18:48:27Z

src/relay/backend/aot_executor_codegen.cc

@@ -45,6 +45,13 @@ namespace tvm {
 namespace relay {
 namespace backend {

+struct EnumClassHash {


Could you EnumClassHash into "utils.h" and remove this definition of it and the definition in the te compiler?

…om LowerTEPass

mbs-octoml · 2021-09-10T13:27:59Z

I think this lgtm but will need to take another look once #8914 is in so the diff is clean. Thanks.

masahi · 2021-09-13T10:04:27Z

@mikepapadim #8914 is merged.

mikepapadim · 2021-09-13T10:08:18Z

@mikepapadim #8914 is merged.

I ll have a look why CI is failing in this one now.

masahi · 2021-09-13T10:12:07Z

test_crt_aot.py and test_zephyr.py should have been fixed by #8980

Mousius

Hi @mikepapadim, overall I think the actual change makes sense but the PR touches a lot of unrelated files which will make the history more confused and it's difficult to understand the rationale behind the changes, such as introducing tec:: and changing relative imports to have ./ for this patch.

Would it be possible for you to scope this PR to just the changes that remove the memory planning?

mbs-octoml · 2021-09-14T16:45:32Z

src/relay/backend/interpreter.cc

-       transform::InferType(),
-       tec::LowerTEPass(targets, device_map, memory_plan, /*module_name=*/"intrp",
-                        [](Function func) { /* no-op */ })});
+  transform::Sequential seq({transform::SimplifyInference(),


Seems we have inconsistent formatters? In any case I'd revert this whitespace change.

mbs-octoml · 2021-09-14T16:46:15Z

src/relay/backend/te_compiler.cc

@@ -596,68 +596,19 @@ class LowerTensorExprMutator : public ExprMutator {
  const Op& debug_op_;
 };

-Pass LowerTensorExpr(TargetMap targets, DeviceMap device_context_map,


How about we don't move these to keep the diff down.

mbs-octoml · 2021-09-14T16:58:33Z

@Mousius

but the PR touches a lot of unrelated files

I'll confess I've been doing the same in my PRs and tend to clean up spelling mistakes and minor style glitches as I encounter them without factoring them out. Absolutely agree ideally they'd go in as 'no semantic change' PRs, but the CI delay and review overhead makes me much more forgiving. That said I left two suggestions to lower the diff.

I've also been using "./foo.h" instead of "foo.h" to remind me of the abs imports, but maybe that's just my OCD.

Note that the LowerTEPass call was resolving to tec::LowerTEPass only because the first argument is also in tec::, a fairly obscure C++ namespace trick, so I'd agree spelling those out is a Good Thing.

@mikepapadim, can you prefix the title with [Relay] to follow convention.

mbs-octoml · 2021-09-14T16:59:52Z

LGTM modulo comments.

electriclilies

Overall, looks good to me! Just one comment about code clarity / structure :)
Also I second the thing about additional cleanups-- given CI time it takes a lot of time to pull these out.

Also @mikepapadim, FYI changing the title will retrigger CI!

electriclilies · 2021-09-14T20:08:45Z

src/relay/backend/aot_executor_codegen.cc

+    if (memory_plan.defined()) {
+      // TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize
+      func_info = tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan->expr_to_storage_info);
+    }


Can you just put the func_info on the mod here before passing the module into LowerTE? Then you don't need to re-extract it later, and also the logic surrounding func_info is all in one place. (LowerTEPass should preserve all attributes on modules passed into it)

electriclilies · 2021-09-14T20:11:12Z

src/relay/backend/aot_executor_codegen.cc

    Optional<backend::FunctionInfo> main_func_info =
        lowered_mod->GetAttr<backend::FunctionInfo>("main_func_info");
+


Like I said above, can you attach the func_info right before LowerTEPass is called? And we can then remove the check about whether the attribute is on the module.

electriclilies · 2021-09-14T20:12:09Z

src/relay/backend/graph_executor_codegen.cc

+      func_info =
+          relay::tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan_->expr_to_storage_info);
+    }
+


Same comment as in aot_executor_codegen -- can we put the func_info on the module here instead of after LowerTEPass is called and delete the check for main_func_info being set?

Mousius · 2021-09-14T23:23:10Z

I'll confess I've been doing the same in my PRs and tend to clean up spelling mistakes and minor style glitches as I encounter them without factoring them out. Absolutely agree ideally they'd go in as 'no semantic change' PRs, but the CI delay and review overhead makes me much more forgiving. That said I left two suggestions to lower the diff.

Also I second the thing about additional cleanups-- given CI time it takes a lot of time to pull these out.

Whilst I can appreciate the annoyance that CI brings, this PR introduces more changes in unrelated files than I'm comfortable with. Now that @mbs-octoml has articulated the tec:: and ./ changes, that seems in line with the purpose of this PR. However, I'd suggest the commit history should not show that removing memory planning was the last change to the issue templates, the microTVM Arduino port, Ethos U, Vitis AI and Relay docs.

Also this PR seems to have inadvertently reverted #8973?

I'd propose integrating @mbs-octoml's comments and then scoping the commit to just src/relay for final review?

mikepapadim · 2021-09-15T16:04:21Z

Thanks for the feedback. I think now is good to go.

Mousius

Hi @mikepapadim, thanks for minimising this, I have a few more nits on my side but could you address the comments from @mbs-octoml?

Mousius · 2021-09-15T16:48:23Z

src/relay/backend/aot_executor_codegen.cc

+    backend::FunctionInfo func_info;
+
+    if (memory_plan.defined()) {
+      // TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize
+      func_info = tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan->expr_to_storage_info);
+      mod = WithAttr(mod, "main_func_info", func_info);
+    }


Can we condense this to:

Suggested change

backend::FunctionInfo func_info;

if (memory_plan.defined()) {

// TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize

func_info = tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan->expr_to_storage_info);

mod = WithAttr(mod, "main_func_info", func_info);

}

if (memory_plan.defined()) {

// TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize

backend::FunctionInfo func_info = tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan->expr_to_storage_info);

mod = WithAttr(mod, "main_func_info", func_info);

}

Mousius · 2021-09-15T16:49:15Z

src/relay/backend/graph_executor_codegen.cc

+    backend::FunctionInfo func_info;
+
+    if (memory_plan_.defined()) {
+      // TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize
+      func_info =
+          relay::tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan_->expr_to_storage_info);
+      mod = WithAttr(mod, "main_func_info", func_info);
+    }
+


As above:

Suggested change

backend::FunctionInfo func_info;

if (memory_plan_.defined()) {

// TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize

func_info =

relay::tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan_->expr_to_storage_info);

mod = WithAttr(mod, "main_func_info", func_info);

}

if (memory_plan_.defined()) {

// TODO(@electriclilies, @jroesch): remove UpdateMainWorkspaceSize

backend::FunctionInfo func_info =

relay::tec::UpdateMainWorkspaceSize(mod, targets_, memory_plan_->expr_to_storage_info);

mod = WithAttr(mod, "main_func_info", func_info);

}

jroesch · 2021-09-15T17:35:14Z

@mikepapadim can you follow up on @Mousius's request in a follow up PR, going to land so we can keep the queue clean and should be easy to send small followup.

@AndrewZhaoLuo

* main: (102 commits) Implementation of relay_to_tir target hook (apache#8423) [Onnx] Fix NLL Loss tests (apache#8971) [Bugfix] Fix other div zero errors also in rewrite_simplify (apache#8983) [ONNX] enable the onnx tests after PR apache#8274 merged (apache#9019) [Hexagon] Disable `thread_local` on Hexagon (apache#9025) [Hexagon] Allow undefined symbols in libtvm_runtime.so on Hexagon (apache#9024) [Onnx] Add momentum (apache#9000) fix (apache#9021) [Community] @AndrewZhaoLuo -> Reviewer (apache#9020) [Hexagon] Implement model launcher (apache#8986) [Relay][Pass] Add ExtractOperators pass (apache#8996) [BYOC][TensorRT] Add TensorRT own int8 calibration support to TensorRT BYOC integration (apache#8808) [ONNX] Add Einsum converter (apache#8985) Add standalone_crt/ to be part of the wheel package, when available. (apache#9005) [Relay] Remove memory planing from LowerTEPass (apache#8974) [Hexagon] Treat floats as float32 when passing args to offloaded kernels (apache#9010) [Runtime] Pipeline Executor Initial patch. (apache#8702) [Hexagon] `llvm-options` attribute is an array of strings (apache#9011) disable cuda int8 schedule for non-cuda gpu target (apache#9014) [Torch] Add an option to make imported models compatible with the Relay text parser (apache#9015) ...

* Clean up LowerTEPass Add attrs to IRModule equal and hash Make LowerTEPass opt_level 0 Copy IRModule attrs to per_target_modules Add ShallowCopy to IRmodule * Fix rebase * Remove comment * [TEC] Remove memory plan from LowerTEPass * Fix linting errors * Fix PR comments * Remove updated module with function info from LowerTe * Refactor UpdateMainWorkspaceSize to update func info independently from LowerTEPass * Fix aot failed tests * Revert whitespaces fixes * Remove obsolete function hoisting and minor cleanups * Address PR comments Co-authored-by: electriclilies <lilyorthsmith@gmail.com>

electriclilies and others added 7 commits September 7, 2021 15:45

Clean up LowerTEPass

f50495b

Add attrs to IRModule equal and hash Make LowerTEPass opt_level 0 Copy IRModule attrs to per_target_modules Add ShallowCopy to IRmodule

Fix rebase

6d00a9c

Remove comment

3d2ba1b

[TEC] Remove memory plan from LowerTEPass

ee52039

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

c68f446

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

48cb35d

Fix linting errors

8a0a3c9

mikepapadim requested review from anijain2305, areusch, comaniac, jroesch, junrushao, jwfromm, MarisaKirisame, mbrookhart, merrymercy, slyubomirsky, tqchen, vinx13, wweic, yzhliu, zhiics, ZihengJiang and a team as code owners September 9, 2021 08:05

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

074ed40

electriclilies approved these changes Sep 9, 2021

View reviewed changes

mikepapadim added 4 commits September 10, 2021 11:49

Fix PR comments

9733283

Remove updated module with function info from LowerTe

0a1beb6

Refactor UpdateMainWorkspaceSize to update func info independently fr…

03a208c

…om LowerTEPass

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

5b3fb11

mikepapadim force-pushed the tec_vm branch from a04159c to b965443 Compare September 10, 2021 12:41

electriclilies mentioned this pull request Sep 10, 2021

Clean up IRModule attrs and LowerTEPass #8914

Merged

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

66168f7

mikepapadim requested a review from leandron as a code owner September 13, 2021 11:15

Mousius requested changes Sep 14, 2021

View reviewed changes

mbs-octoml reviewed Sep 14, 2021

View reviewed changes

electriclilies approved these changes Sep 14, 2021

View reviewed changes

mikepapadim changed the title ~~Remove memory planing from LowerTEPass~~ [Relay] Remove memory planing from LowerTEPass Sep 15, 2021

mikepapadim added 4 commits September 15, 2021 10:56

Merge branch 'main' of https://github.com/apache/tvm into tec_vm

8dfec81

Revert whitespaces fixes

c4cc22d

Remove obsolete function hoisting and minor cleanups

ed2c1cc

Address PR comments

ce9fbdf

Mousius requested changes Sep 15, 2021

View reviewed changes

jroesch approved these changes Sep 15, 2021

View reviewed changes

jroesch merged commit 98ecefb into apache:main Sep 15, 2021

Mousius mentioned this pull request Sep 28, 2021

[CI] Pass failFast flag to Jenkins parallel #9129

Closed

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Relay] Remove memory planing from LowerTEPass #8974

[Relay] Remove memory planing from LowerTEPass #8974

mikepapadim commented Sep 9, 2021 •

edited

Loading

electriclilies left a comment

electriclilies Sep 9, 2021

mikepapadim Sep 10, 2021

mbs-octoml commented Sep 10, 2021

masahi commented Sep 13, 2021

mikepapadim commented Sep 13, 2021

masahi commented Sep 13, 2021 •

edited

Loading

Mousius left a comment

mbs-octoml Sep 14, 2021

mbs-octoml Sep 14, 2021

mbs-octoml commented Sep 14, 2021 •

edited

Loading

mbs-octoml commented Sep 14, 2021

electriclilies left a comment •

edited

Loading

electriclilies Sep 14, 2021

electriclilies Sep 14, 2021

electriclilies Sep 14, 2021

Mousius commented Sep 14, 2021

mikepapadim commented Sep 15, 2021

Mousius left a comment

Mousius Sep 15, 2021

Mousius Sep 15, 2021

jroesch commented Sep 15, 2021

		Optional<backend::FunctionInfo> main_func_info =
		lowered_mod->GetAttr<backend::FunctionInfo>("main_func_info");

[Relay] Remove memory planing from LowerTEPass #8974

[Relay] Remove memory planing from LowerTEPass #8974

Conversation

mikepapadim commented Sep 9, 2021 • edited Loading

electriclilies left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbs-octoml commented Sep 10, 2021

masahi commented Sep 13, 2021

mikepapadim commented Sep 13, 2021

masahi commented Sep 13, 2021 • edited Loading

Mousius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mbs-octoml commented Sep 14, 2021 • edited Loading

mbs-octoml commented Sep 14, 2021

electriclilies left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Mousius commented Sep 14, 2021

mikepapadim commented Sep 15, 2021

Mousius left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jroesch commented Sep 15, 2021

mikepapadim commented Sep 9, 2021 •

edited

Loading

masahi commented Sep 13, 2021 •

edited

Loading

mbs-octoml commented Sep 14, 2021 •

edited

Loading

electriclilies left a comment •

edited

Loading