You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@tvm.apache.org by GitBox <gi...@apache.org> on 2021/09/02 23:01:31 UTC
[GitHub] [tvm] electriclilies opened a new pull request #8914: [DRAFT] Cleanup LowerTE + surrounding machinery more
electriclilies opened a new pull request #8914:
URL: https://github.com/apache/tvm/pull/8914
In this PR, I add `InferType` to the end of `LowerTEPass` so we don't have to call it after we call `LowerTEPass`. I also remove `per_target_modules_` from the interpreter.
To do this, I did have to fix a few bugs, namely:
- Make sure we copy the attributes of the `IRModule` when we soft-copy the `IRModule`
- In the type inferencer, we lookup `GlobalVars` by name, not by pointer address
- In the type inferencer, if we encounter a `tir::PrimFunc`, we the function it instead of failing (this allows us to call InferType on the unified module which contains both `relay::Functions` and `tir::PrimFuncs`)
This is forked from #8886, so there are also extraneous changes in here. They will go away after that is merged and I rebase.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r702368083
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
After having checked :
https://github.com/apache/tvm/blob/9f52e7e3fcb3e607e51b584437857a118fb74609/src/relay/backend/te_compiler.cc#L890-L906
I dont think it is transferred.
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
I think this is a very important change and should be called out in the PR title or the main description.
What happens to these when they are split to Map<Target, IRModule> in lowered_funcs or per_target_module in the intepretter ? Are they copied in ?
Will it be possible to add a test to make sure attrs are passed onto TIR lowering ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-912759031
@Mousius @mbs-octoml @michalpiszczek @leandron @manupa-arm Could you take a look? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r706356687
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
That convention sounds good to me, but one thing we should be cautious of is that the attributes of functions do contain properties of the compilation flow (like targets), and this is inconsistent.
@mbs-octoml By lining up the IRModule and runtime::Module worlds do you mean also moving to a world where runtime::Modules are cross-target? I'm honestly not sure if that's something we want to do. @areusch do you have any thoughts about this?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703364877
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Its not very clear to me how we can avoid per_target_module throughout the lowering process -- we could push it way down.
Unified IRModule --> per_target_modules (lowered_funcs) --> runtime.Modules
Unless Im missing something here, there will always be a stage that IRModule contains things that gets lowered to single runtime.Module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703725048
##########
File path: src/relay/ir/transform.cc
##########
@@ -133,9 +133,8 @@ IRModule FunctionPassNode::operator()(IRModule mod, const PassContext& pass_ctx)
DLOG(INFO) << "Executing function pass : " << pass_info->name
<< " with opt level: " << pass_info->opt_level;
- // Execute the pass function and return a new module.
IRModule updated_mod =
- IRModule(mod->functions, mod->type_definitions, mod->Imports(), mod->source_map);
+ IRModule(mod->functions, mod->type_definitions, mod->Imports(), mod->source_map, mod->attrs);
Review comment:
I was thinking about something like that, but unfortunately the copy constructor will only increment refs on the shared pointers of the IRModule fields, so it won't work.
I can make a helper function that returns `IRModule(mod->functions, ..., mod->attrs)`, though.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703748763
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Thanks @electriclilies .
In the absense of lowered_funcs, will there be a stage that does
'IRModule where each function is annotated with its target' --> Array<runtime.Module> then ?
In other words, build API traversing IRModule and emit runtime.Module s in a single shot ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-916995654
Hi Lily,
After addressing those comments, could you update the description on the PR as to what it should say in the commit message ?
I ll copy that and when I do the merge.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703771030
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
OK, I can add them to the per target IRModules and the we can figure out what to do with it later.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-916983696
@manupa-arm can you merge? #8974 is waiting on this PR! Thanks
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703755898
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Most likely, although I have not dug into that part of the codebase in depth yet so I can't say for sure.
The two options that I think are most likely are
1. `build` consuming the IRModule directly, traversing the functions in the IRModule and dealing with each directly (which is what you just mentioned)
2. Right before `build` is invoked, separating the functions in the module by Target (although we wouldn't store them in a `Map<Target, IRModule>`)
So to summarize, we'll either completely remove the data structure that stores functions separated by target, or just push it all the way down to right before `build` is called.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] mbs-octoml commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
mbs-octoml commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r706195612
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
+1 to:
- preserving module attrs through all passes on the assumption they apply to all targets
- xfering them when we project a cross-target IRModule to a specific-target IRModule just before the transition into lowering phases
We should probably have a convention that IRModule attrs should be for describing properties of the code and not of the compilation? Since they will appear everywhere it would be tempting to start adding Target-like things there.
Does anyone have thoughts on whether we should try to line up the IRModule and runtime::Module worlds?
```
cross-target single-target
IR IRModule ?
Executable ? runtime::Module
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-915862469
@Mousius PTAL when you have some time?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703335133
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Hi @manupa-arm, `attrs` were actually added to `IRModule` in https://github.com/apache/tvm/pull/8750 so anything we do now is an iterative improvement on that. Given this series of PRs aims to remove the `Map<Target, IRModule>` entirely (this one removes the `per_target_module` from the interpreter) I don't think we need to ensure the copy happens here but I agree we should have some test coverage when the unified `IRModule` is lowered to ensure it contains all the attributes we've accrued - this should be a follow up when we change the interface from `Map<Target, IRModule>` to `IRModule` - does that sound reasonable to you?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm edited a comment on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm edited a comment on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-915862469
@Mousius PTAL when you have some time? and explicitly approve, possibly
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703744467
##########
File path: src/ir/module.cc
##########
@@ -43,7 +43,8 @@ namespace tvm {
IRModule::IRModule(tvm::Map<GlobalVar, BaseFunc> functions,
tvm::Map<GlobalTypeVar, TypeData> type_definitions,
- std::unordered_set<String> import_set, parser::SourceMap source_map) {
+ std::unordered_set<String> import_set, parser::SourceMap source_map,
+ DictAttrs attrs) {
Review comment:
Yes, I'll do this. `FunctionNode` has `attrs` in the hash and equal functions, so we should be consistent with that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #8914: Clean up IRModule attrs and LowerTEPass
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-917195182
@manupa-arm I changed the title and responded to the comment! Ready to merge
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703742150
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
@manupa-arm I've been pushing the `Map<Target, IRModule>` further and further down the stack, with the goal of eventually removing it. Eventually, I'd like all the APIs to just consume a IRModule where each function is annotated with its target.
Right now, IRModule's attributes contains all the information needed to construct a LoweredOutput, and I have some conversion functions to extract that info before I stick it into the LoweredOutput.
You're correct that at this point I'm not copying the attributes into the `per_target_modules` IRModules. I think that this is OK for now because `per_target_modules` only exists inside a `LoweredOutput`, and the consumers of `LoweredOutput` don't expect there to be attributes on the `per_target_modules`. Additionally, I'm going removing `LoweredOutput` (and `lowered_funcs`) during more follow up work.
Unfortunately,`LoweredOutput` is deeply intertwined with the `build` API, so removing `LoweredOutput`is not a change I can make here.
My next step will be refactoring the `build` API with the goal of removing `LoweredOutput`. There's also a bunch of other cleanup that needs to be done on the `build` API that will be extensive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #8914: Clean up IRModule attrs and LowerTEPass
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #8914:
URL: https://github.com/apache/tvm/pull/8914
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703762775
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
So the attrs of IRModule are globally valid to all targets. There I feel conveying them to the codegen might still be beneficial.
For 1, we would need to expand the build API to pass the attrs
For 2, we can embed them to each IRModule.
The attrs allows a channel that pass through all of the unified lowering process up until the concept of IRModule cease to exist.
If you agree, I feel we should pass them in to the per target IRModule, then later changed in either way we decide to proceed.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] Mousius commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
Mousius commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r702331382
##########
File path: src/relay/backend/te_compiler.cc
##########
@@ -918,33 +918,15 @@ Map<Target, IRModule> GetPerTargetModules(IRModule mod) {
return per_target_modules;
}
-IRModule GetMainModule(IRModule mod) {
- IRModule main_module;
- // Copy the type defs
- for (const auto& kv : mod->type_definitions) {
- main_module->AddTypeDef(kv.first, kv.second);
- }
- // Copy all Relay functions (we don't include PrimFuncs)
- for (auto kv : mod->functions) {
- const GlobalVar& var = kv.first;
- const BaseFunc& func = kv.second;
- if (func->IsInstance<tvm::relay::FunctionNode>()) {
- main_module->Add(var, func);
- }
- }
- return main_module;
-}
-
Pass LowerTEPass(TargetMap targets, DeviceMap device_context_map,
backend::StaticMemoryPlan memory_plan, const String& module_name,
std::function<void(Function)> process_fn) {
runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> pass_func = [=](IRModule module,
PassContext ctx) {
return LowerTE(module, targets, device_context_map, memory_plan, module_name, process_fn);
};
- // TODO(@electriclilies, mbs): Fold InferType() pass into LowerTEPass since it will always need to
- // be called afterwards
- return tvm::transform::CreateModulePass(pass_func, 1, "LowerTE", {});
+ return tvm::transform::Sequential(
+ {tvm::transform::CreateModulePass(pass_func, 1, "LowerTE", {}), InferType()});
Review comment:
`opt_level` should be `0` for `LowerTE` so it's always ran - it wasn't checked before this change but `Sequential` checks it when it decides whether to run the pass or not (that's why you're seeing weird graph executor codegen failures as the test runs with `opt_level=0`)
As a side note, we should keep the test to ensure things compile successfully at `opt_level=0` even after the compile engine stuff is removed :smile_cat:
##########
File path: src/relay/ir/transform.cc
##########
@@ -133,9 +133,8 @@ IRModule FunctionPassNode::operator()(IRModule mod, const PassContext& pass_ctx)
DLOG(INFO) << "Executing function pass : " << pass_info->name
<< " with opt level: " << pass_info->opt_level;
- // Execute the pass function and return a new module.
IRModule updated_mod =
- IRModule(mod->functions, mod->type_definitions, mod->Imports(), mod->source_map);
+ IRModule(mod->functions, mod->type_definitions, mod->Imports(), mod->source_map, mod->attrs);
Review comment:
Is this getting to the point where a copy constructor would be more appropriate so it'd just be `IRModule(mod)` rather than having to replicate the arguments?
##########
File path: src/ir/module.cc
##########
@@ -43,7 +43,8 @@ namespace tvm {
IRModule::IRModule(tvm::Map<GlobalVar, BaseFunc> functions,
tvm::Map<GlobalTypeVar, TypeData> type_definitions,
- std::unordered_set<String> import_set, parser::SourceMap source_map) {
+ std::unordered_set<String> import_set, parser::SourceMap source_map,
+ DictAttrs attrs) {
Review comment:
Should we add `attrs` to `SEqualReduce` (`return equal(attrs, other->attrs);`) and `SHashReduce` (`hash_reduce(attrs)`) ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703725918
##########
File path: src/relay/backend/te_compiler.cc
##########
@@ -918,33 +918,15 @@ Map<Target, IRModule> GetPerTargetModules(IRModule mod) {
return per_target_modules;
}
-IRModule GetMainModule(IRModule mod) {
- IRModule main_module;
- // Copy the type defs
- for (const auto& kv : mod->type_definitions) {
- main_module->AddTypeDef(kv.first, kv.second);
- }
- // Copy all Relay functions (we don't include PrimFuncs)
- for (auto kv : mod->functions) {
- const GlobalVar& var = kv.first;
- const BaseFunc& func = kv.second;
- if (func->IsInstance<tvm::relay::FunctionNode>()) {
- main_module->Add(var, func);
- }
- }
- return main_module;
-}
-
Pass LowerTEPass(TargetMap targets, DeviceMap device_context_map,
backend::StaticMemoryPlan memory_plan, const String& module_name,
std::function<void(Function)> process_fn) {
runtime::TypedPackedFunc<IRModule(IRModule, PassContext)> pass_func = [=](IRModule module,
PassContext ctx) {
return LowerTE(module, targets, device_context_map, memory_plan, module_name, process_fn);
};
- // TODO(@electriclilies, mbs): Fold InferType() pass into LowerTEPass since it will always need to
- // be called afterwards
- return tvm::transform::CreateModulePass(pass_func, 1, "LowerTE", {});
+ return tvm::transform::Sequential(
+ {tvm::transform::CreateModulePass(pass_func, 1, "LowerTE", {}), InferType()});
Review comment:
Cool, thanks for catching this! I'll add a note to that test about not removing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703345519
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Would there be a PR to remove lowered_funcs from the TE compiler as well ? @electriclilies
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703345519
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Would there be a PR to remove lowered_funcs as well ? @electriclilies
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies edited a comment on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies edited a comment on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-916983696
@manupa-arm can you merge? #8974 is waiting on this PR! Thanks
(Actually, nevermind, I missed a few new comments)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi commented on pull request #8914: Clean up IRModule attrs and LowerTEPass
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-918033965
Thanks @electriclilies @manupa-arm @Mousius @mbs-octoml
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703742150
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
@manupa-arm I've been pushing the `Map<Target, IRModule>` further and further down the stack, with the goal of eventually removing it. Eventually, I'd like all the APIs to just consume a IRModule where each function is annotated with its target.
Right now, IRModule's attributes contains all the information needed to construct a LoweredOutput, and I have some conversion functions to extract that info before I stick it into the LoweredOutput.
You're correct that at this point I'm not copying the attributes into the `per_target_modules` IRModules. I think that this is OK for now because `per_target_modules` only exists inside a `LoweredOutput`, and the consumers of `LoweredOutput` don't expect there to be attributes on the `per_target_modules`.
Additionally, I'm going to remove `LoweredOutput` (and `lowered_funcs`) during more follow up work.
Unfortunately,`LoweredOutput` is deeply intertwined with the `build` API, so removing `LoweredOutput` is not a change I can make here.
My next step will be refactoring the `build` API with the goal of removing `LoweredOutput`. There's also a bunch of other cleanup that needs to be done on the `build` API that will be extensive.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies edited a comment on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies edited a comment on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-912759031
@Mousius @mbs-octoml @mikepapadim @leandron @manupa-arm Could you take a look? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] manupa-arm commented on a change in pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
manupa-arm commented on a change in pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#discussion_r703364877
##########
File path: include/tvm/ir/module.h
##########
@@ -122,6 +122,7 @@ class IRModuleNode : public Object {
v->Visit("global_var_map_", &global_var_map_);
v->Visit("global_type_var_map_", &global_type_var_map_);
v->Visit("source_map", &source_map);
+ v->Visit("attrs", &attrs);
Review comment:
Its not very clear to me how we can avoid per_target_module throughout the lowering process -- we could push it way down.
Unified IRModule --> per_target_modules (lowered_funcs) --> runtime.Modules
Unless Im missing something here, there will always be a stage that IRModule contains things that gets lowered to a single runtime.Module.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] electriclilies commented on pull request #8914: Run InferType in LowerTEPass and remove per_target_modules_ from the Interpreter
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #8914:
URL: https://github.com/apache/tvm/pull/8914#issuecomment-915426069
@manupa-arm @Mousius This is green, can you take another look and approve? Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@tvm.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org