-
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] Inequalities solver #5618
Conversation
@tqchen @sergei-grechanik @MarisaKirisame @eric-haibin-lin @ArmageddonKnight It's ready for review. |
Also cc @xqdan @junrushao1994 if you have time review. |
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 took a look at the code. However i suck at numeric so I dont know what it is doing. I left some small word smithing.
Co-authored-by: Sergei Grechanik <[email protected]>
python/tvm/testing.py
Outdated
|
||
rels = constraints_trans.dst.relations | ||
if len(rels) == 1 and tvm.ir.structural_equal(rels[0], False): | ||
# not solvable, skip |
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 think it should be checked here that the original constraints are also unsolvable.
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.
yes.. but how can I detect the contradiction in the original constraints without running the solver?
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.
You can check that the original constraints are false for all values of the variables. I would remove this check completely, I think _check_forward should do what is needed automatically or with some minor changes.
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.
@yzhliu : Thanks for the PR! Great work 👍
Please find some high level comments and some queries. Thanks!
include/tvm/arith/int_solver.h
Outdated
* \param r The range | ||
* \return constructed bounds. | ||
*/ | ||
static IntGrpBounds range(const Range& r); |
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.
Maybe range --> CreateIntGrpBounds (Or something like that, as we are creating Bounds from Range)?
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'm ok with either way. This is following https://github.com/apache/incubator-tvm/blob/master/src/arith/int_set.cc#L600
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.
Thanks! I see now. I am also okay if it is general convention:)
@tqchen : Do you have any thoughts on the comment ?
@sergei-grechanik @ANSHUMAN87 @tqchen @MarisaKirisame Please take a look again. |
// Note that we use the 0-th expression because they are ordered by complexity, | ||
// so it must be the simplest one. | ||
Range best_range(bnd->equal[0], analyzer.Simplify(bnd->equal[0] + 1, 3)); | ||
res_ranges.Set(var, best_range); |
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.
nit: res_ranges.Set & vranges.Set can be taken to common place below if...else... block.
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.
in the else branch, they are not set unless best_range
is defined.
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.
some minor comments about naming
include/tvm/arith/int_solver.h
Outdated
* coef * var <= upper | ||
* \sa IntGrpBounds | ||
*/ | ||
class IntGrpBoundsNode : public Object { |
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.
IntGroupBounds
include/tvm/arith/int_solver.h
Outdated
* \param r The range | ||
* \return constructed bounds. | ||
*/ | ||
static IntGrpBounds range(const Range& r); |
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.
How about FromRange
|
||
@staticmethod | ||
def make_by_range(rng): | ||
"""Construct a IntGroupedBounds by Range. |
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.
from_range
@tqchen @ANSHUMAN87 Please take a look again. |
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.
LGTM. Thanks @yzhliu 👍
@tqchen kindly ping |
This is the LinearInequalitySolver mentioned in https://discuss.tvm.ai/t/rfc-bring-in-tensor-expression-autodiff