From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1649 invoked by alias); 28 Nov 2017 09:14:59 -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 1379 invoked by uid 89); 28 Nov 2017 09:14:33 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-10.4 required=5.0 tests=BAYES_00,GIT_PATCH_2,GIT_PATCH_3,KAM_ASCII_DIVIDERS,KAM_NUMSUBJECT,KB_WAM_FROM_NAME_SINGLEWORD,SPF_PASS,T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 spammy= X-HELO: mx2.suse.de Received: from mx2.suse.de (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 28 Nov 2017 09:14:30 +0000 Received: from relay2.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 8DAEEACB7; Tue, 28 Nov 2017 09:14:28 +0000 (UTC) Date: Tue, 28 Nov 2017 09:49:00 -0000 From: Richard Biener To: Jeff Law cc: gcc-patches@gcc.gnu.org Subject: Re: [PATCH] Fix PR80776 In-Reply-To: <3d3fbb88-52a5-4cd9-71c3-5d12cfaaf8c5@redhat.com> Message-ID: References: <3d3fbb88-52a5-4cd9-71c3-5d12cfaaf8c5@redhat.com> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-11/txt/msg02377.txt.bz2 On Mon, 27 Nov 2017, Jeff Law wrote: > On 11/27/2017 06:39 AM, Richard Biener wrote: > > > > The following avoids -Wformat-overflow false positives by teaching > > EVRP the trick about __builtin_unreachable () "other" edges and > > attaching range info to SSA names. EVRP does a better job in keeping > > ranges for every SSA name from conditional info (VRP "optimizes" its > > costly ASSERT_EXPR insertion process). > > > > Bootstrapped on x86_64-unknown-linux-gnu, testing in progress. > > > > This will also fix the testcase from PR83072 but it doesn't really > > fix all cases I want to fix with a fix for it. OTOH it might be > > this is enough for stage3. > > > > Richard. > > > > 2017-11-27 Richard Biener > > > > PR tree-optimization/80776 > > * gimple-ssa-evrp-analyze.h (evrp_range_analyzer::set_ssa_range_info): > > Declare. > > * gimple-ssa-evrp-analyze.c (evrp_range_analyzer::set_ssa_range_info): > > New function. > > (evrp_range_analyzer::record_ranges_from_incoming_edges): > > If the incoming edge is an effective fallthru because the other > > edge only reaches a __builtin_unreachable () then record ranges > > derived from the controlling condition in SSA info. > > (evrp_range_analyzer::record_ranges_from_phis): Use set_ssa_range_info. > > (evrp_range_analyzer::record_ranges_from_stmt): Likewise. > > > > * gcc.dg/pr80776-1.c: New testcase. > > * gcc.dg/pr80776-2.c: Likewise. > So the thing to make sure of here is that the range information we > reflect back into the SSA_NAME actually applies everywhere the SSA_NAME > can appear. ie, it's globally valid. > > This means we can't reflect anything we derive from conditionals or > things like a *p making the range non-null back to the SSA_NAME. > > I'd be concerned about the change to record_ranges_from_incoming_edge. It's basically a copy of what VRP does when removing range assertions. I've added the correctness check I missed and also the trick with setting nonzero bits. This causes us to no longer handle the gcc.dg/pr80776-1.c case: : i_4 = somerandom (); if (i_4 < 0) goto ; [INV] else goto ; [INV] : __builtin_unreachable (); : i.0_1 = (unsigned int) i_4; if (i.0_1 > 999999) goto ; [INV] else goto ; [INV] : __builtin_unreachable (); : _7 = __builtin___sprintf_chk (&number, 1, 7, "%d", i_4); when trying to update the SSA range info for i_4 from the if (i.0_1 > 999999) we see the use in the dominating condition and thus conclude we cannot update the SSA range info like we want. ifcombine also doesn't merge the tests because I think it gets confused by the __builtin_unreachable (). ifcombine also runs after VRP1 which gets rid of the __builtin_unreachable ()s. I think for GCC 9 we may want to experiment with moving ifcombine before VRP1 and handling if-chains with __builtin_unreachable ()s. Anyway, re-testing below. Richard. 2017-11-28 Richard Biener PR tree-optimization/80776 * gimple-ssa-evrp-analyze.h (evrp_range_analyzer::set_ssa_range_info): Declare. * gimple-ssa-evrp-analyze.c (evrp_range_analyzer::set_ssa_range_info): New function. (evrp_range_analyzer::record_ranges_from_incoming_edges): If the incoming edge is an effective fallthru because the other edge only reaches a __builtin_unreachable () then record ranges derived from the controlling condition in SSA info. (evrp_range_analyzer::record_ranges_from_phis): Use set_ssa_range_info. (evrp_range_analyzer::record_ranges_from_stmt): Likewise. * gcc.dg/pr80776-1.c: New testcase. * gcc.dg/pr80776-2.c: Likewise. Index: gcc/gimple-ssa-evrp-analyze.c =================================================================== --- gcc/gimple-ssa-evrp-analyze.c (revision 255172) +++ gcc/gimple-ssa-evrp-analyze.c (working copy) @@ -91,6 +91,62 @@ evrp_range_analyzer::try_find_new_range return NULL; } +/* For LHS record VR in the SSA info. */ +void +evrp_range_analyzer::set_ssa_range_info (tree lhs, value_range *vr) +{ + /* Set the SSA with the value range. */ + if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) + { + if ((vr->type == VR_RANGE + || vr->type == VR_ANTI_RANGE) + && (TREE_CODE (vr->min) == INTEGER_CST) + && (TREE_CODE (vr->max) == INTEGER_CST)) + set_range_info (lhs, vr->type, + wi::to_wide (vr->min), + wi::to_wide (vr->max)); + } + else if (POINTER_TYPE_P (TREE_TYPE (lhs)) + && ((vr->type == VR_RANGE + && range_includes_zero_p (vr->min, + vr->max) == 0) + || (vr->type == VR_ANTI_RANGE + && range_includes_zero_p (vr->min, + vr->max) == 1))) + set_ptr_nonnull (lhs); +} + +/* Return true if all uses of NAME are dominated by STMT or feed STMT + via a chain of single immediate uses. */ + +static bool +all_uses_feed_or_dominated_by_stmt (tree name, gimple *stmt) +{ + use_operand_p use_p, use2_p; + imm_use_iterator iter; + basic_block stmt_bb = gimple_bb (stmt); + + FOR_EACH_IMM_USE_FAST (use_p, iter, name) + { + gimple *use_stmt = USE_STMT (use_p), *use_stmt2; + if (use_stmt == stmt + || is_gimple_debug (use_stmt) + || (gimple_bb (use_stmt) != stmt_bb + && dominated_by_p (CDI_DOMINATORS, + gimple_bb (use_stmt), stmt_bb))) + continue; + while (use_stmt != stmt + && is_gimple_assign (use_stmt) + && TREE_CODE (gimple_assign_lhs (use_stmt)) == SSA_NAME + && single_imm_use (gimple_assign_lhs (use_stmt), + &use2_p, &use_stmt2)) + use_stmt = use_stmt2; + if (use_stmt != stmt) + return false; + } + return true; +} + void evrp_range_analyzer::record_ranges_from_incoming_edge (basic_block bb) { @@ -134,10 +190,23 @@ evrp_range_analyzer::record_ranges_from_ if (vr) vrs.safe_push (std::make_pair (asserts[i].name, vr)); } + + /* If pred_e is really a fallthru we can record value ranges + in SSA names as well. */ + bool is_fallthru = assert_unreachable_fallthru_edge_p (pred_e); + /* Push updated ranges only after finding all of them to avoid ordering issues that can lead to worse ranges. */ for (unsigned i = 0; i < vrs.length (); ++i) - push_value_range (vrs[i].first, vrs[i].second); + { + push_value_range (vrs[i].first, vrs[i].second); + if (is_fallthru + && all_uses_feed_or_dominated_by_stmt (vrs[i].first, stmt)) + { + set_ssa_range_info (vrs[i].first, vrs[i].second); + maybe_set_nonzero_bits (pred_e, vrs[i].first); + } + } } } } @@ -185,24 +254,7 @@ evrp_range_analyzer::record_ranges_from_ vr_values->update_value_range (lhs, &vr_result); /* Set the SSA with the value range. */ - if (INTEGRAL_TYPE_P (TREE_TYPE (lhs))) - { - if ((vr_result.type == VR_RANGE - || vr_result.type == VR_ANTI_RANGE) - && (TREE_CODE (vr_result.min) == INTEGER_CST) - && (TREE_CODE (vr_result.max) == INTEGER_CST)) - set_range_info (lhs, vr_result.type, - wi::to_wide (vr_result.min), - wi::to_wide (vr_result.max)); - } - else if (POINTER_TYPE_P (TREE_TYPE (lhs)) - && ((vr_result.type == VR_RANGE - && range_includes_zero_p (vr_result.min, - vr_result.max) == 0) - || (vr_result.type == VR_ANTI_RANGE - && range_includes_zero_p (vr_result.min, - vr_result.max) == 1))) - set_ptr_nonnull (lhs); + set_ssa_range_info (lhs, &vr_result); } } @@ -224,21 +276,7 @@ evrp_range_analyzer::record_ranges_from_ vr_values->update_value_range (output, &vr); /* Set the SSA with the value range. */ - if (INTEGRAL_TYPE_P (TREE_TYPE (output))) - { - if ((vr.type == VR_RANGE || vr.type == VR_ANTI_RANGE) - && (TREE_CODE (vr.min) == INTEGER_CST) - && (TREE_CODE (vr.max) == INTEGER_CST)) - set_range_info (output, vr.type, - wi::to_wide (vr.min), - wi::to_wide (vr.max)); - } - else if (POINTER_TYPE_P (TREE_TYPE (output)) - && ((vr.type == VR_RANGE - && range_includes_zero_p (vr.min, vr.max) == 0) - || (vr.type == VR_ANTI_RANGE - && range_includes_zero_p (vr.min, vr.max) == 1))) - set_ptr_nonnull (output); + set_ssa_range_info (output, &vr); } else vr_values->set_defs_to_varying (stmt); Index: gcc/gimple-ssa-evrp-analyze.h =================================================================== --- gcc/gimple-ssa-evrp-analyze.h (revision 255172) +++ gcc/gimple-ssa-evrp-analyze.h (working copy) @@ -68,6 +68,7 @@ class evrp_range_analyzer value_range *try_find_new_range (tree, tree op, tree_code code, tree limit); void record_ranges_from_incoming_edge (basic_block); void record_ranges_from_phis (basic_block); + void set_ssa_range_info (tree, value_range *); /* STACK holds the old VR. */ auto_vec > stack; Index: gcc/tree-vrp.c =================================================================== --- gcc/tree-vrp.c (revision 255172) +++ gcc/tree-vrp.c (working copy) @@ -5082,10 +5082,9 @@ all_imm_uses_in_stmt_or_feed_cond (tree var is the x_3 var from ASSERT_EXPR, we can clear low 5 bits from the non-zero bitmask. */ -static void -maybe_set_nonzero_bits (basic_block bb, tree var) +void +maybe_set_nonzero_bits (edge e, tree var) { - edge e = single_pred_edge (bb); basic_block cond_bb = e->src; gimple *stmt = last_stmt (cond_bb); tree cst; @@ -5200,7 +5199,7 @@ remove_range_assertions (void) set_range_info (var, SSA_NAME_RANGE_TYPE (lhs), SSA_NAME_RANGE_INFO (lhs)->get_min (), SSA_NAME_RANGE_INFO (lhs)->get_max ()); - maybe_set_nonzero_bits (bb, var); + maybe_set_nonzero_bits (single_pred_edge (bb), var); } } Index: gcc/tree-vrp.h =================================================================== --- gcc/tree-vrp.h (revision 255172) +++ gcc/tree-vrp.h (working copy) @@ -116,6 +116,7 @@ extern bool overflow_comparison_p (tree_ extern bool range_int_cst_singleton_p (value_range *); extern int value_inside_range (tree, tree, tree); extern tree get_single_symbol (tree, bool *, tree *); +extern void maybe_set_nonzero_bits (edge, tree); struct switch_update { Index: gcc/testsuite/gcc.dg/pr80776-1.c =================================================================== --- gcc/testsuite/gcc.dg/pr80776-1.c (nonexistent) +++ gcc/testsuite/gcc.dg/pr80776-1.c (working copy) @@ -0,0 +1,23 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wformat-overflow" } */ + +extern __inline __attribute__ ((__always_inline__)) __attribute__ ((__gnu_inline__)) __attribute__ ((__artificial__)) int +__attribute__ ((__nothrow__ , __leaf__)) sprintf (char *__restrict __s, const char *__restrict __fmt, ...) +{ + return __builtin___sprintf_chk (__s, 2 - 1, + __builtin_object_size (__s, 2 > 1), __fmt, __builtin_va_arg_pack ()); +} +char number[sizeof "999999"]; +int somerandom (void); +void +Foo (void) +{ + int i = somerandom (); + if (! (0 <= i)) + __builtin_unreachable (); + if (! (0 <= i && i <= 999999)) + __builtin_unreachable (); + /* The correctness bits for [E]VRP cannot handle chained conditionals + when deciding to ignore a unreachable branch for setting SSA range info. */ + sprintf (number, "%d", i); /* { dg-bogus "writing" "" { xfail *-*-* } } */ +} Index: gcc/testsuite/gcc.dg/pr80776-2.c =================================================================== --- gcc/testsuite/gcc.dg/pr80776-2.c (nonexistent) +++ gcc/testsuite/gcc.dg/pr80776-2.c (working copy) @@ -0,0 +1,18 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -Wformat-overflow" } */ + +extern int sprintf (char *restrict, const char *restrict, ...) + __attribute__ ((__nothrow__)); + extern int foo (void); + +int +Fgenerate_new_buffer_name (void) +{ + char number[2]; + int i = foo (); + if (i < 0) + __builtin_unreachable (); + if (i >= 2) + __builtin_unreachable (); + return sprintf (number, "%d", i); /* { dg-bogus "writing" } */ +}