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 <rguent...@suse.de>
> >
> > 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:
<bb 2> :
i_4 = somerandom ();
if (i_4 < 0)
goto <bb 3>; [INV]
else
goto <bb 4>; [INV]
<bb 3> :
__builtin_unreachable ();
<bb 4> :
i.0_1 = (unsigned int) i_4;
if (i.0_1 > 999999)
goto <bb 5>; [INV]
else
goto <bb 6>; [INV]
<bb 5> :
__builtin_unreachable ();
<bb 6> :
_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 <rguent...@suse.de>
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<std::pair <tree, value_range*> > 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" } */
+}
