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/01/28 23:36:51 UTC
[GitHub] [tvm] mbrookhart opened a new pull request #7368: Refactor Dynamic to Static
mbrookhart opened a new pull request #7368:
URL: https://github.com/apache/tvm/pull/7368
I recently spent a lot of time fighting dynamic rank issues in a kind of crazy ONNX model. Fixing it required doing incremental dynamic-to-static before type inference. This PR basically changes the logic of dynamic to static from this:
```
infer type on the whole function
constant fold the whole function
then go find dynamic ops who's inputs are constant
replace them with static ops
Rinse and repeat
```
to this:
```
go find dynamic ops
infer type and constant fold their inputs
If the inputs are now constant, replace them with static ops
```
This has the advantage that it can analyze those crazy dynamic rank and control flow graphs and simplify them, but it has the disadvantage that it's slower than the previous version because we call infertype and constant folding many more times.
Performance checking shows that this takes a BERT compile from ~15 seconds to ~60 seconds. This should be fixable when incremental type inference becomes available.
Thanks,
Matthew
cc @masahi @jroesch @tmoreau89 @jwfromm @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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [tvm] masahi merged pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7368:
URL: https://github.com/apache/tvm/pull/7368
----------------------------------------------------------------
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] masahi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-771243476
thanks @mbrookhart
----------------------------------------------------------------
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] masahi merged pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi merged pull request #7368:
URL: https://github.com/apache/tvm/pull/7368
----------------------------------------------------------------
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] masahi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769520559
Since you always run `PrepareArgs` when you find a dynamic op, I'd run `PrepareArgs` here https://github.com/apache/tvm/blob/384714b58ed374cb1e385142b5dc4128041c945c/src/relay/transforms/dynamic_to_static.cc#L209
----------------------------------------------------------------
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] masahi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-771243476
thanks @mbrookhart
----------------------------------------------------------------
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] masahi commented on a change in pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on a change in pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#discussion_r566530421
##########
File path: src/relay/transforms/dynamic_to_static.cc
##########
@@ -32,29 +32,57 @@
namespace tvm {
namespace relay {
+Expr PrepareInput(const Expr& expr) {
+ // TODO(mbrookhart): Rewrite this to use increment type inference
+ // when that feature is available
+ auto mod = IRModule::FromExpr(expr);
+ // Perform FoldConstant->InferType twice due to nested control
+ // flow/dynamic rank issues in certain object models
+ mod = transform::FoldConstant()(mod);
+ mod = transform::InferType()(mod);
+ mod = transform::FoldConstant()(mod);
+ mod = transform::InferType()(mod);
+ if (expr.as<FunctionNode>()) {
+ return mod->Lookup("main");
+ } else {
+ return mod->Lookup("main").as<FunctionNode>()->body;
+ }
+}
+
+std::vector<Expr> PrepareArgs(const CallNode* call_node) {
+ std::vector<Expr> args;
+ for (auto arg : call_node->args) {
+ args.emplace_back(PrepareInput(arg));
Review comment:
maybe skip `PrepareInput` if the arg is already a constant?
----------------------------------------------------------------
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] mbrookhart commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769910694
@t-vi I feel your pain, a few people in OctoML are looking at a possible rewrite of the type inferencer in the coming months to fix some of these issues.
----------------------------------------------------------------
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] t-vi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
t-vi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769681858
> cc @t-vi who might be interested in incremental type inference
He, yeah, I had hoped you fixed it. :wink: I think @jroesch had looked into it more than I did at some point (in the context of #6274).
My impression is that part of the difficulty is that in-place graph operations are not a good fit with how things work in TVM in general and the frequent copying we do removes the type info. If memory serves me well, this was the main reason for doing the incremental type inference "manually" in the PyTorch frontend.
----------------------------------------------------------------
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] mbrookhart commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769918656
> Can we assume that compile time regression is the worst for BERT? I don't recall infer type or fold constant being slow on other models.
The worst model I've seen with this pass is ONNX SSD-Mobilenet, which takes about 3 minutes and prompted all of the dynamic rank fixes.
----------------------------------------------------------------
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] mbrookhart commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
mbrookhart commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769910029
> Since you always run `PrepareArgs` when you find a dynamic op, I'd run `PrepareArgs` here
>
> https://github.com/apache/tvm/blob/384714b58ed374cb1e385142b5dc4128041c945c/src/relay/transforms/dynamic_to_static.cc#L209
I tried this early on, unfortunately, PrepareArgs ends up making a copy of the IR to do type inference, and then we end up with two different versions of input variables depending on whether or not the op that uses them has a dynamic op before it or not, this breaks several unit tests.
To fix it, I would need to do infer_type/constant folding on every op during traversal, but without incremental type inference, that's impossibly slow. This is a middle ground that fixes the problem without too much of a performance hit.
----------------------------------------------------------------
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] masahi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769521367
Can we assume that compile time regression is the worst for BERT? I don't recall infer type or fold constant being slow on other models.
----------------------------------------------------------------
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] masahi commented on pull request #7368: Refactor Dynamic to Static
Posted by GitBox <gi...@apache.org>.
masahi commented on pull request #7368:
URL: https://github.com/apache/tvm/pull/7368#issuecomment-769521672
cc @t-vi who might be interested in incremental type inference
----------------------------------------------------------------
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