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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
