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/04/29 14:16:19 UTC
[GitHub] [tvm] tqchen opened a new pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
tqchen opened a new pull request #7945:
URL: https://github.com/apache/tvm/pull/7945
Previously we are generating the function calls for reshape.
This PR updates the optimization to turn reshape into nop:
- Tag a fused function as reshape only if it only contains reshape.
- Update memory planner to force input output to share the same piece of memory
- Update the graph runtime codegen to emit nop when reshape only function is encountered.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829275660
cc @altanh @jroesch @zhiics @icemelon9 @Meteorix @ZihengJiang
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829551076
@csullivan to followup on your points. I agree that a decision about output aliasing can be a big undertaking to mark in the IR itself. The way that the current PR structured it is as follows:
- K0 The reshape only property is a property with respect to the function itself, without breaching into the property of the phyiscal execution
- K1 The graph execution generator takes that property to generate eliding.
Note that majority part of the property marking in K0 more strict than C1, due to the reason that aliasing info breaches the semantics while K0 is considered as an annotation of the function property, the backend can still choose to implement reshape only function by actually running the code or alias them together.
K1 have to happen at some time point, and can be both target and operation dependent. Given that right now the memory planning and executions are coupled at the graph runtime codegen, the current change to the graph runtime is consistent with that choice.
As we start to factor out more of memory alloca and execution into seperate passes, I agree that introducing physical specific annotations would then make sense since the decisions will then be passed across a few stages.
The target dependence of K1 is indeed something worth thinking about. Given that the current graph planner already assumes flat memory, my assumption is that a separate planning and execution generator will be needed for the N-D case, where we can skip this choice for now(note that the reshape only is a property tag rather than a tag that enforces the execution).
Back to the general topic of aliasing memory. I think that problem would be a bigger decision indeeds deserves an RFC and more in-depth thoughts(I don't think marking aliasing in the graph directly is the right approach as you said it changes the semantics of the graph in an early stage). This PR is mainly intended as a incremental improvement to the graph executor that is handle the case of reshape in a consistent way as VM in the case of flat memory scenario.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829551076
@csullivan to followup on your points. I agree that a decision about output aliasing can be a big undertaking to mark in the IR itself. The rationale that brings the current PR os structured as follows:
- K0 The reshape only property is a property with respect to the function itself, without breaching into the property of the phyiscal execution
- K1 The graph execution generator takes that property to generate eliding.
Note that majority part of the property marking in K0 more strict than C1, due to the reason that aliasing info breaches the semantics while K0 is considered as an annotation of the function property, the backend can still choose to implement reshape only function by actually running the code or eliding and nop. The annotation K0 does not imply the decision.
K1 have to happen at some time point, and can be both target and operation dependent. Given that right now the memory planning and executions are coupled at the graph runtime codegen, the current change to the graph runtime is consistent with that choice.
As we start to factor out more of memory alloca and execution into seperate passes, I agree that introducing physical specific annotations would then make sense since the decisions will then be passed across a few stages.
The target dependence of K1 is indeed something worth thinking about. Given that the current graph planner already assumes flat memory, my assumption is that a separate planning and execution generator will be needed for the N-D case, where we can skip this choice for now(note that the reshape only is a property tag rather than a tag that enforces the execution).
Back to the general topic of aliasing memory. I think that problem would be a bigger decision indeeds deserves an RFC and more in-depth thoughts(I don't think marking aliasing in the graph directly is the right approach as you said it changes the semantics of the graph in an early stage). This PR is mainly intended as a incremental improvement to the graph executor that is handle the case of reshape in a consistent way as VM in the case of flat memory scenario.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on a change in pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on a change in pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#discussion_r623420830
##########
File path: src/relay/backend/graph_executor_codegen.cc
##########
@@ -380,6 +380,19 @@ class GraphExecutorCodegen : public backend::MemoizedExprTranslator<std::vector<
return GraphAddCallNode(op, ext_func->func_name, ext_func->func_name);
}
+ // In the current flat memory allocation scenario
+ // the flat memory allocator can always allocate input
+ // and output of the reshape to the same memory, we can turn reshape only
+ // function to a nop.
+ //
+ // NOTE that for non-flat memory this is not necessarily true.
+ //
+ // TODO(tvm-team) Update checks of flat memory enablement when we support
+ // opaque-nd memory planning to skip this path.
+ if (func->HasNonzeroAttr(attr::kReshapeOnly)) {
+ return GraphAddCallNode(op, "reshape_nop", "__nop");
+ }
Review comment:
My previous thoughts was nd allocation will have a separate executor generation path, agree that cross checking would be more desirable
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829611498
reshape only was a property about the function itself, the property have well-defined meaning in functional land. While alias property itself does not have a clear definition in the function land and is closer to the execution property.
So likely a reshape only tag can appear in early stage of compilation without a problem. While alias tag would likely need to only appear when we start to think about memory planning.
For general ops, having alias information alone may not give a complete picture, as the operator semantics itself is also part of the picture. For some ops, alias needs to be enforced(e.g. case of scatter sparse add to dense), while others the impact is more minimum(e.g. reshape). Reshape only in that regard is a more narrow tag that specifies the op semantics as well
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829275660
cc @altanh @jroesch @zhiics @icemelon9 @Meteorix @ZihengJiang @junrushao1994
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829551076
@csullivan to followup on your points. I agree that a decision about output aliasing can be a big undertaking to mark in the IR itself. The rationale that brings the current PR os structured as follows:
- K0 The reshape only property is a property with respect to the function itself, without breaching into the property of the phyiscal execution
- K1 The graph execution generator takes that property to generate eliding.
Note that majority part of the property marking in K0 more strict than C1, due to the reason that aliasing info breaches the semantics while K0 is considered as an annotation of the function property, the backend can still choose to implement reshape only function by actually running the code or alias them together.
K1 have to happen at some time point, and can be both target and operation dependent. Given that right now the memory planning and executions are coupled at the graph runtime codegen, the current change to the graph runtime is consistent with that choice.
As we start to factor out more of memory alloca and execution into seperate passes, I agree that introducing physical specific annotations would then make sense since the decisions will then be passed across a few stages.
The target dependence of K1 is indeed something worth thinking about. Given that the current graph planner already assumes flat memory, my assumption is that a separate planning and execution generator will be needed for the N-D case, where we can skip this choice for now(note that the reshape only is a property tag rather than a tag that enforces the execution).
Back to the general topic of aliasing memory. I think that problem would be a bigger decision indeeds deserves an RFC and more in-depth thoughts(I don't think marking aliasing in the graph directly is the right approach as you said it changes the semantics of the graph in an early stage). This PR is mainly intended as a incremental improvement to the graph executor that is handle the case of reshape in a consistent way as VM in the case of flat memory scenario.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] ZihengJiang merged pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
ZihengJiang merged pull request #7945:
URL: https://github.com/apache/tvm/pull/7945
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829426059
@icemelon9 I am not too sure the implication to the VM memalloc optimizations(as the assumption can differ), would be good to make that as a followup
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] csullivan commented on a change in pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
csullivan commented on a change in pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#discussion_r623399795
##########
File path: src/relay/backend/graph_executor_codegen.cc
##########
@@ -380,6 +380,19 @@ class GraphExecutorCodegen : public backend::MemoizedExprTranslator<std::vector<
return GraphAddCallNode(op, ext_func->func_name, ext_func->func_name);
}
+ // In the current flat memory allocation scenario
+ // the flat memory allocator can always allocate input
+ // and output of the reshape to the same memory, we can turn reshape only
+ // function to a nop.
+ //
+ // NOTE that for non-flat memory this is not necessarily true.
+ //
+ // TODO(tvm-team) Update checks of flat memory enablement when we support
+ // opaque-nd memory planning to skip this path.
+ if (func->HasNonzeroAttr(attr::kReshapeOnly)) {
+ return GraphAddCallNode(op, "reshape_nop", "__nop");
+ }
Review comment:
This remains problematic once we have memory planning for nd allocations (like #7690).
The criteria for eliding a reshape (that it is logical only) should be that it's both a reshape _and_ its input and output are aliased. My feeling is that the conditional should reflect this, with a check on both the `attr::kReshapeOnly` attribute as well as whether the outputs storage_ids are equivalent. If the latter criteria isn't met we shouldn't emit a nop.
Can you add a helper in the conditional that checks this with the `storage_device_map_`?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829551076
@csullivan to followup on your points. I agree that a decision about output aliasing can be a big undertaking to mark in the IR itself. The rationale that brings the current PR os structured as follows:
- K0 The reshape only property is a property with respect to the function itself, without breaching into the property of the phyiscal execution
- K1 The graph execution generator takes that property to generate eliding.
Note that majority part of the property marking in K0 more strict than C1, due to the reason that aliasing info breaches the semantics while K0 is considered as an annotation of the function property, the backend can still choose to implement reshape only function by actually running the code or eliding and nop. The annotation K0 does not imply the decision.
K1 have to happen at some time point, and can be both target and operation dependent. Given that right now the memory planning and executions are coupled at the graph runtime codegen, the current change to the graph runtime is consistent with that choice.
As we start to factor out more of memory alloca and execution into seperate passes, I agree that introducing physical specific annotations would then make sense since the decisions will then be passed across a few stages.
The target dependence of K1 is indeed something worth thinking about. Given that the current graph planner already assumes flat memory, my assumption is that a separate planning and execution generator will be needed for the N-D case, where we can skip this choice for now(note that the reshape only is a property tag rather than a tag that enforces the execution).
Back to the general topic of aliasing memory. I think that problem would be a bigger decision indeeds deserves an RFC and more in-depth thoughts(I don't think marking aliasing in the graph directly is the right approach as you said it changes the semantics of the graph in an early stage). This PR is mainly intended as a incremental improvement to the graph executor that is handle the case of reshape in a consistent way as VM(which already elides reshape) in the case of flat memory scenario.
We can certainly bring followup refactors once we have a good proposal to handle aliasing memory.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] altanh commented on a change in pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
altanh commented on a change in pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#discussion_r623262746
##########
File path: src/relay/transforms/fuse_ops.cc
##########
@@ -949,14 +949,29 @@ class FuseMutator : private MixedModeMutator {
Expr MakeNewFunction(GraphPartitioner::Group* group, Type ret_type, Expr body) {
// If the function has no call, it is not a primitive function.
- struct HasCallVisitor : ExprVisitor {
+ struct PropertyVisitor : ExprVisitor {
bool has_call = false;
- void VisitExpr_(const CallNode* op) final { has_call = true; }
+ bool reshape_only = true;
+ void VisitExpr_(const CallNode* op) final {
+ static const Op& reshape_op_ = Op::Get("reshape");
Review comment:
similar to my previous comment, we'd at least want to include a check for `reverse_reshape` op, but I think we should generalize and make it less ad hoc if possible
##########
File path: include/tvm/relay/function.h
##########
@@ -144,6 +144,9 @@ constexpr const char* kComposite = "Composite";
constexpr const char* kInline = "Inline";
/*! \brief Indicate the function was created by the Pattern Partitioning Pass. */
constexpr const char* kPartitionedFromPattern = "PartitionedFromPattern";
+
+/*! \brief Mark the function as only composed of reshape operations. */
+constexpr const char* kReshapeOnly = "relay.reshape_only";
Review comment:
are there other ops that could benefit? for example `squeeze`, `expand_dims`, etc.? I feel like this should more generally be like `relay.alias_only` (although alias might be too general since you could "alias" a subset of an input by slicing)
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-830442514
cc @icemelon9 @altanh please take another look
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829558024
Added comments about the flat memory assumptions and TODOs about what can be done under non-flat settings once the support is introduced
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829426059
Thanks @icemelon9 I am not too sure the implication to the VM memalloc optimizations(as the assumption can differ). Perhaps you or others can send that as a followup?
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829426059
Thanks @icemelon9 let me take a look and see what is going on there
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829611498
reshape only was a property about the function itself, the property have well-defined meaning in functional land. While alias property itself does not have a clear definition in the function land and is closer to the execution property.
So a reshape only tag can appear in early stage of compilation without a problem. While alias tag would likely need to only appear when we start to think about memory planning.
For general ops, having alias information alone may not give a complete picture, as the operator semantics itself is also part of the picture. For some ops, alias needs to be enforced(e.g. case of scatter sparse add to dense), while others the impact is more minimum(e.g. reshape). Reshape only in that regard is a more narrow tag that specifies the op semantics as well
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen edited a comment on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen edited a comment on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829426059
Thanks @icemelon9 I am not too sure the implication to the VM memalloc optimizations(as the assumption can differ), would be good to make that as a followup
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] tqchen commented on pull request #7945: [RELAY] Turn reshape into nop in graph executor backend.
Posted by GitBox <gi...@apache.org>.
tqchen commented on pull request #7945:
URL: https://github.com/apache/tvm/pull/7945#issuecomment-829532593
Thanks @icemelon9 @altanh comments are addressed by looking for a reshapeop property.
--
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org