-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Clean up IRModule attrs and LowerTEPass #8914
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should we add There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, I'll do this. |
||
auto n = make_object<IRModuleNode>(); | ||
n->functions = std::move(functions); | ||
n->type_definitions = std::move(type_definitions); | ||
|
@@ -52,6 +53,7 @@ IRModule::IRModule(tvm::Map<GlobalVar, BaseFunc> functions, | |
n->constructor_tag_map_ = {}; | ||
n->import_set_ = std::move(import_set); | ||
n->source_map = source_map; | ||
n->attrs = std::move(attrs); | ||
|
||
for (const auto& kv : n->functions) { | ||
// set global var map | ||
|
@@ -72,6 +74,7 @@ IRModule::IRModule(tvm::Map<GlobalVar, BaseFunc> functions, | |
|
||
bool IRModuleNode::SEqualReduce(const IRModuleNode* other, SEqualReducer equal) const { | ||
if (functions.size() != other->functions.size()) return false; | ||
if (!equal(this->attrs, other->attrs)) return false; | ||
for (const auto& kv : this->functions) { | ||
if (!other->ContainGlobalVar(kv.first->name_hint)) return false; | ||
if (!equal(kv.second, other->Lookup(kv.first->name_hint))) return false; | ||
|
@@ -112,6 +115,7 @@ void IRModuleNode::SHashReduce(SHashReducer hash_reduce) const { | |
temp.emplace_back(kv.first->name_hint, kv.second); | ||
} | ||
reduce_temp(); | ||
hash_reduce(this->attrs); | ||
} | ||
|
||
bool IRModuleNode::ContainGlobalVar(const String& name) const { | ||
|
@@ -361,6 +365,11 @@ void IRModuleNode::Update(const IRModule& mod) { | |
} | ||
} | ||
|
||
IRModule IRModuleNode::ShallowCopy() { | ||
return IRModule(this->functions, this->type_definitions, this->Imports(), this->source_map, | ||
this->attrs); | ||
} | ||
|
||
std::pair<IRModule, GlobalVar> IRModule::FromExprInContext( | ||
const RelayExpr& expr, const tvm::Map<GlobalVar, BaseFunc>& global_funcs, | ||
const tvm::Map<GlobalTypeVar, TypeData>& type_definitions, | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
After having checked :
tvm/src/relay/backend/te_compiler.cc
Lines 890 to 906 in 9f52e7e
I dont think it is transferred.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @manupa-arm,
attrs
were actually added toIRModule
in #8750 so anything we do now is an iterative improvement on that. Given this series of PRs aims to remove theMap<Target, IRModule>
entirely (this one removes theper_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 unifiedIRModule
is lowered to ensure it contains all the attributes we've accrued - this should be a follow up when we change the interface fromMap<Target, IRModule>
toIRModule
- does that sound reasonable to you?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would there be a PR to remove lowered_funcs as well ? @electriclilies
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
build
consuming the IRModule directly, traversing the functions in the IRModule and dealing with each directly (which is what you just mentioned)build
is invoked, separating the functions in the module by Target (although we wouldn't store them in aMap<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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I can add them to the per target IRModules and the we can figure out what to do with it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to:
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?