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

[ARITH] Use floordiv for the deduce bound #4025

Merged
merged 1 commit into from
Oct 8, 2019

Conversation

umangyadav
Copy link
Contributor

Solves #3974

@umangyadav umangyadav changed the title Add floordiv for the deduce bound Use floordiv for in the deduce bound Sep 27, 2019
@umangyadav
Copy link
Contributor Author

@tqchen @ZihengJiang @kimishpatel @xqdan Can you review this? thanks.

@kimishpatel
Copy link
Contributor

Following a quick glance Not sure but unconditional floordiv may not be right when I put Expr cannot be simplified to constant. E.g n/3 vs floordiv(n,3). C integer div is always round towards zero.

@umangyadav
Copy link
Contributor Author

umangyadav commented Sep 27, 2019

Following a quick glance Not sure but unconditional floordiv may not be right when I put Expr cannot be simplified to constant. E.g n/3 vs floordiv(n,3). C integer div is always round towards zero.

@kimishpatel I don't completely understand your example. Can you elaborate more?

floordiv will be converted to truncdiv (C equivalent) during the lowering considering dividends and divisor's sign information in process. Is that what your concern was that floordiv is not the same as C div ?

@umangyadav
Copy link
Contributor Author

umangyadav commented Sep 27, 2019

@tqchen @kimishpatel

I am more concerned about the case where analyzer can not prove that (result_ % operand == 0) for some reason despite being divisible.

For the sake of an example, let' say
(3 * cc2 + 6) % 3 and let's say analyzer_ fails to prove it equal to 0 then bounds would come out wrong for the kGreater.

In fact, one such case where proving modulo is zero fails is here:
https://github.com/dmlc/tvm/blob/18188f4ba3f53cc1dab765b8a0d932d21db0ae8a/tests/python/unittest/test_arith_deduce_bound.py#L124

I agree with removing the tight bounds assumption and not removing the conditions directly in the loop partition now.

Use fdiv in the tests for the deduce_bound
@umangyadav umangyadav changed the title Use floordiv for in the deduce bound Use floordiv for the deduce bound Sep 30, 2019
@ZihengJiang ZihengJiang changed the title Use floordiv for the deduce bound [ARITH] Use floordiv for the deduce bound Oct 2, 2019
@umangyadav
Copy link
Contributor Author

ping @tqchen @ZihengJiang

@ZihengJiang
Copy link
Contributor

@umangyadav Hey, this PR looks good overall. Could you add the example in the issue #3974 as a test case?

@umangyadav
Copy link
Contributor Author

umangyadav commented Oct 8, 2019

@ZihengJiang I'd thought about it, but I found it not necessary, as the existing test cases (in test_arith_deduce_bound) already had one of such cases. Before, it was using relaxed bounds as the benchmark. So, it was all passing. I've modified the benchmark (expected result) to account for floor division now.

I can add the test anyway if you think it would be better or alternatively I can add a test in test_pass_loop_partition

@ZihengJiang ZihengJiang merged commit ec375a8 into apache:master Oct 8, 2019
@ZihengJiang
Copy link
Contributor

No worries. This PR has been mergred. Thanks! @umangyadav

@umangyadav umangyadav deleted the kless branch October 9, 2019 15:42
anijain2305 pushed a commit to anijain2305/tvm that referenced this pull request Oct 17, 2019
Use fdiv in the tests for the deduce_bound
wweic pushed a commit to neo-ai/tvm that referenced this pull request Oct 18, 2019
Use fdiv in the tests for the deduce_bound
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants