From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 6321 invoked by alias); 22 Oct 2018 13:57:05 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 6308 invoked by uid 89); 22 Oct 2018 13:57:04 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-11.1 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,SPF_PASS autolearn=ham version=3.3.2 spammy=beat, Actually, yielding, cst X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Mon, 22 Oct 2018 13:57:00 +0000 Received: from relay1.suse.de (unknown [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 50543AFD1; Mon, 22 Oct 2018 13:56:58 +0000 (UTC) Date: Mon, 22 Oct 2018 14:08:00 -0000 From: Richard Biener To: Aldy Hernandez , gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix some EVRP stupidness In-Reply-To: Message-ID: References: User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: multipart/mixed; BOUNDARY="-1609908220-872443950-1540216618=:4374" X-SW-Source: 2018-10/txt/msg01327.txt.bz2 This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. ---1609908220-872443950-1540216618=:4374 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Content-length: 10760 On Thu, 18 Oct 2018, Richard Biener wrote: > On October 18, 2018 5:42:56 PM GMT+02:00, Aldy Hernandez 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 * 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 &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); } } ---1609908220-872443950-1540216618=:4374--