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

[Relay] Remove memory planing from LowerTEPass #8974

Merged
merged 18 commits into from
Sep 15, 2021
Merged

Conversation

mikepapadim
Copy link
Contributor

@mikepapadim mikepapadim commented Sep 9, 2021

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

@jroesch @electriclilies @mbs-octoml

Copy link
Contributor

@electriclilies electriclilies left a comment

Choose a reason for hiding this comment

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

One small nit but otherwise LGTM! Thanks :)

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

struct EnumClassHash {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@mbs-octoml
Copy link
Contributor

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

@masahi
Copy link
Member

masahi commented Sep 13, 2021

@mikepapadim #8914 is merged.

@mikepapadim
Copy link
Contributor Author

@mikepapadim #8914 is merged.

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

@masahi
Copy link
Member

masahi commented Sep 13, 2021

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

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

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?

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

Choose a reason for hiding this comment

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

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

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

Pass LowerTensorExpr(TargetMap targets, DeviceMap device_context_map,
Copy link
Contributor

Choose a reason for hiding this comment

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

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

@mbs-octoml
Copy link
Contributor

mbs-octoml commented Sep 14, 2021

@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
Copy link
Contributor

LGTM modulo comments.

Copy link
Contributor

@electriclilies electriclilies left a comment

Choose a reason for hiding this comment

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

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!

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

Choose a reason for hiding this comment

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

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)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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
Copy link
Member

Mousius commented Sep 14, 2021

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 mikepapadim changed the title Remove memory planing from LowerTEPass [Relay] Remove memory planing from LowerTEPass Sep 15, 2021
@mikepapadim
Copy link
Contributor Author

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

Copy link
Member

@Mousius Mousius left a comment

Choose a reason for hiding this comment

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

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

Comment on lines +586 to +592
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);
}
Copy link
Member

Choose a reason for hiding this comment

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

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);
}

Comment on lines +224 to +232
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);
}

Copy link
Member

Choose a reason for hiding this comment

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

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
Copy link
Member

jroesch commented Sep 15, 2021

@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.

@jroesch jroesch merged commit 98ecefb into apache:main Sep 15, 2021
AndrewZhaoLuo added a commit to AndrewZhaoLuo/tvm that referenced this pull request Sep 16, 2021
* 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)
  ...
ylc pushed a commit to ylc/tvm that referenced this pull request Sep 29, 2021
* 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>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* 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>
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.

6 participants