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/12/09 01:29:17 UTC
[GitHub] [tvm] electriclilies opened a new pull request #9690: Change function constructors to WithFields
electriclilies opened a new pull request #9690:
URL: https://github.com/apache/tvm/pull/9690
In this PR, I change function constructors to WithFields. I tried to change all of them but may have missed a few.
--
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 #9690: Change function constructors to WithFields
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #9690:
URL: https://github.com/apache/tvm/pull/9690#issuecomment-989394742
i split these out from another branch, there are still some residual changes from that branch that I need to revert.
--
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 #9690: Change function constructors to WithFields
Posted by GitBox <gi...@apache.org>.
electriclilies commented on a change in pull request #9690:
URL: https://github.com/apache/tvm/pull/9690#discussion_r784439587
##########
File path: src/relay/transforms/inline.cc
##########
@@ -131,7 +130,9 @@ class Inliner : ExprMutator {
const auto* fn = base_func.as<FunctionNode>();
ICHECK(fn) << "Expected to work on a Relay function.";
- auto func = Function(fn->params, fn->body, fn->ret_type, fn->type_params, fn->attrs);
+ // TODO(@electriclilies): If Function is a COW node, then if it gets written to we shouldn't
+ // have any sharing, right? So we don't need to reconstruct?
+ Function func = WithFields(GetRef<Function>(fn));
Review comment:
Done!
##########
File path: src/relay/transforms/to_cps.cc
##########
@@ -311,15 +313,17 @@ Function ToCPS(const Function& f, const IRModule& m) {
Function UnCPS(const Function& f) {
CheckFeature(f, FeatureSet::All() - fGraph);
ICHECK_GT(f->params.size(), 0);
- std::vector<Var> new_params;
+ Array<Var> new_params;
for (const auto& p : f->params) {
- new_params.push_back(Var(p->name_hint(), p->checked_type()));
+ // TODO(@electriclilies): Not sure if this is correct, it was copying before,
+ // but seems like we just need to make a copy to pop so should be fine?
+ new_params.push_back(WithFields(std::move(p)));
Review comment:
Done, 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] jroesch merged pull request #9690: Change function constructors to WithFields
Posted by GitBox <gi...@apache.org>.
jroesch merged pull request #9690:
URL: https://github.com/apache/tvm/pull/9690
--
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 #9690: Change function constructors to WithFields
Posted by GitBox <gi...@apache.org>.
mbs-octoml commented on a change in pull request #9690:
URL: https://github.com/apache/tvm/pull/9690#discussion_r783406273
##########
File path: src/relay/transforms/to_cps.cc
##########
@@ -311,15 +313,17 @@ Function ToCPS(const Function& f, const IRModule& m) {
Function UnCPS(const Function& f) {
CheckFeature(f, FeatureSet::All() - fGraph);
ICHECK_GT(f->params.size(), 0);
- std::vector<Var> new_params;
+ Array<Var> new_params;
for (const auto& p : f->params) {
- new_params.push_back(Var(p->name_hint(), p->checked_type()));
+ // TODO(@electriclilies): Not sure if this is correct, it was copying before,
+ // but seems like we just need to make a copy to pop so should be fine?
+ new_params.push_back(WithFields(std::move(p)));
Review comment:
I'd retain the copies here -- it is building a new function with new parameters.
I think including all args including the span in the ctor will ensure any new fields will at least trigger a compilation error so these few examples of explicit shallow copies can be maintained. Adding a generic ShallowCopy or Clone helper seems overkill.
##########
File path: src/relay/transforms/inline.cc
##########
@@ -131,7 +130,9 @@ class Inliner : ExprMutator {
const auto* fn = base_func.as<FunctionNode>();
ICHECK(fn) << "Expected to work on a Relay function.";
- auto func = Function(fn->params, fn->body, fn->ret_type, fn->type_params, fn->attrs);
+ // TODO(@electriclilies): If Function is a COW node, then if it gets written to we shouldn't
+ // have any sharing, right? So we don't need to reconstruct?
+ Function func = WithFields(GetRef<Function>(fn));
Review comment:
It looks like the common case is for the function body to be inlined as you'd expect, but it is also possible for the function itself to be directly inlined (as a call or gv). Despite doing a shallow copy of the function here, there is no deep copy of the function body when it is inlined directly. So the code is inconsistent in it's handling of sharing. Nevertheless I'd suggest retaining the shallow copy of the function with direct use of the ctor.
Could you leave a comment pointing out the inconsistency (function shallow copied, but body not shallow copied)? Thx.
--
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 #9690: Change function constructors to WithFields
Posted by GitBox <gi...@apache.org>.
electriclilies commented on pull request #9690:
URL: https://github.com/apache/tvm/pull/9690#issuecomment-1005260321
This is ready for review, please take a look @mbs-octoml @mikepapadim.
Note that I rebased against #9826, so you'll see some changes from that in the diff until it is merged
--
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