Hi,
On Mon, 9 Nov 2020 at 15:43, Aldy Hernandez via Gcc-patches
<gcc-patches@gcc.gnu.org> wrote:
>
> [This is actually part of a larger patch that actually changes
> behavior, but I thought I'd commit the non-invasive cleanups first
> which will simplify the upcoming work.]
>
> irange::set was doing more work than it should for legacy ranges.
> I cleaned up various unnecessary calls to swap_out_of_order_endpoints,
> as well as some duplicate code that could be done with normalize_min_max.
>
> I also removed an obsolete comment wrt sticky infinite overflows.
> Not only did the -INF/+INF(OVF) code get removed in 2017,
> but normalize_min_max() uses wide ints, which ignored overflows
> altogether.
>
> Pushed.
>
> gcc/ChangeLog:
>
> * value-range.cc (irange::swap_out_of_order_endpoints): Rewrite
> into static function.
> (irange::set): Cleanup redundant manipulations.
> * value-range.h (irange::normalize_min_max): Modify object
> in-place instead of modifying arguments.
This is causing regressions on aarch64, such as:
FAIL: gcc.target/aarch64/pr88838.c (internal compiler error)
FAIL: gcc.target/aarch64/pr88838.c (test for excess errors)
Excess errors:
during GIMPLE pass: dom
/gcc/testsuite/gcc.target/aarch64/pr88838.c:5:1: internal compiler
error: tree check: expected integer_cst, have poly_int_cst in to_wide,
at tree.h:5950
0x66741d tree_check_failed(tree_node const*, char const*, int, char const*, ...)
/gcc/tree.c:9754
0x6404e8 tree_check
/gcc/tree.h:3570
0x6404e8 to_wide
/gcc/tree.h:5950
0x7f0816 wi::to_wide(tree_node const*)
/gcc/tree.h:3505
0x1118bdc irange::legacy_upper_bound(unsigned int) const
/gcc/value-range.cc:447
0x1122cd4 irange::upper_bound(unsigned int) const
/gcc/value-range.h:510
0x19493be range_operator::fold_range(irange&, tree_node*, irange
const&, irange const&) const
/gcc/range-op.cc:159
0x19494f4 range_operator::fold_range(irange&, tree_node*, irange
const&, irange const&) const
/gcc/range-op.cc:74
0x10cfee6 range_fold_binary_expr(int_range<1u>*, tree_code,
tree_node*, int_range<1u> const*, int_range<1u> const*)
/gcc/tree-vrp.c:1243
0x116fda4 vr_values::extract_range_from_binary_expr(value_range_equiv*,
tree_code, tree_node*, tree_node*, tree_node*)
/gcc/vr-values.c:853
0x1178b3c vr_values::extract_range_from_assignment(value_range_equiv*, gassign*)
/gcc/vr-values.c:1564
0x185bcbe evrp_range_analyzer::record_ranges_from_stmt(gimple*, bool)
/gcc/gimple-ssa-evrp-analyze.c:304
0xee7f9b dom_opt_dom_walker::before_dom_children(basic_block_def*)
/gcc/tree-ssa-dom.c:1458
0x1817487 dom_walker::walk(basic_block_def*)
/gcc/domwalk.c:309
0xee4f8b execute
/gcc/tree-ssa-dom.c:724
(actually I can see 3245 ICEs on aarch64)
Can you fix it?
Thanks
> ---
> gcc/value-range.cc | 70 ++++++++++++++++------------------------------
> gcc/value-range.h | 28 +++++++++----------
> 2 files changed, 37 insertions(+), 61 deletions(-)
>
> diff --git a/gcc/value-range.cc b/gcc/value-range.cc
> index 5827e812216..2124e229e0c 100644
> --- a/gcc/value-range.cc
> +++ b/gcc/value-range.cc
> @@ -131,13 +131,14 @@ irange::copy_to_legacy (const irange &src)
> set (src.tree_lower_bound (), src.tree_upper_bound ());
> }
>
> -// Swap min/max if they are out of order. Return TRUE if further
> -// processing of the range is necessary, FALSE otherwise.
> +// Swap MIN/MAX if they are out of order and adjust KIND appropriately.
>
> -bool
> -irange::swap_out_of_order_endpoints (tree &min, tree &max,
> - value_range_kind &kind)
> +static void
> +swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind &kind)
> {
> + gcc_checking_assert (kind != VR_UNDEFINED);
> + if (kind == VR_VARYING)
> + return;
> /* Wrong order for min and max, to swap them and the VR type we need
> to adjust them. */
> if (tree_int_cst_lt (max, min))
> @@ -149,8 +150,8 @@ irange::swap_out_of_order_endpoints (tree &min, tree &max,
> for VR_ANTI_RANGE empty range, so drop to varying as well. */
> if (TYPE_PRECISION (TREE_TYPE (min)) == 1)
> {
> - set_varying (TREE_TYPE (min));
> - return false;
> + kind = VR_VARYING;
> + return;
> }
>
> one = build_int_cst (TREE_TYPE (min), 1);
> @@ -163,12 +164,11 @@ irange::swap_out_of_order_endpoints (tree &min, tree
> &max,
> to varying in this case. */
> if (tree_int_cst_lt (max, min))
> {
> - set_varying (TREE_TYPE (min));
> - return false;
> + kind = VR_VARYING;
> + return;
> }
> kind = kind == VR_RANGE ? VR_ANTI_RANGE : VR_RANGE;
> }
> - return true;
> }
>
> void
> @@ -253,13 +253,6 @@ irange::set (tree min, tree max, value_range_kind kind)
> && (POLY_INT_CST_P (min) || POLY_INT_CST_P (max)))
> kind = VR_VARYING;
>
> - if (kind == VR_VARYING)
> - {
> - set_varying (TREE_TYPE (min));
> - return;
> - }
> -
> - tree type = TREE_TYPE (min);
> // Nothing to canonicalize for symbolic ranges.
> if (TREE_CODE (min) != INTEGER_CST
> || TREE_CODE (max) != INTEGER_CST)
> @@ -270,8 +263,13 @@ irange::set (tree min, tree max, value_range_kind kind)
> m_num_ranges = 1;
> return;
> }
> - if (!swap_out_of_order_endpoints (min, max, kind))
> - goto cleanup_set;
> +
> + swap_out_of_order_endpoints (min, max, kind);
> + if (kind == VR_VARYING)
> + {
> + set_varying (TREE_TYPE (min));
> + return;
> + }
>
> // Anti-ranges that can be represented as ranges should be so.
> if (kind == VR_ANTI_RANGE)
> @@ -280,6 +278,7 @@ irange::set (tree min, tree max, value_range_kind kind)
> values < -INF and values > INF as -INF/INF as well. */
> bool is_min = vrp_val_is_min (min);
> bool is_max = vrp_val_is_max (max);
> + tree type = TREE_TYPE (min);
>
> if (is_min && is_max)
> {
> @@ -314,38 +313,17 @@ irange::set (tree min, tree max, value_range_kind kind)
> kind = VR_RANGE;
> }
> }
> - else if (!swap_out_of_order_endpoints (min, max, kind))
> - goto cleanup_set;
> -
> - /* Do not drop [-INF(OVF), +INF(OVF)] to varying. (OVF) has to be sticky
> - to make sure VRP iteration terminates, otherwise we can get into
> - oscillations. */
> - if (!normalize_min_max (type, min, max, kind))
> - {
> - m_kind = kind;
> - m_base[0] = min;
> - m_base[1] = max;
> - m_num_ranges = 1;
> - if (flag_checking)
> - verify_range ();
> - }
>
> - cleanup_set:
> - // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can
> - // restrict those to a subset of what actually fits in the type.
> - // Instead use the extremes of the type precision
> - unsigned prec = TYPE_PRECISION (type);
> - signop sign = TYPE_SIGN (type);
> - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign))
> - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign)))
> - m_kind = VR_VARYING;
> - else if (undefined_p ())
> - m_kind = VR_UNDEFINED;
> + m_kind = kind;
> + m_base[0] = min;
> + m_base[1] = max;
> + m_num_ranges = 1;
> + normalize_min_max ();
> if (flag_checking)
> verify_range ();
> }
>
> -/* Check the validity of the range. */
> +// Check the validity of the range.
>
> void
> irange::verify_range ()
> diff --git a/gcc/value-range.h b/gcc/value-range.h
> index 760ee772316..a483fc802dd 100644
> --- a/gcc/value-range.h
> +++ b/gcc/value-range.h
> @@ -111,8 +111,7 @@ protected:
> void irange_set (tree, tree);
> void irange_set_anti_range (tree, tree);
>
> - bool swap_out_of_order_endpoints (tree &min, tree &max, value_range_kind
> &);
> - bool normalize_min_max (tree type, tree min, tree max, value_range_kind);
> + void normalize_min_max ();
>
> bool legacy_mode_p () const;
> bool legacy_equal_p (const irange &) const;
> @@ -566,7 +565,7 @@ irange::set_zero (tree type)
> irange_set (z, z);
> }
>
> -// Normalize [MIN, MAX] into VARYING and ~[MIN, MAX] into UNDEFINED.
> +// Normalize a range to VARYING or UNDEFINED if possible.
> //
> // Avoid using TYPE_{MIN,MAX}_VALUE because -fstrict-enums can
> // restrict those to a subset of what actually fits in the type.
> @@ -575,24 +574,23 @@ irange::set_zero (tree type)
> // whereas if we used TYPE_*_VAL, said function would just punt upon
> // seeing a VARYING.
>
> -inline bool
> -irange::normalize_min_max (tree type, tree min, tree max,
> - value_range_kind kind)
> +inline void
> +irange::normalize_min_max ()
> {
> - unsigned prec = TYPE_PRECISION (type);
> - signop sign = TYPE_SIGN (type);
> - if (wi::eq_p (wi::to_wide (min), wi::min_value (prec, sign))
> - && wi::eq_p (wi::to_wide (max), wi::max_value (prec, sign)))
> + gcc_checking_assert (legacy_mode_p ());
> + gcc_checking_assert (!undefined_p ());
> + unsigned prec = TYPE_PRECISION (type ());
> + signop sign = TYPE_SIGN (type ());
> + if (wi::eq_p (wi::to_wide (min ()), wi::min_value (prec, sign))
> + && wi::eq_p (wi::to_wide (max ()), wi::max_value (prec, sign)))
> {
> - if (kind == VR_RANGE)
> - set_varying (type);
> - else if (kind == VR_ANTI_RANGE)
> + if (m_kind == VR_RANGE)
> + set_varying (type ());
> + else if (m_kind == VR_ANTI_RANGE)
> set_undefined ();
> else
> gcc_unreachable ();
> - return true;
> }
> - return false;
> }
>
> // Return the maximum value for TYPE.
> --
> 2.26.2
>
