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

[TIR][Arith] Use TryCompare to narrow inequalities if possible #13024

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

Lunderberg
Copy link
Contributor

Prior to this commit, the result of TryCompare would only be used if it could definitively prove a conditional to be either true or false. For example, if it is known that 0 <= i, a conditional of i <= 0 would be left as-is.

This commit introduces rewrite rules to preferentially simplify into more restrictive conditions. Using the same example, if it is known that 0 <= i, a conditional of i <= 0 would be simplified into i == 0. Similarly, if it is known that 0 <= i, a conditional of i != 0 would be simplified into 0 < i.

Because this change does not introduce significant overhead, as the results of RewriteSimplifier::Impl::TryCompare are already available, this change is enabled for all use cases and does not require a call to RewriteSimplifier::SetEnabledExtensions.

@Lunderberg
Copy link
Contributor Author

This is part of a series of improved simplifications, as part of #12261. Currently marked as draft PR, as it depends on #13023.

@Lunderberg
Copy link
Contributor Author

Last failing unit test requires #13081 to resolve. An unrolled loop produced a T.if_then_else(floormod(-i,2)==0, ...), which resulted in an error thrown from ModularSet. Previously, this was T.if_then_else(floormod(-i,2) <= 0, ...), which was ignored by the ModularSet. Because floormod with a positive denominator is never negative, this could be simplified to T.if_then_else(floormod(-i,2) == 0, ...), triggering that bug.

@Lunderberg Lunderberg marked this pull request as ready for review October 14, 2022 22:01
@areusch areusch added needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it and removed needs-triage PRs or issues that need to be investigated by maintainers to find the right assignees to address it labels Oct 19, 2022
@Lunderberg
Copy link
Contributor Author

Rebased onto main to resolve conflicts. Because this dev branch includes and depends on the changes made in #13023, that PR should be landed before this one. Therefore, marking this PR as a draft to avoid a weird git history from incorrect merge order.

@tvm-bot
Copy link
Collaborator

tvm-bot commented Oct 28, 2022

Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.

Generated by tvm-bot

Prior to this commit, the result of TryCompare would only be used if
it could definitively prove a conditional to be either true or false.
For example, if it is known that `0 <= i`, a conditional of `i <= 0`
would be left as-is.

This commit introduces rewrite rules to preferentially simplify
into more restrictive conditions.  Using the same example, if it is
known that `0 <= i`, a conditional of `i <= 0` would be simplified
into `i == 0`.  Similarly, if it is known that `0 <= i`, a
conditional of `i != 0` would be simplified into `0 < i`.

Because this change does not introduce significant overhead, as the
results of `RewriteSimplifier::Impl::TryCompare` are already
available, this change is enabled for all use cases and does not
require a call to `RewriteSimplifier::SetEnabledExtensions`.
@Lunderberg Lunderberg marked this pull request as ready for review October 31, 2022 13:23
@Lunderberg
Copy link
Contributor Author

Rebased onto main, which now includes #13023 to resolve the last failing unit test, and this PR is now ready for review.

@Lunderberg
Copy link
Contributor Author

@tvm-bot rerun

@Lunderberg
Copy link
Contributor Author

@tvm-bot rerun

1 similar comment
@Lunderberg
Copy link
Contributor Author

@tvm-bot rerun

Copy link
Contributor

@tmoreau89 tmoreau89 left a comment

Choose a reason for hiding this comment

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

PR looks sound to me; thank you Eric for the contribution

@tmoreau89 tmoreau89 merged commit ccb7d07 into apache:main Nov 4, 2022
@Lunderberg Lunderberg deleted the partially_prove_inequalities branch November 4, 2022 18:02
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
…e#13024)

Prior to this commit, the result of TryCompare would only be used if
it could definitively prove a conditional to be either true or false.
For example, if it is known that `0 <= i`, a conditional of `i <= 0`
would be left as-is.

This commit introduces rewrite rules to preferentially simplify
into more restrictive conditions.  Using the same example, if it is
known that `0 <= i`, a conditional of `i <= 0` would be simplified
into `i == 0`.  Similarly, if it is known that `0 <= i`, a
conditional of `i != 0` would be simplified into `0 < i`.

Because this change does not introduce significant overhead, as the
results of `RewriteSimplifier::Impl::TryCompare` are already
available, this change is enabled for all use cases and does not
require a call to `RewriteSimplifier::SetEnabledExtensions`.
xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
…e#13024)

Prior to this commit, the result of TryCompare would only be used if
it could definitively prove a conditional to be either true or false.
For example, if it is known that `0 <= i`, a conditional of `i <= 0`
would be left as-is.

This commit introduces rewrite rules to preferentially simplify
into more restrictive conditions.  Using the same example, if it is
known that `0 <= i`, a conditional of `i <= 0` would be simplified
into `i == 0`.  Similarly, if it is known that `0 <= i`, a
conditional of `i != 0` would be simplified into `0 < i`.

Because this change does not introduce significant overhead, as the
results of `RewriteSimplifier::Impl::TryCompare` are already
available, this change is enabled for all use cases and does not
require a call to `RewriteSimplifier::SetEnabledExtensions`.
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.

4 participants