-
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
[Arith] Support eq in detect_clip_bound #13746
[Arith] Support eq in detect_clip_bound #13746
Conversation
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 |
Could you kindly take a look at this? @Lunderberg |
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 like it overall, especially as you pointed out that simplification preferentially changes inequalities into equalities where possible. I have a couple of suggestions, mostly to prevent letting conditions overwrite each other, but overall looks good!
@@ -217,6 +221,9 @@ bool DetectClipBound(const PrimExpr& cond, | |||
} else { | |||
p.min_value = -ret.base; | |||
} | |||
if (is_eq) { |
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.
Should this also be conditioned on p.min_value.defined()
? Otherwise, if p.max_value
has already been set by another condition, this would overwrite that earlier bound. This would be a repetition of the condition inside the if(is_const_int(ret.coeff, -1))
.
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.
Now that I think of it, maybe it would be better to collect an Optional<PrimExpr> min_value
and Optional<PrimExpr> max_value
. For inequalities, only one of the two is non-empty. For equality expressions, both are non-empty. That way, the logic of how to adjust p.min_value
and p.max_value
wouldn't need to be duplicated. I'm thinking something like the following:
Optional<PrimExpr> min_value = NullOpt;
Optional<PrimExpr> max_value = NullOpt;
if(is_const_int(ret.coeff, 1)) {
min_value = -ret.base;
if(is_eq) {
max_value = min_value;
}
} else if(is_const_int(ret.coeff, -1)) {
max_value = ret.base;
if(is_eq) {
min_value = max_value;
}
}
@@ -202,6 +203,9 @@ bool DetectClipBound(const PrimExpr& cond, | |||
} else if (const GENode* op = cond.as<GENode>()) { | |||
if (!op->a.dtype().is_int()) return false; | |||
canonical = op->a - op->b; | |||
} else if (const EQNode* op = cond.as<EQNode>()) { |
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.
For consistency, should this have the same check for if(!op->a.dtype().is_int())
as the other cases?
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.
Thank you for the changes, and LGTM!
Also, the current CI failure has appeared on a few other PRs as well, and should be fixable by rebasing onto main.
cefb2f8
to
db4cd4a
Compare
* Support eq in detect_clip_bound * follow review suggestion
Hi~ this is a patch to handle EqNode in
detect_clip_bound
. We got some regression failure when using this methodology as the power of upstream arithmetic simplifier grows.Many trivial cases like
x >= 4, x in [0, 4]
seems to get well simplified tox==4
and certain assumptions on some condition be non-eq comparation do not hold then.