[PATCH] Fix some EVRP stupidness
Richard Biener
rguenther@suse.de
Thu Oct 18 13:09:00 GMT 2018
On Thu, 18 Oct 2018, Richard Biener wrote:
>
> At some point we decided to not simply intersect all ranges we get
> via register_edge_assert_for. Instead we simply register them
> in-order. That causes things like replacing [64, +INF] with ~[0, 0].
>
> The following patch avoids replacing a range with a larger one
> as obvious improvement.
>
> Compared to assert_expr based VRP we lack the ability to put down
> actual assert_exprs and thus multiple SSA names with ranges we
> could link via equivalences. In the end we need sth similar,
> for example by keeping a stack of active ranges for each SSA name.
>
> Bootstrapped and tested on x86_64-unknown-linux-gnu, applied to trunk.
Actually not. Needed to update to the new value_range class and after
that (and its introduction of ->check()) we now ICE during bootstrap
with
during GIMPLE pass: evrp
/space/rguenther/src/svn/trunk/libgcc/config/libbid/bid128_div.c: In
function Β‘get_BID128Β’:
/space/rguenther/src/svn/trunk/libgcc/config/libbid/bid128_div.c:1851:1:
internal compiler error: in check, at tree-vrp.c:155
1851 | }
| ^
0xf3a8b5 value_range::check()
/space/rguenther/src/svn/trunk/gcc/tree-vrp.c:155
0xf42424 value_range::value_range(value_range_kind, tree_node*,
tree_node*, bitmap_head*)
/space/rguenther/src/svn/trunk/gcc/tree-vrp.c:110
0xf42424 set_value_range_with_overflow
/space/rguenther/src/svn/trunk/gcc/tree-vrp.c:1422
0xf42424 extract_range_from_binary_expr_1(value_range*, tree_code,
tree_node*, value_range const*, value_range const*)
/space/rguenther/src/svn/trunk/gcc/tree-vrp.c:1679
for a PLUS_EXPR of [12254, expon_43] and [-1, -1] yielding
(temporarily!) [12254, -1] before supposed to be adjusted by the
symbolic bound:
/* Adjust the range for possible overflow. */
set_value_range_with_overflow (*vr, expr_type,
wmin, wmax, min_ovf, max_ovf);
^^^ ICE
if (vr->varying_p ())
return;
/* Build the symbolic bounds if needed. */
min = vr->min ();
max = vr->max ();
adjust_symbolic_bound (min, code, expr_type,
sym_min_op0, sym_min_op1,
neg_min_op0, neg_min_op1);
adjust_symbolic_bound (max, code, expr_type,
sym_max_op0, sym_max_op1,
neg_max_op0, neg_max_op1);
type = vr->kind ();
I think the refactoring that was applied here is simply not suitable
because *vr is _not_ necessarily a valid range before the symbolic
bounds have been adjusted. A fix would be sth like the following
which I am going to test now.
Richard.
diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
index 40d40e5e2fe..c5748a43246 100644
--- a/gcc/tree-vrp.c
+++ b/gcc/tree-vrp.c
@@ -1328,7 +1328,7 @@ combine_bound (enum tree_code code, wide_int &wi,
wi::overflow_type &ovf,
underflow. +1 indicates overflow. 0 indicates neither. */
static void
-set_value_range_with_overflow (value_range &vr,
+set_value_range_with_overflow (value_range_kind &kind, tree &min, tree
&max,
tree type,
const wide_int &wmin, const wide_int &wmax,
wi::overflow_type min_ovf,
@@ -1341,7 +1341,7 @@ set_value_range_with_overflow (value_range &vr,
range covers all values. */
if (prec == 1 && wi::lt_p (wmax, wmin, sgn))
{
- set_value_range_to_varying (&vr);
+ kind = VR_VARYING;
return;
}
@@ -1357,13 +1357,15 @@ set_value_range_with_overflow (value_range &vr,
the entire range. We have a similar check at the end of
extract_range_from_binary_expr_1. */
if (wi::gt_p (tmin, tmax, sgn))
- vr.set_varying ();
+ kind = VR_VARYING;
else
- /* No overflow or both overflow or underflow. The
- range kind stays VR_RANGE. */
- vr = value_range (VR_RANGE,
- wide_int_to_tree (type, tmin),
- wide_int_to_tree (type, tmax));
+ {
+ kind = VR_RANGE;
+ /* No overflow or both overflow or underflow. The
+ range kind stays VR_RANGE. */
+ min = wide_int_to_tree (type, tmin);
+ max = wide_int_to_tree (type, tmax);
+ }
return;
}
else if ((min_ovf == wi::OVF_UNDERFLOW && max_ovf == wi::OVF_NONE)
@@ -1384,18 +1386,18 @@ set_value_range_with_overflow (value_range &vr,
types values. */
if (covers || wi::cmp (tmin, tmax, sgn) > 0)
{
- set_value_range_to_varying (&vr);
+ kind = VR_VARYING;
return;
}
- vr = value_range (VR_ANTI_RANGE,
- wide_int_to_tree (type, tmin),
- wide_int_to_tree (type, tmax));
+ kind = VR_ANTI_RANGE;
+ min = wide_int_to_tree (type, tmin);
+ max = wide_int_to_tree (type, tmax);
return;
}
else
{
/* Other underflow and/or overflow, drop to VR_VARYING. */
- set_value_range_to_varying (&vr);
+ kind = VR_VARYING;
return;
}
}
@@ -1405,7 +1407,7 @@ set_value_range_with_overflow (value_range &vr,
value. */
wide_int type_min = wi::min_value (prec, sgn);
wide_int type_max = wi::max_value (prec, sgn);
- tree min, max;
+ kind = VR_RANGE;
if (min_ovf == wi::OVF_UNDERFLOW)
min = wide_int_to_tree (type, type_min);
else if (min_ovf == wi::OVF_OVERFLOW)
@@ -1419,7 +1421,6 @@ set_value_range_with_overflow (value_range &vr,
max = wide_int_to_tree (type, type_max);
else
max = wide_int_to_tree (type, wmax);
- vr = value_range (VR_RANGE, min, max);
}
}
@@ -1676,21 +1677,20 @@ extract_range_from_binary_expr_1 (value_range *vr,
}
/* Adjust the range for possible overflow. */
- set_value_range_with_overflow (*vr, expr_type,
+ min = NULL_TREE;
+ max = NULL_TREE;
+ set_value_range_with_overflow (type, min, max, expr_type,
wmin, wmax, min_ovf, max_ovf);
- if (vr->varying_p ())
+ if (type == VR_VARYING)
return;
/* Build the symbolic bounds if needed. */
- min = vr->min ();
- max = vr->max ();
adjust_symbolic_bound (min, code, expr_type,
sym_min_op0, sym_min_op1,
neg_min_op0, neg_min_op1);
adjust_symbolic_bound (max, code, expr_type,
sym_max_op0, sym_max_op1,
neg_max_op0, neg_max_op1);
- type = vr->kind ();
}
else
{
> Richard.
>
> 2018-10-18 Richard Biener <rguenther@suse.de>
>
> * gimple-ssa-evrp-analyze.c
> (evrp_range_analyzer::record_ranges_from_incoming_edge): Be
> smarter about what ranges to use.
> * tree-vrp.c (add_assert_info): Dump here.
> (register_edge_assert_for_2): Instead of here at multiple but
> not all places.
>
> * gcc.dg/tree-ssa/evrp12.c: New testcase.
> * gcc.dg/predict-6.c: Adjust.
> * gcc.dg/tree-ssa/vrp33.c: Disable EVRP.
> * gcc.dg/tree-ssa/vrp02.c: Likewise.
> * gcc.dg/tree-ssa/cunroll-9.c: Likewise.
>
> diff --git a/gcc/gimple-ssa-evrp-analyze.c b/gcc/gimple-ssa-evrp-analyze.c
> index e9afa80e191..0748a53cdb8 100644
> --- a/gcc/gimple-ssa-evrp-analyze.c
> +++ b/gcc/gimple-ssa-evrp-analyze.c
> @@ -206,6 +206,17 @@ evrp_range_analyzer::record_ranges_from_incoming_edge (basic_block bb)
> ordering issues that can lead to worse ranges. */
> for (unsigned i = 0; i < vrs.length (); ++i)
> {
> + /* But make sure we do not weaken ranges like when
> + getting first [64, +INF] and then ~[0, 0] from
> + conditions like (s & 0x3cc0) == 0). */
> + value_range *old_vr = get_value_range (vrs[i].first);
> + value_range tem = *old_vr;
> + tem.equiv = NULL;
> + vrp_intersect_ranges (&tem, vrs[i].second);
> + if (tem.type == old_vr->type
> + && tem.min == old_vr->min
> + && tem.max == old_vr->max)
> + continue;
> push_value_range (vrs[i].first, vrs[i].second);
> if (is_fallthru
> && all_uses_feed_or_dominated_by_stmt (vrs[i].first, stmt))
> diff --git a/gcc/testsuite/gcc.dg/predict-6.c b/gcc/testsuite/gcc.dg/predict-6.c
> index 5d6fbf158f2..08ce5cdb81d 100644
> --- a/gcc/testsuite/gcc.dg/predict-6.c
> +++ b/gcc/testsuite/gcc.dg/predict-6.c
> @@ -10,9 +10,9 @@ void foo (int base, int bound)
> int i, ret = 0;
> for (i = base; i <= bound; i++)
> {
> - if (i < base)
> + if (i <= base)
> global += bar (i);
> - if (i < base + 1)
> + if (i < base + 2)
> global += bar (i);
> if (i <= base + 3)
> global += bar (i);
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c b/gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c
> index 0e4407dcbd7..886dc147ad1 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O2 -fdump-tree-cunrolli-details" } */
> +/* { dg-options "-O2 -fdump-tree-cunrolli-details -fdisable-tree-evrp" } */
> void abort (void);
> int q (void);
> int a[10];
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/evrp12.c b/gcc/testsuite/gcc.dg/tree-ssa/evrp12.c
> new file mode 100644
> index 00000000000..b3906c23465
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/evrp12.c
> @@ -0,0 +1,21 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2 -fdump-tree-evrp" } */
> +
> +extern void link_error ();
> +
> +void
> +f3 (unsigned int s)
> +{
> + if ((s & 0x3cc0) == 0)
> + {
> + if (s >= -15552U)
> + link_error ();
> + }
> + else
> + {
> + if (s <= 0x3f)
> + link_error ();
> + }
> +}
> +
> +/* { dg-final { scan-tree-dump-not "link_error" "evrp" } } */
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp02.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp02.c
> index 8d14feadb6a..4be538f5944 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/vrp02.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp02.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O2 -fdump-tree-vrp1 -fdelete-null-pointer-checks" } */
> +/* { dg-options "-O2 -fdump-tree-vrp1 -fdelete-null-pointer-checks -fdisable-tree-evrp" } */
>
> struct A
> {
> diff --git a/gcc/testsuite/gcc.dg/tree-ssa/vrp33.c b/gcc/testsuite/gcc.dg/tree-ssa/vrp33.c
> index 75fefa49925..f1d3863943e 100644
> --- a/gcc/testsuite/gcc.dg/tree-ssa/vrp33.c
> +++ b/gcc/testsuite/gcc.dg/tree-ssa/vrp33.c
> @@ -1,5 +1,5 @@
> /* { dg-do compile } */
> -/* { dg-options "-O2 -fdump-tree-vrp1 -fno-tree-fre" } */
> +/* { dg-options "-O2 -fdump-tree-vrp1 -fno-tree-fre -fdisable-tree-evrp" } */
>
> /* This is from PR14052. */
>
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index cbc2ea2f26b..6f5ec43670e 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -2133,6 +2133,17 @@ add_assert_info (vec<assert_info> &asserts,
> info.val = val;
> info.expr = expr;
> asserts.safe_push (info);
> +
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file, "Adding assert for ");
> + print_generic_expr (dump_file, name);
> + fprintf (dump_file, " from ");
> + print_generic_expr (dump_file, expr);
> + fprintf (dump_file, " %s ", op_symbol_code (comp_code));
> + print_generic_expr (dump_file, val);
> + fprintf (dump_file, "\n");
> + }
> }
>
> /* If NAME doesn't have an ASSERT_EXPR registered for asserting
> @@ -2532,16 +2543,6 @@ register_edge_assert_for_2 (tree name, edge e,
> tmp = build1 (NOP_EXPR, TREE_TYPE (name), name3);
> if (cst2 != NULL_TREE)
> tmp = build2 (PLUS_EXPR, TREE_TYPE (name), tmp, cst2);
> -
> - if (dump_file)
> - {
> - fprintf (dump_file, "Adding assert for ");
> - print_generic_expr (dump_file, name3);
> - fprintf (dump_file, " from ");
> - print_generic_expr (dump_file, tmp);
> - fprintf (dump_file, "\n");
> - }
> -
> add_assert_info (asserts, name3, tmp, comp_code, val);
> }
>
> @@ -2559,16 +2560,6 @@ register_edge_assert_for_2 (tree name, edge e,
> tmp = build1 (NOP_EXPR, TREE_TYPE (name), tmp);
> if (cst2 != NULL_TREE)
> tmp = build2 (PLUS_EXPR, TREE_TYPE (name), tmp, cst2);
> -
> - if (dump_file)
> - {
> - fprintf (dump_file, "Adding assert for ");
> - print_generic_expr (dump_file, name2);
> - fprintf (dump_file, " from ");
> - print_generic_expr (dump_file, tmp);
> - fprintf (dump_file, "\n");
> - }
> -
> add_assert_info (asserts, name2, tmp, comp_code, val);
> }
> }
> @@ -2691,16 +2682,6 @@ register_edge_assert_for_2 (tree name, edge e,
> cst = fold_build2 (MINUS_EXPR, TREE_TYPE (name2), cst,
> build_int_cst (TREE_TYPE (name2), 1));
> }
> -
> - if (dump_file)
> - {
> - fprintf (dump_file, "Adding assert for ");
> - print_generic_expr (dump_file, name2);
> - fprintf (dump_file, " from ");
> - print_generic_expr (dump_file, tmp);
> - fprintf (dump_file, "\n");
> - }
> -
> add_assert_info (asserts, name2, tmp, new_comp_code, cst);
> }
> }
> @@ -2765,18 +2746,7 @@ register_edge_assert_for_2 (tree name, edge e,
> }
>
> if (new_val)
> - {
> - if (dump_file)
> - {
> - fprintf (dump_file, "Adding assert for ");
> - print_generic_expr (dump_file, name2);
> - fprintf (dump_file, " from ");
> - print_generic_expr (dump_file, tmp);
> - fprintf (dump_file, "\n");
> - }
> -
> - add_assert_info (asserts, name2, tmp, new_comp_code, new_val);
> - }
> + add_assert_info (asserts, name2, tmp, new_comp_code, new_val);
> }
>
> /* Add asserts for NAME cmp CST and NAME being defined as
> @@ -3004,16 +2974,6 @@ register_edge_assert_for_2 (tree name, edge e,
> maxv2 = maxv - minv;
> }
> new_val = wide_int_to_tree (type, maxv2);
> -
> - if (dump_file)
> - {
> - fprintf (dump_file, "Adding assert for ");
> - print_generic_expr (dump_file, names[i]);
> - fprintf (dump_file, " from ");
> - print_generic_expr (dump_file, tmp);
> - fprintf (dump_file, "\n");
> - }
> -
> add_assert_info (asserts, names[i], tmp, LE_EXPR, new_val);
> }
> }
>
--
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)
More information about the Gcc-patches
mailing list