[PATCH] Fix some EVRP stupidness
Richard Biener
rguenther@suse.de
Mon Oct 22 14:08:00 GMT 2018
On Thu, 18 Oct 2018, Richard Biener wrote:
> On October 18, 2018 5:42:56 PM GMT+02:00, Aldy Hernandez <aldyh@redhat.com> wrote:
> >
> >
> >On 10/18/18 8:11 AM, Richard Biener wrote:
> >> 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.
> >
> >Sounds reasonable.
>
> Doesn't work and miscompiles all over the place.
>
> >Is this PR87640? Because the testcase there is also crashing while
> >creating the range right before adjusting the symbolics.
>
> Might be.
>
> I'll poke some more tomorrow unless you beat me to it.
This is what I finally applied for the original patch after fixing
the above issue.
Bootstrapped and tested on x86_64-unknown-linux-gnu, applied.
Richard.
2018-10-22 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.
Index: gcc/gimple-ssa-evrp-analyze.c
===================================================================
--- gcc/gimple-ssa-evrp-analyze.c (revision 265381)
+++ gcc/gimple-ssa-evrp-analyze.c (working copy)
@@ -203,6 +203,16 @@ evrp_range_analyzer::record_ranges_from_
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->kind (), old_vr->min (), old_vr->max ());
+ tem.intersect (vrs[i].second);
+ if (tem.kind () == old_vr->kind ()
+ && 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))
Index: gcc/testsuite/gcc.dg/predict-6.c
===================================================================
--- gcc/testsuite/gcc.dg/predict-6.c (revision 265381)
+++ gcc/testsuite/gcc.dg/predict-6.c (working copy)
@@ -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);
Index: gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c (revision 265381)
+++ gcc/testsuite/gcc.dg/tree-ssa/cunroll-9.c (working copy)
@@ -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];
Index: gcc/testsuite/gcc.dg/tree-ssa/evrp12.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/evrp12.c (revision 0)
+++ gcc/testsuite/gcc.dg/tree-ssa/evrp12.c (working copy)
@@ -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" } } */
Index: gcc/testsuite/gcc.dg/tree-ssa/vrp02.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/vrp02.c (revision 265381)
+++ gcc/testsuite/gcc.dg/tree-ssa/vrp02.c (working copy)
@@ -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
{
Index: gcc/testsuite/gcc.dg/tree-ssa/vrp33.c
===================================================================
--- gcc/testsuite/gcc.dg/tree-ssa/vrp33.c (revision 265381)
+++ gcc/testsuite/gcc.dg/tree-ssa/vrp33.c (working copy)
@@ -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. */
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c (revision 265381)
+++ gcc/tree-vrp.c (working copy)
@@ -2299,6 +2299,9 @@ add_assert_info (vec<assert_info> &asser
info.val = val;
info.expr = expr;
asserts.safe_push (info);
+ dump_printf (MSG_NOTE | MSG_PRIORITY_INTERNALS,
+ "Adding assert for %T from %T %s %T\n",
+ name, expr, op_symbol_code (comp_code), val);
}
/* If NAME doesn't have an ASSERT_EXPR registered for asserting
@@ -2698,16 +2701,6 @@ register_edge_assert_for_2 (tree name, 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);
}
@@ -2725,16 +2718,6 @@ register_edge_assert_for_2 (tree name, 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);
}
}
@@ -2857,16 +2840,6 @@ register_edge_assert_for_2 (tree name, 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);
}
}
@@ -2931,18 +2904,7 @@ register_edge_assert_for_2 (tree name, 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
@@ -3170,16 +3132,6 @@ register_edge_assert_for_2 (tree name, 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);
}
}
More information about the Gcc-patches
mailing list