Lunderberg commented on code in PR #13746:
URL: https://github.com/apache/tvm/pull/13746#discussion_r1081430254
##########
src/arith/detect_linear_equation.cc:
##########
@@ -217,6 +221,9 @@ bool DetectClipBound(const PrimExpr& cond,
} else {
p.min_value = -ret.base;
}
+ if (is_eq) {
Review Comment:
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:
```c++
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;
}
}
```
##########
src/arith/detect_linear_equation.cc:
##########
@@ -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>()) {
Review Comment:
For consistency, should this have the same check for
`if(!op->a.dtype().is_int())` as the other cases?
##########
src/arith/detect_linear_equation.cc:
##########
@@ -217,6 +221,9 @@ bool DetectClipBound(const PrimExpr& cond,
} else {
p.min_value = -ret.base;
}
+ if (is_eq) {
Review Comment:
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))`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
