Skip to content
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

Merged
merged 5 commits into from
Sep 6, 2019
Merged

Conversation

yzhliu
Copy link
Member

@yzhliu yzhliu commented Aug 27, 2019

@yzhliu
Copy link
Member Author

yzhliu commented Aug 29, 2019

@tqchen I added arg bind support for the case. please review again.

@@ -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) {
Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

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?

Copy link
Member

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverted

@tqchen tqchen merged commit 9b148f1 into apache:master Sep 6, 2019
@tqchen
Copy link
Member

tqchen commented Sep 6, 2019

Thanks @yzhliu

MarisaKirisame pushed a commit to MarisaKirisame/tvm that referenced this pull request Sep 7, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
wweic pushed a commit to wweic/tvm that referenced this pull request Sep 16, 2019
wweic pushed a commit to neo-ai/tvm that referenced this pull request Sep 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants