-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Tighten bounds of abs() #8168
Tighten bounds of abs() #8168
Conversation
if (a.is_bounded()) { | ||
if (equal(a.min, a.max)) { | ||
interval = Interval::single_point(Call::make(t, Call::abs, {a.max}, Call::PureIntrinsic)); | ||
} else if (op->args[0].type().is_int() && op->args[0].type().bits() >= 32) { | ||
interval.max = Max::make(Cast::make(t, -a.min), Cast::make(t, a.max)); | ||
interval.min = Cast::make(t, Max::make(make_zero(a.min.type()), Max::make(a.min, -a.max))); | ||
interval.max = Cast::make(t, Max::make(-a.min, a.max)); |
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.
It's abs, so t is unsigned, and a.min.type() is signed. Don't you want to do the max in the unsigned type?
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.
If a = [2, 10] then originally you got interval.max = max(u32(-2), u32(10)) = 4294967294
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.
Right, I was only considering the a.min < 0 case. Doing it in the unsigned type would be: max(abs(a.min), abs(a.max))
, but I'm guessing elsewhere in bounds inference isn't going to like that.
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.
Yeah, the original code works for a.min < 0
, but I think the new code works for both negative and positive a.min
. If you assume negation can't overflow, then the comparison should be done in the signed type.
Ready to land? |
This PR does four things:
abs()
on an expression that is strictly positiveabs()
on anint32/int64expression that is strictly negativeabs()
of an int32/int64 that could be negative. (move cast outside of the max).I am struggling to come up with the right lower bound that tightens the interval forabs()
of a strictly negative thing that is a smaller integer type, because-a.max
could overflow.Update: figured out the right expression for tightening bounds without negating something that could overflow. (updated z3 example as well)
Also, new bounds are formally verified via z3: