[ARITH] Migrate simplifier to new infra #3368
Conversation
tqchen
changed the title
tqchen
force-pushed
the
arith
branch
2 times, most recently
from
47a1a33 to
ac5ec4b
Compare
tqchen
force-pushed
the
arith
branch
7 times, most recently
from
25ec955 to
7f7efe8
Compare
|
cc @dmlc/tvm-team |
| bool target_is_non_neg = expr_map_[target_var].can_prove_non_negative(); | ||
|
|
||
| if (is_greater_) { | ||
| result_ += 1; |
There was a problem hiding this comment.
I think this may be wrong: if we consider x*2 >= -1, then it will be transformed into x >= 0 + 1 which excludes x = 0.
There was a problem hiding this comment.
The bound deduction may allow relaxation and is not guaranteed to be tight
There was a problem hiding this comment.
But I agree we could start to think about generating tighter bound if possible(by looking into signs)
There was a problem hiding this comment.
No, this is wrong, because in this case it is not a relaxation, but rather an overtightening, which is unsound.
There was a problem hiding this comment.
Sorry, I meant overtightening, as documented by the API note, we want to find a set in which the condition is always satisfied, so we can eliminate the condition by constraining the loop within that range.
There was a problem hiding this comment.
Oh, ok, that makes sense. However I think the DeduceBound function may return an approximation which is not a subset of the true set, because it also performs relaxation. I'm not sure, but I think it might break loop partitioning in some cases.
|
Thanks, @sgrechanik-h , this PR is now merged |
|
By the way, I'm a bit concerned that this and some other PRs were merged without sufficient review and discussion. |
Migrate the current usage of simplifier to new infra part of #2588
After this PR. We are no longer dependent on HalideIR's Simplifier, and completely relies on the new integer analysis infrastructure. We might get certain regressions due to slightly different behavior, although the general infra will head to a better direction as we continue to improve the new consistent simplification infrastructure.