-
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
[schedule] Improve ceil_divide in tile/split #3842
Conversation
@tqchen I added arg bind support for the case. please review again. |
src/pass/arg_binder.cc
Outdated
@@ -46,25 +47,41 @@ void BinderAddAssert(Expr cond, | |||
} | |||
} | |||
|
|||
// Deal with things like arg = 8 * M | |||
// TODO(Yizhi Liu): make it more general | |||
std::pair<Expr, Expr> TryExtractVariable(const Expr& arg, const Expr& value) { |
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.
Please comment about an example use-case.
This logic deals with an additional constraint like 8 * arg = value, and we would like to know what is the example application. Because normally, this will generate a constraint instead and the simplifier should be able to simplify the constraint. So we won't need this logic.
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.
without the fix, tvm will complain things like "TVMError: Not all Vars are passed in api_args: 'M' 'K' 'N' does not appear in api_args"
The example use-case is op computation for mxnet. To get decent performance for unknown shape (during compile time), we generate multiple kernels for one operator, and dispatch according to the shape during runtime. However, if the shape is pure symbolic, tiling/splitting has to generate if-else, which hurts the performance. One idea is to "hint" tvm, say, this shape can be divided by 8, pls don't generate if-else for tile size 2/4/8. This is where 8*M
comes from.
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 see, i think we could use better expressions for such kind of hint. Your proposal is interesting.
We could also use alternatives to resolve this problem, in particular, we can do something like
AssertExpr(x, x % 8 == 0)
. Given this change affects more of the perf, can be separate it out from this PR?
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.
do you mean remove the second commit (support arithmetic size in codegen) from this PR and merge the first one (Improve ceil_divide in tile/split) for now?
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.
Yes, we can use a second PR for that
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.
reverted
Thanks @yzhliu |
https://discuss.tvm.ai/t/codegen-codegen-with-minimum-if-condition/3838
@tqchen @icemelon9