From 0e64045a8286b362c135a1a29d40c26c74912913 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Tue, 29 Aug 2023 16:43:34 -0700 Subject: [PATCH 1/7] 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 https://github.com/halide/Halide/pull/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? --- src/Bounds.cpp | 51 +++++++++++++++++++++++++++++++++++++++++--------- 1 file changed, 42 insertions(+), 9 deletions(-) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index e96217edb2c1..0cbeebab4493 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -41,6 +41,17 @@ using std::string; using std::vector; namespace { + +Expr widen(Expr 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)); +} + int static_sign(const Expr &x) { if (is_positive_const(x)) { return 1; @@ -56,6 +67,7 @@ int static_sign(const Expr &x) { } return 0; } + } // anonymous namespace const FuncValueBounds &empty_func_value_bounds() { @@ -1276,6 +1288,18 @@ class Bounds : public IRVisitor { } else if (op->is_intrinsic(Call::return_second)) { internal_assert(op->args.size() == 2); interval = arg_bounds.get(1); + } else if (op->is_intrinsic(Call::strict_float)) { + internal_assert(op->args.size() == 1); + interval = arg_bounds.get(0); + } else if (op->is_intrinsic(Call::round)) { + internal_assert(op->args.size() == 1); + interval = arg_bounds.get(0); + if (interval.has_lower_bound()) { + interval.min -= 1.f; + } + if (interval.has_upper_bound()) { + interval.max += 1.f; + } } else if (op->is_intrinsic(Call::if_then_else)) { internal_assert(op->args.size() == 2 || op->args.size() == 3); // Probably more conservative than necessary @@ -1517,15 +1541,24 @@ class Bounds : public IRVisitor { result.include(arg_bounds.get(i)); } 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); - } 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); + } else if (op->is_intrinsic(Call::halving_add)) { + // lower_halving_add() uses bitwise tricks that are hard to reason + // about; let's do this instead: + Expr e = narrow((widen(op->args[0]) + widen(op->args[1])) / 2); + e.accept(this); + } else if (op->is_intrinsic(Call::rounding_halving_add)) { + // lower_rounding_halving_add() uses bitwise tricks that are hard to reason + // about; let's do this instead: + Expr e = narrow((widen(op->args[0]) + widen(op->args[1]) + 1) / 2); + e.accept(this); + } else if (op->call_type == Call::PureIntrinsic) { + Expr e = lower_intrinsic(op); + if (e.defined()) { + e.accept(this); + } else { + // Just use the bounds of the type + bounds_of_type(t); + } } else if (op->call_type == Call::Halide) { bounds_of_func(op->name, op->value_index, op->type); } else { From fd0b4757ee5561a02986d019033a9795818cd303 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Tue, 29 Aug 2023 17:12:48 -0700 Subject: [PATCH 2/7] Fix round() and strict_float() handling --- src/Bounds.cpp | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index 0cbeebab4493..b6be89d3394d 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -1288,18 +1288,6 @@ class Bounds : public IRVisitor { } else if (op->is_intrinsic(Call::return_second)) { internal_assert(op->args.size() == 2); interval = arg_bounds.get(1); - } else if (op->is_intrinsic(Call::strict_float)) { - internal_assert(op->args.size() == 1); - interval = arg_bounds.get(0); - } else if (op->is_intrinsic(Call::round)) { - internal_assert(op->args.size() == 1); - interval = arg_bounds.get(0); - if (interval.has_lower_bound()) { - interval.min -= 1.f; - } - if (interval.has_upper_bound()) { - interval.max += 1.f; - } } else if (op->is_intrinsic(Call::if_then_else)) { internal_assert(op->args.size() == 2 || op->args.size() == 3); // Probably more conservative than necessary @@ -1492,6 +1480,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" || From 0d5b3e4b9853acefa7eb95291b48e7cee76740e7 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Tue, 29 Aug 2023 18:33:03 -0700 Subject: [PATCH 3/7] Update Bounds.cpp --- src/Bounds.cpp | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index b6be89d3394d..d5740a12da71 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -1533,13 +1533,22 @@ class Bounds : public IRVisitor { } else if (op->is_intrinsic(Call::halving_add)) { // lower_halving_add() uses bitwise tricks that are hard to reason // about; let's do this instead: - Expr e = narrow((widen(op->args[0]) + widen(op->args[1])) / 2); - e.accept(this); + Expr e; + if (op->type.bits() == 64) { + bounds_of_type(t); + } else { + Expr e = narrow((widen(op->args[0]) + widen(op->args[1])) / 2); + e.accept(this); + } } else if (op->is_intrinsic(Call::rounding_halving_add)) { // lower_rounding_halving_add() uses bitwise tricks that are hard to reason // about; let's do this instead: - Expr e = narrow((widen(op->args[0]) + widen(op->args[1]) + 1) / 2); - e.accept(this); + if (op->type.bits() == 64) { + bounds_of_type(t); + } else { + Expr e = narrow((widen(op->args[0]) + widen(op->args[1]) + 1) / 2); + e.accept(this); + } } else if (op->call_type == Call::PureIntrinsic) { Expr e = lower_intrinsic(op); if (e.defined()) { From c78829434d323c9208141b4d354621218d06b6b3 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 31 Aug 2023 11:38:14 -0700 Subject: [PATCH 4/7] Fixes? --- src/Bounds.cpp | 176 +++++++++++++++++++++++++++++++++++-------- src/FindIntrinsics.h | 1 + 2 files changed, 147 insertions(+), 30 deletions(-) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index d5740a12da71..d9cafa4a3325 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -42,7 +42,21 @@ using std::vector; namespace { +bool can_widen(const Expr &e) { + return e.type().bits() < 64; +} + +bool can_widen_all(const std::vector &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)); } @@ -52,6 +66,11 @@ Expr narrow(Expr a) { 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; @@ -1207,6 +1226,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); @@ -1241,12 +1269,6 @@ class Bounds : public IRVisitor { bounds_of_type(t); } } - } else if (op->is_intrinsic(Call::saturating_cast)) { - internal_assert(op->args.size() == 1); - - Expr a = lower_saturating_cast(op->type, op->args[0]); - a.accept(this); - return; } else if (op->is_intrinsic(Call::unsafe_promise_clamped) || op->is_intrinsic(Call::promise_clamped)) { // Unlike an explicit clamp, we are also permitted to @@ -1530,33 +1552,128 @@ class Bounds : public IRVisitor { result.include(arg_bounds.get(i)); } interval = result; - } else if (op->is_intrinsic(Call::halving_add)) { - // lower_halving_add() uses bitwise tricks that are hard to reason - // about; let's do this instead: - Expr e; - if (op->type.bits() == 64) { - bounds_of_type(t); - } else { - Expr e = narrow((widen(op->args[0]) + widen(op->args[1])) / 2); - e.accept(this); - } - } else if (op->is_intrinsic(Call::rounding_halving_add)) { - // lower_rounding_halving_add() uses bitwise tricks that are hard to reason - // about; let's do this instead: - if (op->type.bits() == 64) { - bounds_of_type(t); + } else if (op->is_intrinsic(Call::widen_right_add)) { + 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)) { + 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::saturating_cast)) { + internal_assert(op->args.size() == 1); + bounds_of_type(t); + Interval type_bounds = interval; + interval = arg_bounds.get(0); + + if (interval.has_lower_bound()) { + interval.min = saturating_cast(t, interval.min); + } else if (op->args[0].type().is_uint()) { + // If we're casting from a uint, we can at least lower bound it + // with zero. + interval.min = make_zero(t); } else { - Expr e = narrow((widen(op->args[0]) + widen(op->args[1]) + 1) / 2); - e.accept(this); + interval.min = type_bounds.min; } - } else if (op->call_type == Call::PureIntrinsic) { - Expr e = lower_intrinsic(op); - if (e.defined()) { - e.accept(this); + if (interval.has_upper_bound()) { + interval.max = saturating_cast(t, interval.max); } else { - // Just use the bounds of the type - bounds_of_type(t); + interval.max = type_bounds.max; } + } 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 { @@ -2292,7 +2409,6 @@ class BoxesTouched : public IRGraphVisitor { Stmt else_case = Evaluate::make(op->args[2]); equivalent_if = IfThenElse::make(op->args[0], then_case, else_case); } else { - internal_assert(op->args.size() == 2); equivalent_if = IfThenElse::make(op->args[0], then_case); } equivalent_if.accept(this); diff --git a/src/FindIntrinsics.h b/src/FindIntrinsics.h index f8ddaf171bc3..fc4c2a8e90f5 100644 --- a/src/FindIntrinsics.h +++ b/src/FindIntrinsics.h @@ -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); From 2bcb879f7689196d5ad79674f78b29ed06480ce7 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Wed, 29 Nov 2023 14:46:55 -0800 Subject: [PATCH 5/7] trigger buildbots From 13745f8404c333adc39afb255d7fef9dc8e063c3 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 30 Nov 2023 10:49:52 -0800 Subject: [PATCH 6/7] Revert saturating_cast handling --- src/Bounds.cpp | 29 ++++++++--------------------- 1 file changed, 8 insertions(+), 21 deletions(-) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index b4e50a2e5f72..191d64933144 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -43,7 +43,8 @@ using std::vector; namespace { bool can_widen(const Expr &e) { - return e.type().bits() < 64; + // We don't want to widen Xtensa 48-bit integers + return e.type().bits() <= 32; } bool can_widen_all(const std::vector &args) { @@ -1269,6 +1270,12 @@ class Bounds : public IRVisitor { bounds_of_type(t); } } + } else if (op->is_intrinsic(Call::saturating_cast)) { + internal_assert(op->args.size() == 1); + + Expr a = lower_saturating_cast(op->type, op->args[0]); + a.accept(this); + return; } else if (op->is_intrinsic(Call::unsafe_promise_clamped) || op->is_intrinsic(Call::promise_clamped)) { // Unlike an explicit clamp, we are also permitted to @@ -1600,26 +1607,6 @@ class Bounds : public IRVisitor { 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_cast)) { - internal_assert(op->args.size() == 1); - bounds_of_type(t); - Interval type_bounds = interval; - interval = arg_bounds.get(0); - - if (interval.has_lower_bound()) { - interval.min = saturating_cast(t, interval.min); - } else if (op->args[0].type().is_uint()) { - // If we're casting from a uint, we can at least lower bound it - // with zero. - interval.min = make_zero(t); - } else { - interval.min = type_bounds.min; - } - if (interval.has_upper_bound()) { - interval.max = saturating_cast(t, interval.max); - } else { - interval.max = type_bounds.max; - } } else if (op->is_intrinsic(Call::widening_shift_left)) { internal_assert(op->args.size() == 2); Expr e = can_widen(op->args[0]) ? From a42428ffa4f271600f62e113428e18db26e97506 Mon Sep 17 00:00:00 2001 From: Steven Johnson Date: Thu, 30 Nov 2023 10:51:39 -0800 Subject: [PATCH 7/7] Update Bounds.cpp --- src/Bounds.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/Bounds.cpp b/src/Bounds.cpp index 191d64933144..0ba1f5440056 100644 --- a/src/Bounds.cpp +++ b/src/Bounds.cpp @@ -2396,6 +2396,7 @@ class BoxesTouched : public IRGraphVisitor { Stmt else_case = Evaluate::make(op->args[2]); equivalent_if = IfThenElse::make(op->args[0], then_case, else_case); } else { + internal_assert(op->args.size() == 2); equivalent_if = IfThenElse::make(op->args[0], then_case); } equivalent_if.accept(this);