Skip to content

Commit

Permalink
Handle many more intrinsics in Bounds.cpp (#7823)
Browse files Browse the repository at this point in the history 
* Handle many more intrinsics in Bounds.cpp

This addresses many (but not all) of the `signed integer overflow` issues we're seeing in Google due to #7814 -- a lot of the issues seems to be in code that uses intrinsics that had no handling in value bounds checking, so the bounds were naively large and overflowed.

- Most of the intrinsics from FindIntrinsics.h weren't handled; now they all are (most by lowering to other IR, though the halving_add variants were modeled directly because the bitwise ops don't mesh well)
- strict_float() is just a pass-through
- round() is a best guess (basically, if bounds exist, expand by one as a worst-case)

There are definitely others we should handle here... trunc/floor/ceil probably?

* Fix round() and strict_float() handling

* Update Bounds.cpp

* Fixes?

* trigger buildbots

* Revert saturating_cast handling

* Update Bounds.cpp

---------

Co-authored-by: Andrew Adams <andrew.b.adams@gmail.com>
  • Loading branch information
@steven-johnson @abadams
steven-johnson and abadams authored Dec 1, 2023
1 parent 3136819 commit 4fc2a7d
Show file tree
Hide file tree
Showing 2 changed files with 143 additions and 7 deletions.
149 changes: 142 additions & 7 deletions src/Bounds.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,37 @@ using std::string;
using std::vector;

namespace {

bool can_widen(const Expr &e) {
// We don't want to widen Xtensa 48-bit integers
return e.type().bits() <= 32;
}

bool can_widen_all(const std::vector<Expr> &args) {
for (const auto &e : args) {
if (!can_widen(e)) {
return false;
}
}
return true;
}

Expr widen(Expr a) {
internal_assert(can_widen(a));
Type result_type = a.type().widen();
return Cast::make(result_type, std::move(a));
}

Expr narrow(Expr a) {
Type result_type = a.type().narrow();
return Cast::make(result_type, std::move(a));
}

Expr saturating_narrow(const Expr &a) {
Type narrow = a.type().narrow();
return saturating_cast(narrow, a);
}

int static_sign(const Expr &x) {
if (is_positive_const(x)) {
return 1;
Expand All @@ -56,6 +87,7 @@ int static_sign(const Expr &x) {
}
return 0;
}

} // anonymous namespace

const FuncValueBounds &empty_func_value_bounds() {
Expand Down Expand Up @@ -1195,6 +1227,15 @@ class Bounds : public IRVisitor {
// else fall thru and continue
}

const auto handle_expr_bounds = [this, t](const Expr &e) -> void {
if (e.defined()) {
e.accept(this);
} else {
// Just use the bounds of the type
this->bounds_of_type(t);
}
};

if (op->is_intrinsic(Call::abs)) {
Interval a = arg_bounds.get(0);
interval.min = make_zero(t);
Expand Down Expand Up @@ -1468,6 +1509,7 @@ class Bounds : public IRVisitor {
}
} else if (op->args.size() == 1 &&
(op->is_intrinsic(Call::round) ||
op->is_intrinsic(Call::strict_float) ||
op->name == "ceil_f32" || op->name == "ceil_f64" ||
op->name == "floor_f32" || op->name == "floor_f64" ||
op->name == "exp_f32" || op->name == "exp_f64" ||
Expand Down Expand Up @@ -1518,14 +1560,107 @@ class Bounds : public IRVisitor {
}
interval = result;
} else if (op->is_intrinsic(Call::widen_right_add)) {
Expr add = Add::make(op->args[0], cast(op->args[0].type(), op->args[1]));
add.accept(this);
} else if (op->is_intrinsic(Call::widen_right_sub)) {
Expr sub = Sub::make(op->args[0], cast(op->args[0].type(), op->args[1]));
sub.accept(this);
internal_assert(op->args.size() == 2);
Expr e = can_widen(op->args[1]) ?
lower_widen_right_add(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widen_right_mul)) {
Expr mul = Mul::make(op->args[0], cast(op->args[0].type(), op->args[1]));
mul.accept(this);
internal_assert(op->args.size() == 2);
Expr e = can_widen(op->args[1]) ?
lower_widen_right_mul(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widen_right_sub)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen(op->args[1]) ?
lower_widen_right_sub(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widening_add)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
lower_widening_add(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widening_mul)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
lower_widening_mul(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widening_sub)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
lower_widening_sub(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::saturating_add)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
narrow(clamp(widen(op->args[0]) + widen(op->args[1]), t.min(), t.max())) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::saturating_sub)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
narrow(clamp(widen(op->args[0]) - widen(op->args[1]), t.min(), t.max())) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widening_shift_left)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen(op->args[0]) ?
lower_widening_shift_left(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::widening_shift_right)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen(op->args[0]) ?
lower_widening_shift_right(op->args[0], op->args[1]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::rounding_shift_right)) {
internal_assert(op->args.size() == 2);
// TODO: uses bitwise ops we may not handle well
handle_expr_bounds(lower_rounding_shift_right(op->args[0], op->args[1]));
} else if (op->is_intrinsic(Call::rounding_shift_left)) {
internal_assert(op->args.size() == 2);
// TODO: uses bitwise ops we may not handle well
handle_expr_bounds(lower_rounding_shift_left(op->args[0], op->args[1]));
} else if (op->is_intrinsic(Call::halving_add)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
narrow((widen(op->args[0]) + widen(op->args[1])) / 2) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::halving_sub)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
narrow((widen(op->args[0]) - widen(op->args[1])) / 2) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::rounding_halving_add)) {
internal_assert(op->args.size() == 2);
Expr e = can_widen_all(op->args) ?
narrow((widen(op->args[0]) + widen(op->args[1]) + 1) / 2) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::rounding_mul_shift_right)) {
internal_assert(op->args.size() == 3);
Expr e = can_widen_all(op->args) ?
saturating_narrow(rounding_shift_right(widening_mul(op->args[0], op->args[1]), op->args[2])) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::mul_shift_right)) {
internal_assert(op->args.size() == 3);
Expr e = can_widen_all(op->args) ?
saturating_narrow(widening_mul(op->args[0], op->args[1]) >> op->args[2]) :
Expr();
handle_expr_bounds(e);
} else if (op->is_intrinsic(Call::sorted_avg)) {
internal_assert(op->args.size() == 2);
Expr e = lower_sorted_avg(op->args[0], op->args[1]);
handle_expr_bounds(e);
} else if (op->call_type == Call::Halide) {
bounds_of_func(op->name, op->value_index, op->type);
} else {
Expand Down
1 change: 1 addition & 0 deletions src/FindIntrinsics.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Expr lower_saturating_cast(const Type &t, const Expr &a);
Expr lower_halving_add(const Expr &a, const Expr &b);
Expr lower_halving_sub(const Expr &a, const Expr &b);
Expr lower_rounding_halving_add(const Expr &a, const Expr &b);
Expr lower_sorted_avg(const Expr &a, const Expr &b);

Expr lower_mul_shift_right(const Expr &a, const Expr &b, const Expr &q);
Expr lower_rounding_mul_shift_right(const Expr &a, const Expr &b, const Expr &q);
Expand Down

0 comments on commit 4fc2a7d

Please sign in to comment.