Skip to content
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

Merged
merged 3 commits into from
Apr 5, 2024
Merged

Tighten bounds of abs() #8168

merged 3 commits into from
Apr 5, 2024

Conversation

rootjalex
Copy link
Member

@rootjalex rootjalex commented Mar 31, 2024

This PR does four things:

  • Drive-by update of error message regarding LLVM version.
  • Tightens bounds of abs() on an expression that is strictly positive
  • Tightens bounds of abs() on an int32/int64 expression that is strictly negative
  • Fixes horrendous upper bound overflow on abs() 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 for abs() 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:

import z3
abs = lambda x: z3.If(x > 0, x, 0 - x)
min = lambda x, y: z3.If(x > y, y, x)
max = lambda x, y: z3.If(x > y, x, y)
x, xl, xu = z3.Ints("x xl xu")

z3.solve(xl <= x, x <= xu, abs(x) < max(xl, 0 - min(0, xu)))
z3.solve(xl <= x, abs(x) < max(xl, 0))
z3.solve(x <= xu, abs(x) < 0 - min(0, xu))

@rootjalex rootjalex requested a review from abadams March 31, 2024 02:12
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));
Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

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.

@steven-johnson
Copy link
Contributor

Ready to land?

@abadams abadams merged commit a462044 into main Apr 5, 2024
19 checks passed
@rootjalex rootjalex deleted the rootjalex/abs-bounds branch April 5, 2024 17:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants