* Add a constant_range_value_p function (PR 92033)
@ 2019-10-11 14:45 Richard Sandiford
2019-10-13 16:22 ` Aldy Hernandez
2019-10-14 11:44 ` Richard Biener
0 siblings, 2 replies; 14+ messages in thread
From: Richard Sandiford @ 2019-10-11 14:45 UTC (permalink / raw)
To: gcc-patches
The range-tracking code has a pretty hard-coded assumption that
is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
ADDR_EXPR". It seems better to add a predicate specifically for
that rather than contiually fight cases in which it can't handle
other invariants.
Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
Richard
2019-10-11 Richard Sandiford <richard.sandiford@arm.com>
gcc/
PR tree-optimization/92033
* tree-vrp.h (constant_range_value_p): Declare.
* tree-vrp.c (constant_range_value_p): New function.
(value_range_base::symbolic_p, value_range_base::singleton_p)
(get_single_symbol, compare_values_warnv, intersect_ranges)
(value_range_base::normalize_symbolics): Use it instead of
is_gimple_min_invariant.
(simplify_stmt_for_jump_threading): Likewise.
* vr-values.c (symbolic_range_based_on_p, valid_value_p): Likewise.
(vr_values::op_with_constant_singleton_value_range): Likewise.
(vr_values::extract_range_from_binary_expr): Likewise.
(vr_values::extract_range_from_unary_expr): Likewise.
(vr_values::extract_range_from_cond_expr): Likewise.
(vr_values::extract_range_from_comparison): Likewise.
(vr_values::extract_range_from_assignment): Likewise.
(vr_values::adjust_range_with_scev, vrp_valueize): Likewise.
(vr_values::vrp_visit_assignment_or_call): Likewise.
(vr_values::vrp_evaluate_conditional): Likewise.
(vr_values::simplify_bit_ops_using_ranges): Likewise.
(test_for_singularity): Likewise.
(vr_values::simplify_cond_using_ranges_1): Likewise.
Index: gcc/tree-vrp.h
===================================================================
--- gcc/tree-vrp.h 2019-10-08 09:23:31.282533990 +0100
+++ gcc/tree-vrp.h 2019-10-11 15:41:20.380576059 +0100
@@ -284,6 +284,7 @@ value_range_base::supports_type_p (tree
return false;
}
+extern bool constant_range_value_p (const_tree);
extern void register_edge_assert_for (tree, edge, enum tree_code,
tree, tree, vec<assert_info> &);
extern bool stmt_interesting_for_vrp (gimple *);
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c 2019-10-08 09:23:31.282533990 +0100
+++ gcc/tree-vrp.c 2019-10-11 15:41:20.380576059 +0100
@@ -78,6 +78,18 @@ ranges_from_anti_range (const value_rang
for still active basic-blocks. */
static sbitmap *live;
+/* Return true if VALUE is considered constant for range tracking.
+ This is stricter than is_gimple_min_invariant and should be
+ used instead of it in range-related code. */
+
+bool
+constant_range_value_p (const_tree value)
+{
+ return (TREE_CODE (value) == INTEGER_CST
+ || (TREE_CODE (value) == ADDR_EXPR
+ && is_gimple_invariant_address (value)));
+}
+
void
value_range::set_equiv (bitmap equiv)
{
@@ -273,8 +285,8 @@ value_range_base::symbolic_p () const
{
return (!varying_p ()
&& !undefined_p ()
- && (!is_gimple_min_invariant (m_min)
- || !is_gimple_min_invariant (m_max)));
+ && (!constant_range_value_p (m_min)
+ || !constant_range_value_p (m_max)));
}
/* NOTE: This is not the inverse of symbolic_p because the range
@@ -388,7 +400,7 @@ value_range_base::singleton_p (tree *res
}
if (m_kind == VR_RANGE
&& vrp_operand_equal_p (min (), max ())
- && is_gimple_min_invariant (min ()))
+ && constant_range_value_p (min ()))
{
if (result)
*result = min ();
@@ -953,13 +965,13 @@ get_single_symbol (tree t, bool *neg, tr
|| TREE_CODE (t) == POINTER_PLUS_EXPR
|| TREE_CODE (t) == MINUS_EXPR)
{
- if (is_gimple_min_invariant (TREE_OPERAND (t, 0)))
+ if (constant_range_value_p (TREE_OPERAND (t, 0)))
{
neg_ = (TREE_CODE (t) == MINUS_EXPR);
inv_ = TREE_OPERAND (t, 0);
t = TREE_OPERAND (t, 1);
}
- else if (is_gimple_min_invariant (TREE_OPERAND (t, 1)))
+ else if (constant_range_value_p (TREE_OPERAND (t, 1)))
{
neg_ = false;
inv_ = TREE_OPERAND (t, 1);
@@ -1106,8 +1118,8 @@ compare_values_warnv (tree val1, tree va
TYPE_SIGN (TREE_TYPE (val1)));
}
- const bool cst1 = is_gimple_min_invariant (val1);
- const bool cst2 = is_gimple_min_invariant (val2);
+ const bool cst1 = constant_range_value_p (val1);
+ const bool cst2 = constant_range_value_p (val2);
/* If one is of the form '[-]NAME + CST' and the other is constant, then
it might be possible to say something depending on the constants. */
@@ -5785,7 +5797,7 @@ intersect_ranges (enum value_range_kind
correct estimate unless VR1 is a constant singleton range
in which case we choose that. */
if (vr1type == VR_RANGE
- && is_gimple_min_invariant (vr1min)
+ && constant_range_value_p (vr1min)
&& vrp_operand_equal_p (vr1min, vr1max))
{
*vr0type = vr1type;
@@ -6045,8 +6057,8 @@ value_range_base::normalize_symbolics ()
if (varying_p () || undefined_p ())
return *this;
tree ttype = type ();
- bool min_symbolic = !is_gimple_min_invariant (min ());
- bool max_symbolic = !is_gimple_min_invariant (max ());
+ bool min_symbolic = !constant_range_value_p (min ());
+ bool max_symbolic = !constant_range_value_p (max ());
if (!min_symbolic && !max_symbolic)
return normalize_addresses ();
@@ -6432,7 +6444,7 @@ simplify_stmt_for_jump_threading (gimple
{
/* First see if the conditional is in the hash table. */
tree cached_lhs = avail_exprs_stack->lookup_avail_expr (stmt, false, true);
- if (cached_lhs && is_gimple_min_invariant (cached_lhs))
+ if (cached_lhs && constant_range_value_p (cached_lhs))
return cached_lhs;
vr_values *vr_values = x_vr_values;
Index: gcc/vr-values.c
===================================================================
--- gcc/vr-values.c 2019-10-03 14:04:54.161520173 +0100
+++ gcc/vr-values.c 2019-10-11 15:41:20.380576059 +0100
@@ -263,14 +263,14 @@ symbolic_range_based_on_p (value_range_b
bool neg, min_has_symbol, max_has_symbol;
tree inv;
- if (is_gimple_min_invariant (vr->min ()))
+ if (constant_range_value_p (vr->min ()))
min_has_symbol = false;
else if (get_single_symbol (vr->min (), &neg, &inv) == sym)
min_has_symbol = true;
else
return false;
- if (is_gimple_min_invariant (vr->max ()))
+ if (constant_range_value_p (vr->max ()))
max_has_symbol = false;
else if (get_single_symbol (vr->max (), &neg, &inv) == sym)
max_has_symbol = true;
@@ -409,7 +409,7 @@ valid_value_p (tree expr)
return (TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
&& TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST);
- return is_gimple_min_invariant (expr);
+ return constant_range_value_p (expr);
}
/* If OP has a value range with a single constant value return that,
@@ -419,7 +419,7 @@ valid_value_p (tree expr)
tree
vr_values::op_with_constant_singleton_value_range (tree op)
{
- if (is_gimple_min_invariant (op))
+ if (constant_range_value_p (op))
return op;
if (TREE_CODE (op) != SSA_NAME)
@@ -778,14 +778,14 @@ vr_values::extract_range_from_binary_exp
value_range_base vr0, vr1;
if (TREE_CODE (op0) == SSA_NAME)
vr0 = *(get_value_range (op0));
- else if (is_gimple_min_invariant (op0))
+ else if (constant_range_value_p (op0))
vr0.set (op0);
else
vr0.set_varying (TREE_TYPE (op0));
if (TREE_CODE (op1) == SSA_NAME)
vr1 = *(get_value_range (op1));
- else if (is_gimple_min_invariant (op1))
+ else if (constant_range_value_p (op1))
vr1.set (op1);
else
vr1.set_varying (TREE_TYPE (op1));
@@ -855,11 +855,11 @@ vr_values::extract_range_from_binary_exp
value_range n_vr1;
/* Try with VR0 and [-INF, OP1]. */
- if (is_gimple_min_invariant (minus_p ? vr0.max () : vr0.min ()))
+ if (constant_range_value_p (minus_p ? vr0.max () : vr0.min ()))
n_vr1.set (VR_RANGE, vrp_val_min (expr_type), op1);
/* Try with VR0 and [OP1, +INF]. */
- else if (is_gimple_min_invariant (minus_p ? vr0.min () : vr0.max ()))
+ else if (constant_range_value_p (minus_p ? vr0.min () : vr0.max ()))
n_vr1.set (VR_RANGE, op1, vrp_val_max (expr_type));
/* Try with VR0 and [OP1, OP1]. */
@@ -879,11 +879,11 @@ vr_values::extract_range_from_binary_exp
value_range n_vr0;
/* Try with [-INF, OP0] and VR1. */
- if (is_gimple_min_invariant (minus_p ? vr1.max () : vr1.min ()))
+ if (constant_range_value_p (minus_p ? vr1.max () : vr1.min ()))
n_vr0.set (VR_RANGE, vrp_val_min (expr_type), op0);
/* Try with [OP0, +INF] and VR1. */
- else if (is_gimple_min_invariant (minus_p ? vr1.min (): vr1.max ()))
+ else if (constant_range_value_p (minus_p ? vr1.min (): vr1.max ()))
n_vr0.set (VR_RANGE, op0, vrp_val_max (expr_type));
/* Try with [OP0, OP0] and VR1. */
@@ -926,7 +926,7 @@ vr_values::extract_range_from_unary_expr
a new value range with the operand to simplify processing. */
if (TREE_CODE (op0) == SSA_NAME)
vr0 = *(get_value_range (op0));
- else if (is_gimple_min_invariant (op0))
+ else if (constant_range_value_p (op0))
vr0.set (op0);
else
vr0.set_varying (type);
@@ -948,7 +948,7 @@ vr_values::extract_range_from_cond_expr
const value_range *vr0 = &tem0;
if (TREE_CODE (op0) == SSA_NAME)
vr0 = get_value_range (op0);
- else if (is_gimple_min_invariant (op0))
+ else if (constant_range_value_p (op0))
tem0.set (op0);
else
tem0.set_varying (TREE_TYPE (op0));
@@ -958,7 +958,7 @@ vr_values::extract_range_from_cond_expr
const value_range *vr1 = &tem1;
if (TREE_CODE (op1) == SSA_NAME)
vr1 = get_value_range (op1);
- else if (is_gimple_min_invariant (op1))
+ else if (constant_range_value_p (op1))
tem1.set (op1);
else
tem1.set_varying (TREE_TYPE (op1));
@@ -987,7 +987,7 @@ vr_values::extract_range_from_comparison
its type may be different from _Bool. Convert VAL to EXPR's
type. */
val = fold_convert (type, val);
- if (is_gimple_min_invariant (val))
+ if (constant_range_value_p (val))
vr->set (val);
else
vr->update (VR_RANGE, val, val);
@@ -1479,7 +1479,7 @@ vr_values::extract_range_from_assignment
gimple_assign_rhs1 (stmt),
gimple_assign_rhs2 (stmt));
else if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS
- && is_gimple_min_invariant (gimple_assign_rhs1 (stmt)))
+ && constant_range_value_p (gimple_assign_rhs1 (stmt)))
vr->set (gimple_assign_rhs1 (stmt));
else
vr->set_varying (TREE_TYPE (gimple_assign_lhs (stmt)));
@@ -1756,7 +1756,7 @@ vr_values::adjust_range_with_scev (value
chrec = instantiate_parameters (loop, analyze_scalar_evolution (loop, var));
/* Like in PR19590, scev can return a constant function. */
- if (is_gimple_min_invariant (chrec))
+ if (constant_range_value_p (chrec))
{
vr->set (chrec);
return;
@@ -1779,7 +1779,7 @@ vr_values::adjust_range_with_scev (value
a simple expression, compare_values and possibly other functions
in tree-vrp won't be able to handle it. */
if (step == NULL_TREE
- || !is_gimple_min_invariant (step)
+ || !constant_range_value_p (step)
|| !valid_value_p (init))
return;
@@ -1841,7 +1841,7 @@ vr_values::adjust_range_with_scev (value
if (TREE_CODE (init) == SSA_NAME)
initvr = *(get_value_range (init));
- else if (is_gimple_min_invariant (init))
+ else if (constant_range_value_p (init))
initvr.set (init);
else
return;
@@ -1995,7 +1995,7 @@ vrp_valueize (tree name)
const value_range *vr = x_vr_values->get_value_range (name);
if (vr->kind () == VR_RANGE
&& (TREE_CODE (vr->min ()) == SSA_NAME
- || is_gimple_min_invariant (vr->min ()))
+ || constant_range_value_p (vr->min ()))
&& vrp_operand_equal_p (vr->min (), vr->max ()))
return vr->min ();
}
@@ -2077,7 +2077,7 @@ vr_values::vrp_visit_assignment_or_call
extract_range_from_ssa_name (vr, tem);
return;
}
- else if (is_gimple_min_invariant (tem))
+ else if (constant_range_value_p (tem))
{
vr->set (tem);
return;
@@ -2475,7 +2475,7 @@ vr_values::vrp_evaluate_conditional (tre
enum warn_strict_overflow_code wc;
const char* warnmsg;
- if (is_gimple_min_invariant (ret))
+ if (constant_range_value_p (ret))
{
wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
warnmsg = G_("assuming signed overflow does not occur when "
@@ -2517,7 +2517,7 @@ vr_values::vrp_evaluate_conditional (tre
&& INTEGRAL_TYPE_P (type)
&& vrp_val_is_min (vr0->min ())
&& vrp_val_is_max (vr0->max ())
- && is_gimple_min_invariant (op1))
+ && constant_range_value_p (op1))
{
location_t location;
@@ -3361,14 +3361,14 @@ vr_values::simplify_bit_ops_using_ranges
if (TREE_CODE (op0) == SSA_NAME)
vr0 = *(get_value_range (op0));
- else if (is_gimple_min_invariant (op0))
+ else if (constant_range_value_p (op0))
vr0.set (op0);
else
return false;
if (TREE_CODE (op1) == SSA_NAME)
vr1 = *(get_value_range (op1));
- else if (is_gimple_min_invariant (op1))
+ else if (constant_range_value_p (op1))
vr1.set (op1);
else
return false;
@@ -3481,7 +3481,7 @@ test_for_singularity (enum tree_code con
/* If the new min/max values have converged to a single value,
then there is only one value which can satisfy the condition,
return that value. */
- if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
+ if (operand_equal_p (min, max, 0) && constant_range_value_p (min))
return min;
}
return NULL;
@@ -3554,7 +3554,7 @@ vr_values::simplify_cond_using_ranges_1
&& cond_code != EQ_EXPR
&& TREE_CODE (op0) == SSA_NAME
&& INTEGRAL_TYPE_P (TREE_TYPE (op0))
- && is_gimple_min_invariant (op1))
+ && constant_range_value_p (op1))
{
const value_range *vr = get_value_range (op0);
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-11 14:45 Add a constant_range_value_p function (PR 92033) Richard Sandiford
@ 2019-10-13 16:22 ` Aldy Hernandez
2019-10-14 8:34 ` Richard Sandiford
2019-10-14 11:44 ` Richard Biener
1 sibling, 1 reply; 14+ messages in thread
From: Aldy Hernandez @ 2019-10-13 16:22 UTC (permalink / raw)
To: gcc-patches, richard.sandiford
On 10/11/19 10:42 AM, Richard Sandiford wrote:
> The range-tracking code has a pretty hard-coded assumption that
> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
> ADDR_EXPR". It seems better to add a predicate specifically for
> that rather than contiually fight cases in which it can't handle
> other invariants.
I was going to suggest we normalize ranges to numerics completely before
folding. That is, replacing normalize_addresses() here:
*vr = op->fold_range (expr_type,
vr0.normalize_addresses (),
vr1.normalize_addresses ());
...into normalize_symbolics(). But I suppose getting the gate correct
is even better. Thanks for taking the care of this extensive and manual
change.
The patch looks good to me. However, I do wonder if VRP and
subsidiaries can't handle non-integer invariants, if we shouldn't
disallow them from the setters as well:
void
value_range_base::set (tree val)
{
gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
(val));
if (TREE_OVERFLOW_P (val))
val = drop_tree_overflow (val);
set (VR_RANGE, val, val);
}
void
value_range::set (tree val)
{
gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
(val));
if (TREE_OVERFLOW_P (val))
val = drop_tree_overflow (val);
set (VR_RANGE, val, val, NULL);
}
This would still allow setting of VARYING and UNDEFINED, but disallow
poly-ints, etc from a range.
Was this a small oversight, or was there a reason you left those in?
Aldy
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>
> Richard
>
>
> 2019-10-11 Richard Sandiford <richard.sandiford@arm.com>
>
> gcc/
> PR tree-optimization/92033
> * tree-vrp.h (constant_range_value_p): Declare.
> * tree-vrp.c (constant_range_value_p): New function.
> (value_range_base::symbolic_p, value_range_base::singleton_p)
> (get_single_symbol, compare_values_warnv, intersect_ranges)
> (value_range_base::normalize_symbolics): Use it instead of
> is_gimple_min_invariant.
> (simplify_stmt_for_jump_threading): Likewise.
> * vr-values.c (symbolic_range_based_on_p, valid_value_p): Likewise.
> (vr_values::op_with_constant_singleton_value_range): Likewise.
> (vr_values::extract_range_from_binary_expr): Likewise.
> (vr_values::extract_range_from_unary_expr): Likewise.
> (vr_values::extract_range_from_cond_expr): Likewise.
> (vr_values::extract_range_from_comparison): Likewise.
> (vr_values::extract_range_from_assignment): Likewise.
> (vr_values::adjust_range_with_scev, vrp_valueize): Likewise.
> (vr_values::vrp_visit_assignment_or_call): Likewise.
> (vr_values::vrp_evaluate_conditional): Likewise.
> (vr_values::simplify_bit_ops_using_ranges): Likewise.
> (test_for_singularity): Likewise.
> (vr_values::simplify_cond_using_ranges_1): Likewise.
>
> Index: gcc/tree-vrp.h
> ===================================================================
> --- gcc/tree-vrp.h 2019-10-08 09:23:31.282533990 +0100
> +++ gcc/tree-vrp.h 2019-10-11 15:41:20.380576059 +0100
> @@ -284,6 +284,7 @@ value_range_base::supports_type_p (tree
> return false;
> }
>
> +extern bool constant_range_value_p (const_tree);
> extern void register_edge_assert_for (tree, edge, enum tree_code,
> tree, tree, vec<assert_info> &);
> extern bool stmt_interesting_for_vrp (gimple *);
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c 2019-10-08 09:23:31.282533990 +0100
> +++ gcc/tree-vrp.c 2019-10-11 15:41:20.380576059 +0100
> @@ -78,6 +78,18 @@ ranges_from_anti_range (const value_rang
> for still active basic-blocks. */
> static sbitmap *live;
>
> +/* Return true if VALUE is considered constant for range tracking.
> + This is stricter than is_gimple_min_invariant and should be
> + used instead of it in range-related code. */
> +
> +bool
> +constant_range_value_p (const_tree value)
> +{
> + return (TREE_CODE (value) == INTEGER_CST
> + || (TREE_CODE (value) == ADDR_EXPR
> + && is_gimple_invariant_address (value)));
> +}
> +
> void
> value_range::set_equiv (bitmap equiv)
> {
> @@ -273,8 +285,8 @@ value_range_base::symbolic_p () const
> {
> return (!varying_p ()
> && !undefined_p ()
> - && (!is_gimple_min_invariant (m_min)
> - || !is_gimple_min_invariant (m_max)));
> + && (!constant_range_value_p (m_min)
> + || !constant_range_value_p (m_max)));
> }
>
> /* NOTE: This is not the inverse of symbolic_p because the range
> @@ -388,7 +400,7 @@ value_range_base::singleton_p (tree *res
> }
> if (m_kind == VR_RANGE
> && vrp_operand_equal_p (min (), max ())
> - && is_gimple_min_invariant (min ()))
> + && constant_range_value_p (min ()))
> {
> if (result)
> *result = min ();
> @@ -953,13 +965,13 @@ get_single_symbol (tree t, bool *neg, tr
> || TREE_CODE (t) == POINTER_PLUS_EXPR
> || TREE_CODE (t) == MINUS_EXPR)
> {
> - if (is_gimple_min_invariant (TREE_OPERAND (t, 0)))
> + if (constant_range_value_p (TREE_OPERAND (t, 0)))
> {
> neg_ = (TREE_CODE (t) == MINUS_EXPR);
> inv_ = TREE_OPERAND (t, 0);
> t = TREE_OPERAND (t, 1);
> }
> - else if (is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> + else if (constant_range_value_p (TREE_OPERAND (t, 1)))
> {
> neg_ = false;
> inv_ = TREE_OPERAND (t, 1);
> @@ -1106,8 +1118,8 @@ compare_values_warnv (tree val1, tree va
> TYPE_SIGN (TREE_TYPE (val1)));
> }
>
> - const bool cst1 = is_gimple_min_invariant (val1);
> - const bool cst2 = is_gimple_min_invariant (val2);
> + const bool cst1 = constant_range_value_p (val1);
> + const bool cst2 = constant_range_value_p (val2);
>
> /* If one is of the form '[-]NAME + CST' and the other is constant, then
> it might be possible to say something depending on the constants. */
> @@ -5785,7 +5797,7 @@ intersect_ranges (enum value_range_kind
> correct estimate unless VR1 is a constant singleton range
> in which case we choose that. */
> if (vr1type == VR_RANGE
> - && is_gimple_min_invariant (vr1min)
> + && constant_range_value_p (vr1min)
> && vrp_operand_equal_p (vr1min, vr1max))
> {
> *vr0type = vr1type;
> @@ -6045,8 +6057,8 @@ value_range_base::normalize_symbolics ()
> if (varying_p () || undefined_p ())
> return *this;
> tree ttype = type ();
> - bool min_symbolic = !is_gimple_min_invariant (min ());
> - bool max_symbolic = !is_gimple_min_invariant (max ());
> + bool min_symbolic = !constant_range_value_p (min ());
> + bool max_symbolic = !constant_range_value_p (max ());
> if (!min_symbolic && !max_symbolic)
> return normalize_addresses ();
>
> @@ -6432,7 +6444,7 @@ simplify_stmt_for_jump_threading (gimple
> {
> /* First see if the conditional is in the hash table. */
> tree cached_lhs = avail_exprs_stack->lookup_avail_expr (stmt, false, true);
> - if (cached_lhs && is_gimple_min_invariant (cached_lhs))
> + if (cached_lhs && constant_range_value_p (cached_lhs))
> return cached_lhs;
>
> vr_values *vr_values = x_vr_values;
> Index: gcc/vr-values.c
> ===================================================================
> --- gcc/vr-values.c 2019-10-03 14:04:54.161520173 +0100
> +++ gcc/vr-values.c 2019-10-11 15:41:20.380576059 +0100
> @@ -263,14 +263,14 @@ symbolic_range_based_on_p (value_range_b
> bool neg, min_has_symbol, max_has_symbol;
> tree inv;
>
> - if (is_gimple_min_invariant (vr->min ()))
> + if (constant_range_value_p (vr->min ()))
> min_has_symbol = false;
> else if (get_single_symbol (vr->min (), &neg, &inv) == sym)
> min_has_symbol = true;
> else
> return false;
>
> - if (is_gimple_min_invariant (vr->max ()))
> + if (constant_range_value_p (vr->max ()))
> max_has_symbol = false;
> else if (get_single_symbol (vr->max (), &neg, &inv) == sym)
> max_has_symbol = true;
> @@ -409,7 +409,7 @@ valid_value_p (tree expr)
> return (TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
> && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST);
>
> - return is_gimple_min_invariant (expr);
> + return constant_range_value_p (expr);
> }
>
> /* If OP has a value range with a single constant value return that,
> @@ -419,7 +419,7 @@ valid_value_p (tree expr)
> tree
> vr_values::op_with_constant_singleton_value_range (tree op)
> {
> - if (is_gimple_min_invariant (op))
> + if (constant_range_value_p (op))
> return op;
>
> if (TREE_CODE (op) != SSA_NAME)
> @@ -778,14 +778,14 @@ vr_values::extract_range_from_binary_exp
> value_range_base vr0, vr1;
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> vr0.set_varying (TREE_TYPE (op0));
>
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = *(get_value_range (op1));
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> vr1.set (op1);
> else
> vr1.set_varying (TREE_TYPE (op1));
> @@ -855,11 +855,11 @@ vr_values::extract_range_from_binary_exp
> value_range n_vr1;
>
> /* Try with VR0 and [-INF, OP1]. */
> - if (is_gimple_min_invariant (minus_p ? vr0.max () : vr0.min ()))
> + if (constant_range_value_p (minus_p ? vr0.max () : vr0.min ()))
> n_vr1.set (VR_RANGE, vrp_val_min (expr_type), op1);
>
> /* Try with VR0 and [OP1, +INF]. */
> - else if (is_gimple_min_invariant (minus_p ? vr0.min () : vr0.max ()))
> + else if (constant_range_value_p (minus_p ? vr0.min () : vr0.max ()))
> n_vr1.set (VR_RANGE, op1, vrp_val_max (expr_type));
>
> /* Try with VR0 and [OP1, OP1]. */
> @@ -879,11 +879,11 @@ vr_values::extract_range_from_binary_exp
> value_range n_vr0;
>
> /* Try with [-INF, OP0] and VR1. */
> - if (is_gimple_min_invariant (minus_p ? vr1.max () : vr1.min ()))
> + if (constant_range_value_p (minus_p ? vr1.max () : vr1.min ()))
> n_vr0.set (VR_RANGE, vrp_val_min (expr_type), op0);
>
> /* Try with [OP0, +INF] and VR1. */
> - else if (is_gimple_min_invariant (minus_p ? vr1.min (): vr1.max ()))
> + else if (constant_range_value_p (minus_p ? vr1.min (): vr1.max ()))
> n_vr0.set (VR_RANGE, op0, vrp_val_max (expr_type));
>
> /* Try with [OP0, OP0] and VR1. */
> @@ -926,7 +926,7 @@ vr_values::extract_range_from_unary_expr
> a new value range with the operand to simplify processing. */
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> vr0.set_varying (type);
> @@ -948,7 +948,7 @@ vr_values::extract_range_from_cond_expr
> const value_range *vr0 = &tem0;
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = get_value_range (op0);
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> tem0.set (op0);
> else
> tem0.set_varying (TREE_TYPE (op0));
> @@ -958,7 +958,7 @@ vr_values::extract_range_from_cond_expr
> const value_range *vr1 = &tem1;
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = get_value_range (op1);
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> tem1.set (op1);
> else
> tem1.set_varying (TREE_TYPE (op1));
> @@ -987,7 +987,7 @@ vr_values::extract_range_from_comparison
> its type may be different from _Bool. Convert VAL to EXPR's
> type. */
> val = fold_convert (type, val);
> - if (is_gimple_min_invariant (val))
> + if (constant_range_value_p (val))
> vr->set (val);
> else
> vr->update (VR_RANGE, val, val);
> @@ -1479,7 +1479,7 @@ vr_values::extract_range_from_assignment
> gimple_assign_rhs1 (stmt),
> gimple_assign_rhs2 (stmt));
> else if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS
> - && is_gimple_min_invariant (gimple_assign_rhs1 (stmt)))
> + && constant_range_value_p (gimple_assign_rhs1 (stmt)))
> vr->set (gimple_assign_rhs1 (stmt));
> else
> vr->set_varying (TREE_TYPE (gimple_assign_lhs (stmt)));
> @@ -1756,7 +1756,7 @@ vr_values::adjust_range_with_scev (value
> chrec = instantiate_parameters (loop, analyze_scalar_evolution (loop, var));
>
> /* Like in PR19590, scev can return a constant function. */
> - if (is_gimple_min_invariant (chrec))
> + if (constant_range_value_p (chrec))
> {
> vr->set (chrec);
> return;
> @@ -1779,7 +1779,7 @@ vr_values::adjust_range_with_scev (value
> a simple expression, compare_values and possibly other functions
> in tree-vrp won't be able to handle it. */
> if (step == NULL_TREE
> - || !is_gimple_min_invariant (step)
> + || !constant_range_value_p (step)
> || !valid_value_p (init))
> return;
>
> @@ -1841,7 +1841,7 @@ vr_values::adjust_range_with_scev (value
>
> if (TREE_CODE (init) == SSA_NAME)
> initvr = *(get_value_range (init));
> - else if (is_gimple_min_invariant (init))
> + else if (constant_range_value_p (init))
> initvr.set (init);
> else
> return;
> @@ -1995,7 +1995,7 @@ vrp_valueize (tree name)
> const value_range *vr = x_vr_values->get_value_range (name);
> if (vr->kind () == VR_RANGE
> && (TREE_CODE (vr->min ()) == SSA_NAME
> - || is_gimple_min_invariant (vr->min ()))
> + || constant_range_value_p (vr->min ()))
> && vrp_operand_equal_p (vr->min (), vr->max ()))
> return vr->min ();
> }
> @@ -2077,7 +2077,7 @@ vr_values::vrp_visit_assignment_or_call
> extract_range_from_ssa_name (vr, tem);
> return;
> }
> - else if (is_gimple_min_invariant (tem))
> + else if (constant_range_value_p (tem))
> {
> vr->set (tem);
> return;
> @@ -2475,7 +2475,7 @@ vr_values::vrp_evaluate_conditional (tre
> enum warn_strict_overflow_code wc;
> const char* warnmsg;
>
> - if (is_gimple_min_invariant (ret))
> + if (constant_range_value_p (ret))
> {
> wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
> warnmsg = G_("assuming signed overflow does not occur when "
> @@ -2517,7 +2517,7 @@ vr_values::vrp_evaluate_conditional (tre
> && INTEGRAL_TYPE_P (type)
> && vrp_val_is_min (vr0->min ())
> && vrp_val_is_max (vr0->max ())
> - && is_gimple_min_invariant (op1))
> + && constant_range_value_p (op1))
> {
> location_t location;
>
> @@ -3361,14 +3361,14 @@ vr_values::simplify_bit_ops_using_ranges
>
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> return false;
>
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = *(get_value_range (op1));
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> vr1.set (op1);
> else
> return false;
> @@ -3481,7 +3481,7 @@ test_for_singularity (enum tree_code con
> /* If the new min/max values have converged to a single value,
> then there is only one value which can satisfy the condition,
> return that value. */
> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> + if (operand_equal_p (min, max, 0) && constant_range_value_p (min))
> return min;
> }
> return NULL;
> @@ -3554,7 +3554,7 @@ vr_values::simplify_cond_using_ranges_1
> && cond_code != EQ_EXPR
> && TREE_CODE (op0) == SSA_NAME
> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> - && is_gimple_min_invariant (op1))
> + && constant_range_value_p (op1))
> {
> const value_range *vr = get_value_range (op0);
>
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-13 16:22 ` Aldy Hernandez
@ 2019-10-14 8:34 ` Richard Sandiford
2019-10-14 10:01 ` Aldy Hernandez
0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2019-10-14 8:34 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches
Aldy Hernandez <aldyh@redhat.com> writes:
> On 10/11/19 10:42 AM, Richard Sandiford wrote:
>> The range-tracking code has a pretty hard-coded assumption that
>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>> ADDR_EXPR". It seems better to add a predicate specifically for
>> that rather than contiually fight cases in which it can't handle
>> other invariants.
>
> I was going to suggest we normalize ranges to numerics completely before
> folding. That is, replacing normalize_addresses() here:
>
> *vr = op->fold_range (expr_type,
> vr0.normalize_addresses (),
> vr1.normalize_addresses ());
>
> ...into normalize_symbolics(). But I suppose getting the gate correct
> is even better. Thanks for taking the care of this extensive and manual
> change.
>
> The patch looks good to me. However, I do wonder if VRP and
> subsidiaries can't handle non-integer invariants, if we shouldn't
> disallow them from the setters as well:
>
> void
> value_range_base::set (tree val)
> {
> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
> (val));
> if (TREE_OVERFLOW_P (val))
> val = drop_tree_overflow (val);
> set (VR_RANGE, val, val);
> }
>
> void
> value_range::set (tree val)
> {
> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
> (val));
> if (TREE_OVERFLOW_P (val))
> val = drop_tree_overflow (val);
> set (VR_RANGE, val, val, NULL);
> }
>
> This would still allow setting of VARYING and UNDEFINED, but disallow
> poly-ints, etc from a range.
>
> Was this a small oversight, or was there a reason you left those in?
Yeah, this was intentional. The patch is effectively treating
POLY_INY_CST as symbolic rather than constant. (It's really somewhere
in the middle: it's at least a function invariant, but the invariant
depends on a runtime target property.) So places like here that
can handle both symbolics and constants should be able to handle
POLY_INT_CST wihout problems. We just need to make sure that
POLY_INT_CSTs aren't treated as constants for range tracking,
because they're not "constant enough" there.
Thanks,
Richard
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 8:34 ` Richard Sandiford
@ 2019-10-14 10:01 ` Aldy Hernandez
2019-10-14 12:32 ` Richard Sandiford
0 siblings, 1 reply; 14+ messages in thread
From: Aldy Hernandez @ 2019-10-14 10:01 UTC (permalink / raw)
To: gcc-patches, richard.sandiford
On 10/14/19 4:30 AM, Richard Sandiford wrote:
> Aldy Hernandez <aldyh@redhat.com> writes:
>> On 10/11/19 10:42 AM, Richard Sandiford wrote:
>>> The range-tracking code has a pretty hard-coded assumption that
>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>> that rather than contiually fight cases in which it can't handle
>>> other invariants.
>>
>> I was going to suggest we normalize ranges to numerics completely before
>> folding. That is, replacing normalize_addresses() here:
>>
>> *vr = op->fold_range (expr_type,
>> vr0.normalize_addresses (),
>> vr1.normalize_addresses ());
>>
>> ...into normalize_symbolics(). But I suppose getting the gate correct
>> is even better. Thanks for taking the care of this extensive and manual
>> change.
>>
>> The patch looks good to me. However, I do wonder if VRP and
>> subsidiaries can't handle non-integer invariants, if we shouldn't
>> disallow them from the setters as well:
>>
>> void
>> value_range_base::set (tree val)
>> {
>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>> (val));
>> if (TREE_OVERFLOW_P (val))
>> val = drop_tree_overflow (val);
>> set (VR_RANGE, val, val);
>> }
>>
>> void
>> value_range::set (tree val)
>> {
>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>> (val));
>> if (TREE_OVERFLOW_P (val))
>> val = drop_tree_overflow (val);
>> set (VR_RANGE, val, val, NULL);
>> }
>>
>> This would still allow setting of VARYING and UNDEFINED, but disallow
>> poly-ints, etc from a range.
>>
>> Was this a small oversight, or was there a reason you left those in?
>
> Yeah, this was intentional. The patch is effectively treating
> POLY_INY_CST as symbolic rather than constant. (It's really somewhere
> in the middle: it's at least a function invariant, but the invariant
> depends on a runtime target property.) So places like here that
> can handle both symbolics and constants should be able to handle
> POLY_INT_CST wihout problems. We just need to make sure that
> POLY_INT_CSTs aren't treated as constants for range tracking,
> because they're not "constant enough" there.
Hmmm... We don't handle POLY_INT_CST value_range's anywhere, so perhaps
it's better to stop their creation at the source and fix the caller, to
inhibit their proliferation. AFAICT, the only POLY_INT_CST ranges that
would be used are VARYING and UNDEFINED (for the lattice), and that
already works.
If you don't agree, then at least a comment in ::set() would be nice, to
document that we're allowing their creation and why.
Thanks.
Aldy
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-11 14:45 Add a constant_range_value_p function (PR 92033) Richard Sandiford
2019-10-13 16:22 ` Aldy Hernandez
@ 2019-10-14 11:44 ` Richard Biener
2019-10-14 12:49 ` Richard Sandiford
1 sibling, 1 reply; 14+ messages in thread
From: Richard Biener @ 2019-10-14 11:44 UTC (permalink / raw)
To: Richard Sandiford; +Cc: GCC Patches
On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> The range-tracking code has a pretty hard-coded assumption that
> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
> ADDR_EXPR". It seems better to add a predicate specifically for
> that rather than contiually fight cases in which it can't handle
> other invariants.
>
> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
ICK. Nobody is going to remember this new restriction and
constant_range_value_p reads like constant_value_range_p ;)
Btw, is_gimple_invariant_address shouldn't have been exported,
it's only use could have used is_gimple_min_invariant...
Richard.
> Richard
>
>
> 2019-10-11 Richard Sandiford <richard.sandiford@arm.com>
>
> gcc/
> PR tree-optimization/92033
> * tree-vrp.h (constant_range_value_p): Declare.
> * tree-vrp.c (constant_range_value_p): New function.
> (value_range_base::symbolic_p, value_range_base::singleton_p)
> (get_single_symbol, compare_values_warnv, intersect_ranges)
> (value_range_base::normalize_symbolics): Use it instead of
> is_gimple_min_invariant.
> (simplify_stmt_for_jump_threading): Likewise.
> * vr-values.c (symbolic_range_based_on_p, valid_value_p): Likewise.
> (vr_values::op_with_constant_singleton_value_range): Likewise.
> (vr_values::extract_range_from_binary_expr): Likewise.
> (vr_values::extract_range_from_unary_expr): Likewise.
> (vr_values::extract_range_from_cond_expr): Likewise.
> (vr_values::extract_range_from_comparison): Likewise.
> (vr_values::extract_range_from_assignment): Likewise.
> (vr_values::adjust_range_with_scev, vrp_valueize): Likewise.
> (vr_values::vrp_visit_assignment_or_call): Likewise.
> (vr_values::vrp_evaluate_conditional): Likewise.
> (vr_values::simplify_bit_ops_using_ranges): Likewise.
> (test_for_singularity): Likewise.
> (vr_values::simplify_cond_using_ranges_1): Likewise.
>
> Index: gcc/tree-vrp.h
> ===================================================================
> --- gcc/tree-vrp.h 2019-10-08 09:23:31.282533990 +0100
> +++ gcc/tree-vrp.h 2019-10-11 15:41:20.380576059 +0100
> @@ -284,6 +284,7 @@ value_range_base::supports_type_p (tree
> return false;
> }
>
> +extern bool constant_range_value_p (const_tree);
> extern void register_edge_assert_for (tree, edge, enum tree_code,
> tree, tree, vec<assert_info> &);
> extern bool stmt_interesting_for_vrp (gimple *);
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c 2019-10-08 09:23:31.282533990 +0100
> +++ gcc/tree-vrp.c 2019-10-11 15:41:20.380576059 +0100
> @@ -78,6 +78,18 @@ ranges_from_anti_range (const value_rang
> for still active basic-blocks. */
> static sbitmap *live;
>
> +/* Return true if VALUE is considered constant for range tracking.
> + This is stricter than is_gimple_min_invariant and should be
> + used instead of it in range-related code. */
> +
> +bool
> +constant_range_value_p (const_tree value)
> +{
> + return (TREE_CODE (value) == INTEGER_CST
> + || (TREE_CODE (value) == ADDR_EXPR
> + && is_gimple_invariant_address (value)));
> +}
> +
> void
> value_range::set_equiv (bitmap equiv)
> {
> @@ -273,8 +285,8 @@ value_range_base::symbolic_p () const
> {
> return (!varying_p ()
> && !undefined_p ()
> - && (!is_gimple_min_invariant (m_min)
> - || !is_gimple_min_invariant (m_max)));
> + && (!constant_range_value_p (m_min)
> + || !constant_range_value_p (m_max)));
> }
>
> /* NOTE: This is not the inverse of symbolic_p because the range
> @@ -388,7 +400,7 @@ value_range_base::singleton_p (tree *res
> }
> if (m_kind == VR_RANGE
> && vrp_operand_equal_p (min (), max ())
> - && is_gimple_min_invariant (min ()))
> + && constant_range_value_p (min ()))
> {
> if (result)
> *result = min ();
> @@ -953,13 +965,13 @@ get_single_symbol (tree t, bool *neg, tr
> || TREE_CODE (t) == POINTER_PLUS_EXPR
> || TREE_CODE (t) == MINUS_EXPR)
> {
> - if (is_gimple_min_invariant (TREE_OPERAND (t, 0)))
> + if (constant_range_value_p (TREE_OPERAND (t, 0)))
> {
> neg_ = (TREE_CODE (t) == MINUS_EXPR);
> inv_ = TREE_OPERAND (t, 0);
> t = TREE_OPERAND (t, 1);
> }
> - else if (is_gimple_min_invariant (TREE_OPERAND (t, 1)))
> + else if (constant_range_value_p (TREE_OPERAND (t, 1)))
> {
> neg_ = false;
> inv_ = TREE_OPERAND (t, 1);
> @@ -1106,8 +1118,8 @@ compare_values_warnv (tree val1, tree va
> TYPE_SIGN (TREE_TYPE (val1)));
> }
>
> - const bool cst1 = is_gimple_min_invariant (val1);
> - const bool cst2 = is_gimple_min_invariant (val2);
> + const bool cst1 = constant_range_value_p (val1);
> + const bool cst2 = constant_range_value_p (val2);
>
> /* If one is of the form '[-]NAME + CST' and the other is constant, then
> it might be possible to say something depending on the constants. */
> @@ -5785,7 +5797,7 @@ intersect_ranges (enum value_range_kind
> correct estimate unless VR1 is a constant singleton range
> in which case we choose that. */
> if (vr1type == VR_RANGE
> - && is_gimple_min_invariant (vr1min)
> + && constant_range_value_p (vr1min)
> && vrp_operand_equal_p (vr1min, vr1max))
> {
> *vr0type = vr1type;
> @@ -6045,8 +6057,8 @@ value_range_base::normalize_symbolics ()
> if (varying_p () || undefined_p ())
> return *this;
> tree ttype = type ();
> - bool min_symbolic = !is_gimple_min_invariant (min ());
> - bool max_symbolic = !is_gimple_min_invariant (max ());
> + bool min_symbolic = !constant_range_value_p (min ());
> + bool max_symbolic = !constant_range_value_p (max ());
> if (!min_symbolic && !max_symbolic)
> return normalize_addresses ();
>
> @@ -6432,7 +6444,7 @@ simplify_stmt_for_jump_threading (gimple
> {
> /* First see if the conditional is in the hash table. */
> tree cached_lhs = avail_exprs_stack->lookup_avail_expr (stmt, false, true);
> - if (cached_lhs && is_gimple_min_invariant (cached_lhs))
> + if (cached_lhs && constant_range_value_p (cached_lhs))
> return cached_lhs;
>
> vr_values *vr_values = x_vr_values;
> Index: gcc/vr-values.c
> ===================================================================
> --- gcc/vr-values.c 2019-10-03 14:04:54.161520173 +0100
> +++ gcc/vr-values.c 2019-10-11 15:41:20.380576059 +0100
> @@ -263,14 +263,14 @@ symbolic_range_based_on_p (value_range_b
> bool neg, min_has_symbol, max_has_symbol;
> tree inv;
>
> - if (is_gimple_min_invariant (vr->min ()))
> + if (constant_range_value_p (vr->min ()))
> min_has_symbol = false;
> else if (get_single_symbol (vr->min (), &neg, &inv) == sym)
> min_has_symbol = true;
> else
> return false;
>
> - if (is_gimple_min_invariant (vr->max ()))
> + if (constant_range_value_p (vr->max ()))
> max_has_symbol = false;
> else if (get_single_symbol (vr->max (), &neg, &inv) == sym)
> max_has_symbol = true;
> @@ -409,7 +409,7 @@ valid_value_p (tree expr)
> return (TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
> && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST);
>
> - return is_gimple_min_invariant (expr);
> + return constant_range_value_p (expr);
> }
>
> /* If OP has a value range with a single constant value return that,
> @@ -419,7 +419,7 @@ valid_value_p (tree expr)
> tree
> vr_values::op_with_constant_singleton_value_range (tree op)
> {
> - if (is_gimple_min_invariant (op))
> + if (constant_range_value_p (op))
> return op;
>
> if (TREE_CODE (op) != SSA_NAME)
> @@ -778,14 +778,14 @@ vr_values::extract_range_from_binary_exp
> value_range_base vr0, vr1;
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> vr0.set_varying (TREE_TYPE (op0));
>
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = *(get_value_range (op1));
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> vr1.set (op1);
> else
> vr1.set_varying (TREE_TYPE (op1));
> @@ -855,11 +855,11 @@ vr_values::extract_range_from_binary_exp
> value_range n_vr1;
>
> /* Try with VR0 and [-INF, OP1]. */
> - if (is_gimple_min_invariant (minus_p ? vr0.max () : vr0.min ()))
> + if (constant_range_value_p (minus_p ? vr0.max () : vr0.min ()))
> n_vr1.set (VR_RANGE, vrp_val_min (expr_type), op1);
>
> /* Try with VR0 and [OP1, +INF]. */
> - else if (is_gimple_min_invariant (minus_p ? vr0.min () : vr0.max ()))
> + else if (constant_range_value_p (minus_p ? vr0.min () : vr0.max ()))
> n_vr1.set (VR_RANGE, op1, vrp_val_max (expr_type));
>
> /* Try with VR0 and [OP1, OP1]. */
> @@ -879,11 +879,11 @@ vr_values::extract_range_from_binary_exp
> value_range n_vr0;
>
> /* Try with [-INF, OP0] and VR1. */
> - if (is_gimple_min_invariant (minus_p ? vr1.max () : vr1.min ()))
> + if (constant_range_value_p (minus_p ? vr1.max () : vr1.min ()))
> n_vr0.set (VR_RANGE, vrp_val_min (expr_type), op0);
>
> /* Try with [OP0, +INF] and VR1. */
> - else if (is_gimple_min_invariant (minus_p ? vr1.min (): vr1.max ()))
> + else if (constant_range_value_p (minus_p ? vr1.min (): vr1.max ()))
> n_vr0.set (VR_RANGE, op0, vrp_val_max (expr_type));
>
> /* Try with [OP0, OP0] and VR1. */
> @@ -926,7 +926,7 @@ vr_values::extract_range_from_unary_expr
> a new value range with the operand to simplify processing. */
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> vr0.set_varying (type);
> @@ -948,7 +948,7 @@ vr_values::extract_range_from_cond_expr
> const value_range *vr0 = &tem0;
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = get_value_range (op0);
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> tem0.set (op0);
> else
> tem0.set_varying (TREE_TYPE (op0));
> @@ -958,7 +958,7 @@ vr_values::extract_range_from_cond_expr
> const value_range *vr1 = &tem1;
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = get_value_range (op1);
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> tem1.set (op1);
> else
> tem1.set_varying (TREE_TYPE (op1));
> @@ -987,7 +987,7 @@ vr_values::extract_range_from_comparison
> its type may be different from _Bool. Convert VAL to EXPR's
> type. */
> val = fold_convert (type, val);
> - if (is_gimple_min_invariant (val))
> + if (constant_range_value_p (val))
> vr->set (val);
> else
> vr->update (VR_RANGE, val, val);
> @@ -1479,7 +1479,7 @@ vr_values::extract_range_from_assignment
> gimple_assign_rhs1 (stmt),
> gimple_assign_rhs2 (stmt));
> else if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS
> - && is_gimple_min_invariant (gimple_assign_rhs1 (stmt)))
> + && constant_range_value_p (gimple_assign_rhs1 (stmt)))
> vr->set (gimple_assign_rhs1 (stmt));
> else
> vr->set_varying (TREE_TYPE (gimple_assign_lhs (stmt)));
> @@ -1756,7 +1756,7 @@ vr_values::adjust_range_with_scev (value
> chrec = instantiate_parameters (loop, analyze_scalar_evolution (loop, var));
>
> /* Like in PR19590, scev can return a constant function. */
> - if (is_gimple_min_invariant (chrec))
> + if (constant_range_value_p (chrec))
> {
> vr->set (chrec);
> return;
> @@ -1779,7 +1779,7 @@ vr_values::adjust_range_with_scev (value
> a simple expression, compare_values and possibly other functions
> in tree-vrp won't be able to handle it. */
> if (step == NULL_TREE
> - || !is_gimple_min_invariant (step)
> + || !constant_range_value_p (step)
> || !valid_value_p (init))
> return;
>
> @@ -1841,7 +1841,7 @@ vr_values::adjust_range_with_scev (value
>
> if (TREE_CODE (init) == SSA_NAME)
> initvr = *(get_value_range (init));
> - else if (is_gimple_min_invariant (init))
> + else if (constant_range_value_p (init))
> initvr.set (init);
> else
> return;
> @@ -1995,7 +1995,7 @@ vrp_valueize (tree name)
> const value_range *vr = x_vr_values->get_value_range (name);
> if (vr->kind () == VR_RANGE
> && (TREE_CODE (vr->min ()) == SSA_NAME
> - || is_gimple_min_invariant (vr->min ()))
> + || constant_range_value_p (vr->min ()))
> && vrp_operand_equal_p (vr->min (), vr->max ()))
> return vr->min ();
> }
> @@ -2077,7 +2077,7 @@ vr_values::vrp_visit_assignment_or_call
> extract_range_from_ssa_name (vr, tem);
> return;
> }
> - else if (is_gimple_min_invariant (tem))
> + else if (constant_range_value_p (tem))
> {
> vr->set (tem);
> return;
> @@ -2475,7 +2475,7 @@ vr_values::vrp_evaluate_conditional (tre
> enum warn_strict_overflow_code wc;
> const char* warnmsg;
>
> - if (is_gimple_min_invariant (ret))
> + if (constant_range_value_p (ret))
> {
> wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
> warnmsg = G_("assuming signed overflow does not occur when "
> @@ -2517,7 +2517,7 @@ vr_values::vrp_evaluate_conditional (tre
> && INTEGRAL_TYPE_P (type)
> && vrp_val_is_min (vr0->min ())
> && vrp_val_is_max (vr0->max ())
> - && is_gimple_min_invariant (op1))
> + && constant_range_value_p (op1))
> {
> location_t location;
>
> @@ -3361,14 +3361,14 @@ vr_values::simplify_bit_ops_using_ranges
>
> if (TREE_CODE (op0) == SSA_NAME)
> vr0 = *(get_value_range (op0));
> - else if (is_gimple_min_invariant (op0))
> + else if (constant_range_value_p (op0))
> vr0.set (op0);
> else
> return false;
>
> if (TREE_CODE (op1) == SSA_NAME)
> vr1 = *(get_value_range (op1));
> - else if (is_gimple_min_invariant (op1))
> + else if (constant_range_value_p (op1))
> vr1.set (op1);
> else
> return false;
> @@ -3481,7 +3481,7 @@ test_for_singularity (enum tree_code con
> /* If the new min/max values have converged to a single value,
> then there is only one value which can satisfy the condition,
> return that value. */
> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
> + if (operand_equal_p (min, max, 0) && constant_range_value_p (min))
> return min;
> }
> return NULL;
> @@ -3554,7 +3554,7 @@ vr_values::simplify_cond_using_ranges_1
> && cond_code != EQ_EXPR
> && TREE_CODE (op0) == SSA_NAME
> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
> - && is_gimple_min_invariant (op1))
> + && constant_range_value_p (op1))
> {
> const value_range *vr = get_value_range (op0);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 10:01 ` Aldy Hernandez
@ 2019-10-14 12:32 ` Richard Sandiford
2019-10-14 12:53 ` Aldy Hernandez
0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2019-10-14 12:32 UTC (permalink / raw)
To: Aldy Hernandez; +Cc: gcc-patches
Aldy Hernandez <aldyh@redhat.com> writes:
> On 10/14/19 4:30 AM, Richard Sandiford wrote:
>> Aldy Hernandez <aldyh@redhat.com> writes:
>>> On 10/11/19 10:42 AM, Richard Sandiford wrote:
>>>> The range-tracking code has a pretty hard-coded assumption that
>>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>>> that rather than contiually fight cases in which it can't handle
>>>> other invariants.
>>>
>>> I was going to suggest we normalize ranges to numerics completely before
>>> folding. That is, replacing normalize_addresses() here:
>>>
>>> *vr = op->fold_range (expr_type,
>>> vr0.normalize_addresses (),
>>> vr1.normalize_addresses ());
>>>
>>> ...into normalize_symbolics(). But I suppose getting the gate correct
>>> is even better. Thanks for taking the care of this extensive and manual
>>> change.
>>>
>>> The patch looks good to me. However, I do wonder if VRP and
>>> subsidiaries can't handle non-integer invariants, if we shouldn't
>>> disallow them from the setters as well:
>>>
>>> void
>>> value_range_base::set (tree val)
>>> {
>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>> (val));
>>> if (TREE_OVERFLOW_P (val))
>>> val = drop_tree_overflow (val);
>>> set (VR_RANGE, val, val);
>>> }
>>>
>>> void
>>> value_range::set (tree val)
>>> {
>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>> (val));
>>> if (TREE_OVERFLOW_P (val))
>>> val = drop_tree_overflow (val);
>>> set (VR_RANGE, val, val, NULL);
>>> }
>>>
>>> This would still allow setting of VARYING and UNDEFINED, but disallow
>>> poly-ints, etc from a range.
>>>
>>> Was this a small oversight, or was there a reason you left those in?
>>
>> Yeah, this was intentional. The patch is effectively treating
>> POLY_INY_CST as symbolic rather than constant. (It's really somewhere
>> in the middle: it's at least a function invariant, but the invariant
>> depends on a runtime target property.) So places like here that
>> can handle both symbolics and constants should be able to handle
>> POLY_INT_CST wihout problems. We just need to make sure that
>> POLY_INT_CSTs aren't treated as constants for range tracking,
>> because they're not "constant enough" there.
>
> Hmmm... We don't handle POLY_INT_CST value_range's anywhere, so perhaps
> it's better to stop their creation at the source and fix the caller, to
> inhibit their proliferation. AFAICT, the only POLY_INT_CST ranges that
> would be used are VARYING and UNDEFINED (for the lattice), and that
> already works.
>
> If you don't agree, then at least a comment in ::set() would be nice, to
> document that we're allowing their creation and why.
I don't think it makes sense to allow SSA_NAME (which is completely
unconstrained) and not allow POLY_INT_CST. Like I say, POLY_INT_CST
can be conservatively treated as symbolic.
POLY_INT_CST can produce useful singleton ranges or be a minimum or
a maximum, even if we don't use them to reduce ranges arithmetically.
Richard
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 11:44 ` Richard Biener
@ 2019-10-14 12:49 ` Richard Sandiford
2019-10-14 16:41 ` Richard Biener
0 siblings, 1 reply; 14+ messages in thread
From: Richard Sandiford @ 2019-10-14 12:49 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
Richard Biener <richard.guenther@gmail.com> writes:
> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> The range-tracking code has a pretty hard-coded assumption that
>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>> ADDR_EXPR". It seems better to add a predicate specifically for
>> that rather than contiually fight cases in which it can't handle
>> other invariants.
>>
>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>
> ICK. Nobody is going to remember this new restriction and
> constant_range_value_p reads like constant_value_range_p ;)
>
> Btw, is_gimple_invariant_address shouldn't have been exported,
> it's only use could have used is_gimple_min_invariant...
What do you think we should do instead?
Richard
>
> Richard.
>
>> Richard
>>
>>
>> 2019-10-11 Richard Sandiford <richard.sandiford@arm.com>
>>
>> gcc/
>> PR tree-optimization/92033
>> * tree-vrp.h (constant_range_value_p): Declare.
>> * tree-vrp.c (constant_range_value_p): New function.
>> (value_range_base::symbolic_p, value_range_base::singleton_p)
>> (get_single_symbol, compare_values_warnv, intersect_ranges)
>> (value_range_base::normalize_symbolics): Use it instead of
>> is_gimple_min_invariant.
>> (simplify_stmt_for_jump_threading): Likewise.
>> * vr-values.c (symbolic_range_based_on_p, valid_value_p): Likewise.
>> (vr_values::op_with_constant_singleton_value_range): Likewise.
>> (vr_values::extract_range_from_binary_expr): Likewise.
>> (vr_values::extract_range_from_unary_expr): Likewise.
>> (vr_values::extract_range_from_cond_expr): Likewise.
>> (vr_values::extract_range_from_comparison): Likewise.
>> (vr_values::extract_range_from_assignment): Likewise.
>> (vr_values::adjust_range_with_scev, vrp_valueize): Likewise.
>> (vr_values::vrp_visit_assignment_or_call): Likewise.
>> (vr_values::vrp_evaluate_conditional): Likewise.
>> (vr_values::simplify_bit_ops_using_ranges): Likewise.
>> (test_for_singularity): Likewise.
>> (vr_values::simplify_cond_using_ranges_1): Likewise.
>>
>> Index: gcc/tree-vrp.h
>> ===================================================================
>> --- gcc/tree-vrp.h 2019-10-08 09:23:31.282533990 +0100
>> +++ gcc/tree-vrp.h 2019-10-11 15:41:20.380576059 +0100
>> @@ -284,6 +284,7 @@ value_range_base::supports_type_p (tree
>> return false;
>> }
>>
>> +extern bool constant_range_value_p (const_tree);
>> extern void register_edge_assert_for (tree, edge, enum tree_code,
>> tree, tree, vec<assert_info> &);
>> extern bool stmt_interesting_for_vrp (gimple *);
>> Index: gcc/tree-vrp.c
>> ===================================================================
>> --- gcc/tree-vrp.c 2019-10-08 09:23:31.282533990 +0100
>> +++ gcc/tree-vrp.c 2019-10-11 15:41:20.380576059 +0100
>> @@ -78,6 +78,18 @@ ranges_from_anti_range (const value_rang
>> for still active basic-blocks. */
>> static sbitmap *live;
>>
>> +/* Return true if VALUE is considered constant for range tracking.
>> + This is stricter than is_gimple_min_invariant and should be
>> + used instead of it in range-related code. */
>> +
>> +bool
>> +constant_range_value_p (const_tree value)
>> +{
>> + return (TREE_CODE (value) == INTEGER_CST
>> + || (TREE_CODE (value) == ADDR_EXPR
>> + && is_gimple_invariant_address (value)));
>> +}
>> +
>> void
>> value_range::set_equiv (bitmap equiv)
>> {
>> @@ -273,8 +285,8 @@ value_range_base::symbolic_p () const
>> {
>> return (!varying_p ()
>> && !undefined_p ()
>> - && (!is_gimple_min_invariant (m_min)
>> - || !is_gimple_min_invariant (m_max)));
>> + && (!constant_range_value_p (m_min)
>> + || !constant_range_value_p (m_max)));
>> }
>>
>> /* NOTE: This is not the inverse of symbolic_p because the range
>> @@ -388,7 +400,7 @@ value_range_base::singleton_p (tree *res
>> }
>> if (m_kind == VR_RANGE
>> && vrp_operand_equal_p (min (), max ())
>> - && is_gimple_min_invariant (min ()))
>> + && constant_range_value_p (min ()))
>> {
>> if (result)
>> *result = min ();
>> @@ -953,13 +965,13 @@ get_single_symbol (tree t, bool *neg, tr
>> || TREE_CODE (t) == POINTER_PLUS_EXPR
>> || TREE_CODE (t) == MINUS_EXPR)
>> {
>> - if (is_gimple_min_invariant (TREE_OPERAND (t, 0)))
>> + if (constant_range_value_p (TREE_OPERAND (t, 0)))
>> {
>> neg_ = (TREE_CODE (t) == MINUS_EXPR);
>> inv_ = TREE_OPERAND (t, 0);
>> t = TREE_OPERAND (t, 1);
>> }
>> - else if (is_gimple_min_invariant (TREE_OPERAND (t, 1)))
>> + else if (constant_range_value_p (TREE_OPERAND (t, 1)))
>> {
>> neg_ = false;
>> inv_ = TREE_OPERAND (t, 1);
>> @@ -1106,8 +1118,8 @@ compare_values_warnv (tree val1, tree va
>> TYPE_SIGN (TREE_TYPE (val1)));
>> }
>>
>> - const bool cst1 = is_gimple_min_invariant (val1);
>> - const bool cst2 = is_gimple_min_invariant (val2);
>> + const bool cst1 = constant_range_value_p (val1);
>> + const bool cst2 = constant_range_value_p (val2);
>>
>> /* If one is of the form '[-]NAME + CST' and the other is constant, then
>> it might be possible to say something depending on the constants. */
>> @@ -5785,7 +5797,7 @@ intersect_ranges (enum value_range_kind
>> correct estimate unless VR1 is a constant singleton range
>> in which case we choose that. */
>> if (vr1type == VR_RANGE
>> - && is_gimple_min_invariant (vr1min)
>> + && constant_range_value_p (vr1min)
>> && vrp_operand_equal_p (vr1min, vr1max))
>> {
>> *vr0type = vr1type;
>> @@ -6045,8 +6057,8 @@ value_range_base::normalize_symbolics ()
>> if (varying_p () || undefined_p ())
>> return *this;
>> tree ttype = type ();
>> - bool min_symbolic = !is_gimple_min_invariant (min ());
>> - bool max_symbolic = !is_gimple_min_invariant (max ());
>> + bool min_symbolic = !constant_range_value_p (min ());
>> + bool max_symbolic = !constant_range_value_p (max ());
>> if (!min_symbolic && !max_symbolic)
>> return normalize_addresses ();
>>
>> @@ -6432,7 +6444,7 @@ simplify_stmt_for_jump_threading (gimple
>> {
>> /* First see if the conditional is in the hash table. */
>> tree cached_lhs = avail_exprs_stack->lookup_avail_expr (stmt, false, true);
>> - if (cached_lhs && is_gimple_min_invariant (cached_lhs))
>> + if (cached_lhs && constant_range_value_p (cached_lhs))
>> return cached_lhs;
>>
>> vr_values *vr_values = x_vr_values;
>> Index: gcc/vr-values.c
>> ===================================================================
>> --- gcc/vr-values.c 2019-10-03 14:04:54.161520173 +0100
>> +++ gcc/vr-values.c 2019-10-11 15:41:20.380576059 +0100
>> @@ -263,14 +263,14 @@ symbolic_range_based_on_p (value_range_b
>> bool neg, min_has_symbol, max_has_symbol;
>> tree inv;
>>
>> - if (is_gimple_min_invariant (vr->min ()))
>> + if (constant_range_value_p (vr->min ()))
>> min_has_symbol = false;
>> else if (get_single_symbol (vr->min (), &neg, &inv) == sym)
>> min_has_symbol = true;
>> else
>> return false;
>>
>> - if (is_gimple_min_invariant (vr->max ()))
>> + if (constant_range_value_p (vr->max ()))
>> max_has_symbol = false;
>> else if (get_single_symbol (vr->max (), &neg, &inv) == sym)
>> max_has_symbol = true;
>> @@ -409,7 +409,7 @@ valid_value_p (tree expr)
>> return (TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
>> && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST);
>>
>> - return is_gimple_min_invariant (expr);
>> + return constant_range_value_p (expr);
>> }
>>
>> /* If OP has a value range with a single constant value return that,
>> @@ -419,7 +419,7 @@ valid_value_p (tree expr)
>> tree
>> vr_values::op_with_constant_singleton_value_range (tree op)
>> {
>> - if (is_gimple_min_invariant (op))
>> + if (constant_range_value_p (op))
>> return op;
>>
>> if (TREE_CODE (op) != SSA_NAME)
>> @@ -778,14 +778,14 @@ vr_values::extract_range_from_binary_exp
>> value_range_base vr0, vr1;
>> if (TREE_CODE (op0) == SSA_NAME)
>> vr0 = *(get_value_range (op0));
>> - else if (is_gimple_min_invariant (op0))
>> + else if (constant_range_value_p (op0))
>> vr0.set (op0);
>> else
>> vr0.set_varying (TREE_TYPE (op0));
>>
>> if (TREE_CODE (op1) == SSA_NAME)
>> vr1 = *(get_value_range (op1));
>> - else if (is_gimple_min_invariant (op1))
>> + else if (constant_range_value_p (op1))
>> vr1.set (op1);
>> else
>> vr1.set_varying (TREE_TYPE (op1));
>> @@ -855,11 +855,11 @@ vr_values::extract_range_from_binary_exp
>> value_range n_vr1;
>>
>> /* Try with VR0 and [-INF, OP1]. */
>> - if (is_gimple_min_invariant (minus_p ? vr0.max () : vr0.min ()))
>> + if (constant_range_value_p (minus_p ? vr0.max () : vr0.min ()))
>> n_vr1.set (VR_RANGE, vrp_val_min (expr_type), op1);
>>
>> /* Try with VR0 and [OP1, +INF]. */
>> - else if (is_gimple_min_invariant (minus_p ? vr0.min () : vr0.max ()))
>> + else if (constant_range_value_p (minus_p ? vr0.min () : vr0.max ()))
>> n_vr1.set (VR_RANGE, op1, vrp_val_max (expr_type));
>>
>> /* Try with VR0 and [OP1, OP1]. */
>> @@ -879,11 +879,11 @@ vr_values::extract_range_from_binary_exp
>> value_range n_vr0;
>>
>> /* Try with [-INF, OP0] and VR1. */
>> - if (is_gimple_min_invariant (minus_p ? vr1.max () : vr1.min ()))
>> + if (constant_range_value_p (minus_p ? vr1.max () : vr1.min ()))
>> n_vr0.set (VR_RANGE, vrp_val_min (expr_type), op0);
>>
>> /* Try with [OP0, +INF] and VR1. */
>> - else if (is_gimple_min_invariant (minus_p ? vr1.min (): vr1.max ()))
>> + else if (constant_range_value_p (minus_p ? vr1.min (): vr1.max ()))
>> n_vr0.set (VR_RANGE, op0, vrp_val_max (expr_type));
>>
>> /* Try with [OP0, OP0] and VR1. */
>> @@ -926,7 +926,7 @@ vr_values::extract_range_from_unary_expr
>> a new value range with the operand to simplify processing. */
>> if (TREE_CODE (op0) == SSA_NAME)
>> vr0 = *(get_value_range (op0));
>> - else if (is_gimple_min_invariant (op0))
>> + else if (constant_range_value_p (op0))
>> vr0.set (op0);
>> else
>> vr0.set_varying (type);
>> @@ -948,7 +948,7 @@ vr_values::extract_range_from_cond_expr
>> const value_range *vr0 = &tem0;
>> if (TREE_CODE (op0) == SSA_NAME)
>> vr0 = get_value_range (op0);
>> - else if (is_gimple_min_invariant (op0))
>> + else if (constant_range_value_p (op0))
>> tem0.set (op0);
>> else
>> tem0.set_varying (TREE_TYPE (op0));
>> @@ -958,7 +958,7 @@ vr_values::extract_range_from_cond_expr
>> const value_range *vr1 = &tem1;
>> if (TREE_CODE (op1) == SSA_NAME)
>> vr1 = get_value_range (op1);
>> - else if (is_gimple_min_invariant (op1))
>> + else if (constant_range_value_p (op1))
>> tem1.set (op1);
>> else
>> tem1.set_varying (TREE_TYPE (op1));
>> @@ -987,7 +987,7 @@ vr_values::extract_range_from_comparison
>> its type may be different from _Bool. Convert VAL to EXPR's
>> type. */
>> val = fold_convert (type, val);
>> - if (is_gimple_min_invariant (val))
>> + if (constant_range_value_p (val))
>> vr->set (val);
>> else
>> vr->update (VR_RANGE, val, val);
>> @@ -1479,7 +1479,7 @@ vr_values::extract_range_from_assignment
>> gimple_assign_rhs1 (stmt),
>> gimple_assign_rhs2 (stmt));
>> else if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS
>> - && is_gimple_min_invariant (gimple_assign_rhs1 (stmt)))
>> + && constant_range_value_p (gimple_assign_rhs1 (stmt)))
>> vr->set (gimple_assign_rhs1 (stmt));
>> else
>> vr->set_varying (TREE_TYPE (gimple_assign_lhs (stmt)));
>> @@ -1756,7 +1756,7 @@ vr_values::adjust_range_with_scev (value
>> chrec = instantiate_parameters (loop, analyze_scalar_evolution (loop, var));
>>
>> /* Like in PR19590, scev can return a constant function. */
>> - if (is_gimple_min_invariant (chrec))
>> + if (constant_range_value_p (chrec))
>> {
>> vr->set (chrec);
>> return;
>> @@ -1779,7 +1779,7 @@ vr_values::adjust_range_with_scev (value
>> a simple expression, compare_values and possibly other functions
>> in tree-vrp won't be able to handle it. */
>> if (step == NULL_TREE
>> - || !is_gimple_min_invariant (step)
>> + || !constant_range_value_p (step)
>> || !valid_value_p (init))
>> return;
>>
>> @@ -1841,7 +1841,7 @@ vr_values::adjust_range_with_scev (value
>>
>> if (TREE_CODE (init) == SSA_NAME)
>> initvr = *(get_value_range (init));
>> - else if (is_gimple_min_invariant (init))
>> + else if (constant_range_value_p (init))
>> initvr.set (init);
>> else
>> return;
>> @@ -1995,7 +1995,7 @@ vrp_valueize (tree name)
>> const value_range *vr = x_vr_values->get_value_range (name);
>> if (vr->kind () == VR_RANGE
>> && (TREE_CODE (vr->min ()) == SSA_NAME
>> - || is_gimple_min_invariant (vr->min ()))
>> + || constant_range_value_p (vr->min ()))
>> && vrp_operand_equal_p (vr->min (), vr->max ()))
>> return vr->min ();
>> }
>> @@ -2077,7 +2077,7 @@ vr_values::vrp_visit_assignment_or_call
>> extract_range_from_ssa_name (vr, tem);
>> return;
>> }
>> - else if (is_gimple_min_invariant (tem))
>> + else if (constant_range_value_p (tem))
>> {
>> vr->set (tem);
>> return;
>> @@ -2475,7 +2475,7 @@ vr_values::vrp_evaluate_conditional (tre
>> enum warn_strict_overflow_code wc;
>> const char* warnmsg;
>>
>> - if (is_gimple_min_invariant (ret))
>> + if (constant_range_value_p (ret))
>> {
>> wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
>> warnmsg = G_("assuming signed overflow does not occur when "
>> @@ -2517,7 +2517,7 @@ vr_values::vrp_evaluate_conditional (tre
>> && INTEGRAL_TYPE_P (type)
>> && vrp_val_is_min (vr0->min ())
>> && vrp_val_is_max (vr0->max ())
>> - && is_gimple_min_invariant (op1))
>> + && constant_range_value_p (op1))
>> {
>> location_t location;
>>
>> @@ -3361,14 +3361,14 @@ vr_values::simplify_bit_ops_using_ranges
>>
>> if (TREE_CODE (op0) == SSA_NAME)
>> vr0 = *(get_value_range (op0));
>> - else if (is_gimple_min_invariant (op0))
>> + else if (constant_range_value_p (op0))
>> vr0.set (op0);
>> else
>> return false;
>>
>> if (TREE_CODE (op1) == SSA_NAME)
>> vr1 = *(get_value_range (op1));
>> - else if (is_gimple_min_invariant (op1))
>> + else if (constant_range_value_p (op1))
>> vr1.set (op1);
>> else
>> return false;
>> @@ -3481,7 +3481,7 @@ test_for_singularity (enum tree_code con
>> /* If the new min/max values have converged to a single value,
>> then there is only one value which can satisfy the condition,
>> return that value. */
>> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant (min))
>> + if (operand_equal_p (min, max, 0) && constant_range_value_p (min))
>> return min;
>> }
>> return NULL;
>> @@ -3554,7 +3554,7 @@ vr_values::simplify_cond_using_ranges_1
>> && cond_code != EQ_EXPR
>> && TREE_CODE (op0) == SSA_NAME
>> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
>> - && is_gimple_min_invariant (op1))
>> + && constant_range_value_p (op1))
>> {
>> const value_range *vr = get_value_range (op0);
>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 12:32 ` Richard Sandiford
@ 2019-10-14 12:53 ` Aldy Hernandez
0 siblings, 0 replies; 14+ messages in thread
From: Aldy Hernandez @ 2019-10-14 12:53 UTC (permalink / raw)
To: gcc-patches, richard.sandiford
On 10/14/19 8:31 AM, Richard Sandiford wrote:
> Aldy Hernandez <aldyh@redhat.com> writes:
>> On 10/14/19 4:30 AM, Richard Sandiford wrote:
>>> Aldy Hernandez <aldyh@redhat.com> writes:
>>>> On 10/11/19 10:42 AM, Richard Sandiford wrote:
>>>>> The range-tracking code has a pretty hard-coded assumption that
>>>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>>>> that rather than contiually fight cases in which it can't handle
>>>>> other invariants.
>>>>
>>>> I was going to suggest we normalize ranges to numerics completely before
>>>> folding. That is, replacing normalize_addresses() here:
>>>>
>>>> *vr = op->fold_range (expr_type,
>>>> vr0.normalize_addresses (),
>>>> vr1.normalize_addresses ());
>>>>
>>>> ...into normalize_symbolics(). But I suppose getting the gate correct
>>>> is even better. Thanks for taking the care of this extensive and manual
>>>> change.
>>>>
>>>> The patch looks good to me. However, I do wonder if VRP and
>>>> subsidiaries can't handle non-integer invariants, if we shouldn't
>>>> disallow them from the setters as well:
>>>>
>>>> void
>>>> value_range_base::set (tree val)
>>>> {
>>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>>> (val));
>>>> if (TREE_OVERFLOW_P (val))
>>>> val = drop_tree_overflow (val);
>>>> set (VR_RANGE, val, val);
>>>> }
>>>>
>>>> void
>>>> value_range::set (tree val)
>>>> {
>>>> gcc_assert (TREE_CODE (val) == SSA_NAME || is_gimple_min_invariant
>>>> (val));
>>>> if (TREE_OVERFLOW_P (val))
>>>> val = drop_tree_overflow (val);
>>>> set (VR_RANGE, val, val, NULL);
>>>> }
>>>>
>>>> This would still allow setting of VARYING and UNDEFINED, but disallow
>>>> poly-ints, etc from a range.
>>>>
>>>> Was this a small oversight, or was there a reason you left those in?
>>>
>>> Yeah, this was intentional. The patch is effectively treating
>>> POLY_INY_CST as symbolic rather than constant. (It's really somewhere
>>> in the middle: it's at least a function invariant, but the invariant
>>> depends on a runtime target property.) So places like here that
>>> can handle both symbolics and constants should be able to handle
>>> POLY_INT_CST wihout problems. We just need to make sure that
>>> POLY_INT_CSTs aren't treated as constants for range tracking,
>>> because they're not "constant enough" there.
>>
>> Hmmm... We don't handle POLY_INT_CST value_range's anywhere, so perhaps
>> it's better to stop their creation at the source and fix the caller, to
>> inhibit their proliferation. AFAICT, the only POLY_INT_CST ranges that
>> would be used are VARYING and UNDEFINED (for the lattice), and that
>> already works.
>>
>> If you don't agree, then at least a comment in ::set() would be nice, to
>> document that we're allowing their creation and why.
>
> I don't think it makes sense to allow SSA_NAME (which is completely
> unconstrained) and not allow POLY_INT_CST. Like I say, POLY_INT_CST
> can be conservatively treated as symbolic.
Oh, I see your change to ::symbolic_p() would effectively treat them as
symbolics.
>
> POLY_INT_CST can produce useful singleton ranges or be a minimum or
> a maximum, even if we don't use them to reduce ranges arithmetically.
In which case, singleton_p() should be adapted to return the singleton.
I don't expect you to do this, unless you want to :). Could you at
least put a comment there, so we don't forget?
I'm feel a bit queasy allowing POLY_INT ranges throughout, when we have
no code that handle them anywhere, but I won't object to your patch.
Thanks for fixing this.
The patch with the comment on singleton is fine with me.
Aldy
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 12:49 ` Richard Sandiford
@ 2019-10-14 16:41 ` Richard Biener
2019-10-14 18:46 ` Aldy Hernandez
2019-10-15 10:46 ` Richard Sandiford
0 siblings, 2 replies; 14+ messages in thread
From: Richard Biener @ 2019-10-14 16:41 UTC (permalink / raw)
To: Richard Sandiford; +Cc: GCC Patches
On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>Richard Biener <richard.guenther@gmail.com> writes:
>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
>> <richard.sandiford@arm.com> wrote:
>>>
>>> The range-tracking code has a pretty hard-coded assumption that
>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>> that rather than contiually fight cases in which it can't handle
>>> other invariants.
>>>
>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>>
>> ICK. Nobody is going to remember this new restriction and
>> constant_range_value_p reads like constant_value_range_p ;)
>>
>> Btw, is_gimple_invariant_address shouldn't have been exported,
>> it's only use could have used is_gimple_min_invariant...
>
>What do you think we should do instead?
Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
Richard.
>Richard
>
>>
>> Richard.
>>
>>> Richard
>>>
>>>
>>> 2019-10-11 Richard Sandiford <richard.sandiford@arm.com>
>>>
>>> gcc/
>>> PR tree-optimization/92033
>>> * tree-vrp.h (constant_range_value_p): Declare.
>>> * tree-vrp.c (constant_range_value_p): New function.
>>> (value_range_base::symbolic_p,
>value_range_base::singleton_p)
>>> (get_single_symbol, compare_values_warnv, intersect_ranges)
>>> (value_range_base::normalize_symbolics): Use it instead of
>>> is_gimple_min_invariant.
>>> (simplify_stmt_for_jump_threading): Likewise.
>>> * vr-values.c (symbolic_range_based_on_p, valid_value_p):
>Likewise.
>>> (vr_values::op_with_constant_singleton_value_range):
>Likewise.
>>> (vr_values::extract_range_from_binary_expr): Likewise.
>>> (vr_values::extract_range_from_unary_expr): Likewise.
>>> (vr_values::extract_range_from_cond_expr): Likewise.
>>> (vr_values::extract_range_from_comparison): Likewise.
>>> (vr_values::extract_range_from_assignment): Likewise.
>>> (vr_values::adjust_range_with_scev, vrp_valueize): Likewise.
>>> (vr_values::vrp_visit_assignment_or_call): Likewise.
>>> (vr_values::vrp_evaluate_conditional): Likewise.
>>> (vr_values::simplify_bit_ops_using_ranges): Likewise.
>>> (test_for_singularity): Likewise.
>>> (vr_values::simplify_cond_using_ranges_1): Likewise.
>>>
>>> Index: gcc/tree-vrp.h
>>> ===================================================================
>>> --- gcc/tree-vrp.h 2019-10-08 09:23:31.282533990 +0100
>>> +++ gcc/tree-vrp.h 2019-10-11 15:41:20.380576059 +0100
>>> @@ -284,6 +284,7 @@ value_range_base::supports_type_p (tree
>>> return false;
>>> }
>>>
>>> +extern bool constant_range_value_p (const_tree);
>>> extern void register_edge_assert_for (tree, edge, enum tree_code,
>>> tree, tree, vec<assert_info>
>&);
>>> extern bool stmt_interesting_for_vrp (gimple *);
>>> Index: gcc/tree-vrp.c
>>> ===================================================================
>>> --- gcc/tree-vrp.c 2019-10-08 09:23:31.282533990 +0100
>>> +++ gcc/tree-vrp.c 2019-10-11 15:41:20.380576059 +0100
>>> @@ -78,6 +78,18 @@ ranges_from_anti_range (const value_rang
>>> for still active basic-blocks. */
>>> static sbitmap *live;
>>>
>>> +/* Return true if VALUE is considered constant for range tracking.
>>> + This is stricter than is_gimple_min_invariant and should be
>>> + used instead of it in range-related code. */
>>> +
>>> +bool
>>> +constant_range_value_p (const_tree value)
>>> +{
>>> + return (TREE_CODE (value) == INTEGER_CST
>>> + || (TREE_CODE (value) == ADDR_EXPR
>>> + && is_gimple_invariant_address (value)));
>>> +}
>>> +
>>> void
>>> value_range::set_equiv (bitmap equiv)
>>> {
>>> @@ -273,8 +285,8 @@ value_range_base::symbolic_p () const
>>> {
>>> return (!varying_p ()
>>> && !undefined_p ()
>>> - && (!is_gimple_min_invariant (m_min)
>>> - || !is_gimple_min_invariant (m_max)));
>>> + && (!constant_range_value_p (m_min)
>>> + || !constant_range_value_p (m_max)));
>>> }
>>>
>>> /* NOTE: This is not the inverse of symbolic_p because the range
>>> @@ -388,7 +400,7 @@ value_range_base::singleton_p (tree *res
>>> }
>>> if (m_kind == VR_RANGE
>>> && vrp_operand_equal_p (min (), max ())
>>> - && is_gimple_min_invariant (min ()))
>>> + && constant_range_value_p (min ()))
>>> {
>>> if (result)
>>> *result = min ();
>>> @@ -953,13 +965,13 @@ get_single_symbol (tree t, bool *neg, tr
>>> || TREE_CODE (t) == POINTER_PLUS_EXPR
>>> || TREE_CODE (t) == MINUS_EXPR)
>>> {
>>> - if (is_gimple_min_invariant (TREE_OPERAND (t, 0)))
>>> + if (constant_range_value_p (TREE_OPERAND (t, 0)))
>>> {
>>> neg_ = (TREE_CODE (t) == MINUS_EXPR);
>>> inv_ = TREE_OPERAND (t, 0);
>>> t = TREE_OPERAND (t, 1);
>>> }
>>> - else if (is_gimple_min_invariant (TREE_OPERAND (t, 1)))
>>> + else if (constant_range_value_p (TREE_OPERAND (t, 1)))
>>> {
>>> neg_ = false;
>>> inv_ = TREE_OPERAND (t, 1);
>>> @@ -1106,8 +1118,8 @@ compare_values_warnv (tree val1, tree va
>>> TYPE_SIGN (TREE_TYPE (val1)));
>>> }
>>>
>>> - const bool cst1 = is_gimple_min_invariant (val1);
>>> - const bool cst2 = is_gimple_min_invariant (val2);
>>> + const bool cst1 = constant_range_value_p (val1);
>>> + const bool cst2 = constant_range_value_p (val2);
>>>
>>> /* If one is of the form '[-]NAME + CST' and the other is
>constant, then
>>> it might be possible to say something depending on the
>constants. */
>>> @@ -5785,7 +5797,7 @@ intersect_ranges (enum value_range_kind
>>> correct estimate unless VR1 is a constant singleton range
>>> in which case we choose that. */
>>> if (vr1type == VR_RANGE
>>> - && is_gimple_min_invariant (vr1min)
>>> + && constant_range_value_p (vr1min)
>>> && vrp_operand_equal_p (vr1min, vr1max))
>>> {
>>> *vr0type = vr1type;
>>> @@ -6045,8 +6057,8 @@ value_range_base::normalize_symbolics ()
>>> if (varying_p () || undefined_p ())
>>> return *this;
>>> tree ttype = type ();
>>> - bool min_symbolic = !is_gimple_min_invariant (min ());
>>> - bool max_symbolic = !is_gimple_min_invariant (max ());
>>> + bool min_symbolic = !constant_range_value_p (min ());
>>> + bool max_symbolic = !constant_range_value_p (max ());
>>> if (!min_symbolic && !max_symbolic)
>>> return normalize_addresses ();
>>>
>>> @@ -6432,7 +6444,7 @@ simplify_stmt_for_jump_threading (gimple
>>> {
>>> /* First see if the conditional is in the hash table. */
>>> tree cached_lhs = avail_exprs_stack->lookup_avail_expr (stmt,
>false, true);
>>> - if (cached_lhs && is_gimple_min_invariant (cached_lhs))
>>> + if (cached_lhs && constant_range_value_p (cached_lhs))
>>> return cached_lhs;
>>>
>>> vr_values *vr_values = x_vr_values;
>>> Index: gcc/vr-values.c
>>> ===================================================================
>>> --- gcc/vr-values.c 2019-10-03 14:04:54.161520173 +0100
>>> +++ gcc/vr-values.c 2019-10-11 15:41:20.380576059 +0100
>>> @@ -263,14 +263,14 @@ symbolic_range_based_on_p (value_range_b
>>> bool neg, min_has_symbol, max_has_symbol;
>>> tree inv;
>>>
>>> - if (is_gimple_min_invariant (vr->min ()))
>>> + if (constant_range_value_p (vr->min ()))
>>> min_has_symbol = false;
>>> else if (get_single_symbol (vr->min (), &neg, &inv) == sym)
>>> min_has_symbol = true;
>>> else
>>> return false;
>>>
>>> - if (is_gimple_min_invariant (vr->max ()))
>>> + if (constant_range_value_p (vr->max ()))
>>> max_has_symbol = false;
>>> else if (get_single_symbol (vr->max (), &neg, &inv) == sym)
>>> max_has_symbol = true;
>>> @@ -409,7 +409,7 @@ valid_value_p (tree expr)
>>> return (TREE_CODE (TREE_OPERAND (expr, 0)) == SSA_NAME
>>> && TREE_CODE (TREE_OPERAND (expr, 1)) == INTEGER_CST);
>>>
>>> - return is_gimple_min_invariant (expr);
>>> + return constant_range_value_p (expr);
>>> }
>>>
>>> /* If OP has a value range with a single constant value return
>that,
>>> @@ -419,7 +419,7 @@ valid_value_p (tree expr)
>>> tree
>>> vr_values::op_with_constant_singleton_value_range (tree op)
>>> {
>>> - if (is_gimple_min_invariant (op))
>>> + if (constant_range_value_p (op))
>>> return op;
>>>
>>> if (TREE_CODE (op) != SSA_NAME)
>>> @@ -778,14 +778,14 @@ vr_values::extract_range_from_binary_exp
>>> value_range_base vr0, vr1;
>>> if (TREE_CODE (op0) == SSA_NAME)
>>> vr0 = *(get_value_range (op0));
>>> - else if (is_gimple_min_invariant (op0))
>>> + else if (constant_range_value_p (op0))
>>> vr0.set (op0);
>>> else
>>> vr0.set_varying (TREE_TYPE (op0));
>>>
>>> if (TREE_CODE (op1) == SSA_NAME)
>>> vr1 = *(get_value_range (op1));
>>> - else if (is_gimple_min_invariant (op1))
>>> + else if (constant_range_value_p (op1))
>>> vr1.set (op1);
>>> else
>>> vr1.set_varying (TREE_TYPE (op1));
>>> @@ -855,11 +855,11 @@ vr_values::extract_range_from_binary_exp
>>> value_range n_vr1;
>>>
>>> /* Try with VR0 and [-INF, OP1]. */
>>> - if (is_gimple_min_invariant (minus_p ? vr0.max () : vr0.min
>()))
>>> + if (constant_range_value_p (minus_p ? vr0.max () : vr0.min
>()))
>>> n_vr1.set (VR_RANGE, vrp_val_min (expr_type), op1);
>>>
>>> /* Try with VR0 and [OP1, +INF]. */
>>> - else if (is_gimple_min_invariant (minus_p ? vr0.min () :
>vr0.max ()))
>>> + else if (constant_range_value_p (minus_p ? vr0.min () :
>vr0.max ()))
>>> n_vr1.set (VR_RANGE, op1, vrp_val_max (expr_type));
>>>
>>> /* Try with VR0 and [OP1, OP1]. */
>>> @@ -879,11 +879,11 @@ vr_values::extract_range_from_binary_exp
>>> value_range n_vr0;
>>>
>>> /* Try with [-INF, OP0] and VR1. */
>>> - if (is_gimple_min_invariant (minus_p ? vr1.max () : vr1.min
>()))
>>> + if (constant_range_value_p (minus_p ? vr1.max () : vr1.min
>()))
>>> n_vr0.set (VR_RANGE, vrp_val_min (expr_type), op0);
>>>
>>> /* Try with [OP0, +INF] and VR1. */
>>> - else if (is_gimple_min_invariant (minus_p ? vr1.min ():
>vr1.max ()))
>>> + else if (constant_range_value_p (minus_p ? vr1.min ():
>vr1.max ()))
>>> n_vr0.set (VR_RANGE, op0, vrp_val_max (expr_type));
>>>
>>> /* Try with [OP0, OP0] and VR1. */
>>> @@ -926,7 +926,7 @@ vr_values::extract_range_from_unary_expr
>>> a new value range with the operand to simplify processing. */
>>> if (TREE_CODE (op0) == SSA_NAME)
>>> vr0 = *(get_value_range (op0));
>>> - else if (is_gimple_min_invariant (op0))
>>> + else if (constant_range_value_p (op0))
>>> vr0.set (op0);
>>> else
>>> vr0.set_varying (type);
>>> @@ -948,7 +948,7 @@ vr_values::extract_range_from_cond_expr
>>> const value_range *vr0 = &tem0;
>>> if (TREE_CODE (op0) == SSA_NAME)
>>> vr0 = get_value_range (op0);
>>> - else if (is_gimple_min_invariant (op0))
>>> + else if (constant_range_value_p (op0))
>>> tem0.set (op0);
>>> else
>>> tem0.set_varying (TREE_TYPE (op0));
>>> @@ -958,7 +958,7 @@ vr_values::extract_range_from_cond_expr
>>> const value_range *vr1 = &tem1;
>>> if (TREE_CODE (op1) == SSA_NAME)
>>> vr1 = get_value_range (op1);
>>> - else if (is_gimple_min_invariant (op1))
>>> + else if (constant_range_value_p (op1))
>>> tem1.set (op1);
>>> else
>>> tem1.set_varying (TREE_TYPE (op1));
>>> @@ -987,7 +987,7 @@ vr_values::extract_range_from_comparison
>>> its type may be different from _Bool. Convert VAL to
>EXPR's
>>> type. */
>>> val = fold_convert (type, val);
>>> - if (is_gimple_min_invariant (val))
>>> + if (constant_range_value_p (val))
>>> vr->set (val);
>>> else
>>> vr->update (VR_RANGE, val, val);
>>> @@ -1479,7 +1479,7 @@ vr_values::extract_range_from_assignment
>>> gimple_assign_rhs1 (stmt),
>>> gimple_assign_rhs2 (stmt));
>>> else if (get_gimple_rhs_class (code) == GIMPLE_SINGLE_RHS
>>> - && is_gimple_min_invariant (gimple_assign_rhs1 (stmt)))
>>> + && constant_range_value_p (gimple_assign_rhs1 (stmt)))
>>> vr->set (gimple_assign_rhs1 (stmt));
>>> else
>>> vr->set_varying (TREE_TYPE (gimple_assign_lhs (stmt)));
>>> @@ -1756,7 +1756,7 @@ vr_values::adjust_range_with_scev (value
>>> chrec = instantiate_parameters (loop, analyze_scalar_evolution
>(loop, var));
>>>
>>> /* Like in PR19590, scev can return a constant function. */
>>> - if (is_gimple_min_invariant (chrec))
>>> + if (constant_range_value_p (chrec))
>>> {
>>> vr->set (chrec);
>>> return;
>>> @@ -1779,7 +1779,7 @@ vr_values::adjust_range_with_scev (value
>>> a simple expression, compare_values and possibly other
>functions
>>> in tree-vrp won't be able to handle it. */
>>> if (step == NULL_TREE
>>> - || !is_gimple_min_invariant (step)
>>> + || !constant_range_value_p (step)
>>> || !valid_value_p (init))
>>> return;
>>>
>>> @@ -1841,7 +1841,7 @@ vr_values::adjust_range_with_scev (value
>>>
>>> if (TREE_CODE (init) == SSA_NAME)
>>> initvr = *(get_value_range (init));
>>> - else if (is_gimple_min_invariant (init))
>>> + else if (constant_range_value_p (init))
>>> initvr.set (init);
>>> else
>>> return;
>>> @@ -1995,7 +1995,7 @@ vrp_valueize (tree name)
>>> const value_range *vr = x_vr_values->get_value_range (name);
>>> if (vr->kind () == VR_RANGE
>>> && (TREE_CODE (vr->min ()) == SSA_NAME
>>> - || is_gimple_min_invariant (vr->min ()))
>>> + || constant_range_value_p (vr->min ()))
>>> && vrp_operand_equal_p (vr->min (), vr->max ()))
>>> return vr->min ();
>>> }
>>> @@ -2077,7 +2077,7 @@ vr_values::vrp_visit_assignment_or_call
>>> extract_range_from_ssa_name (vr, tem);
>>> return;
>>> }
>>> - else if (is_gimple_min_invariant (tem))
>>> + else if (constant_range_value_p (tem))
>>> {
>>> vr->set (tem);
>>> return;
>>> @@ -2475,7 +2475,7 @@ vr_values::vrp_evaluate_conditional (tre
>>> enum warn_strict_overflow_code wc;
>>> const char* warnmsg;
>>>
>>> - if (is_gimple_min_invariant (ret))
>>> + if (constant_range_value_p (ret))
>>> {
>>> wc = WARN_STRICT_OVERFLOW_CONDITIONAL;
>>> warnmsg = G_("assuming signed overflow does not occur when
>"
>>> @@ -2517,7 +2517,7 @@ vr_values::vrp_evaluate_conditional (tre
>>> && INTEGRAL_TYPE_P (type)
>>> && vrp_val_is_min (vr0->min ())
>>> && vrp_val_is_max (vr0->max ())
>>> - && is_gimple_min_invariant (op1))
>>> + && constant_range_value_p (op1))
>>> {
>>> location_t location;
>>>
>>> @@ -3361,14 +3361,14 @@ vr_values::simplify_bit_ops_using_ranges
>>>
>>> if (TREE_CODE (op0) == SSA_NAME)
>>> vr0 = *(get_value_range (op0));
>>> - else if (is_gimple_min_invariant (op0))
>>> + else if (constant_range_value_p (op0))
>>> vr0.set (op0);
>>> else
>>> return false;
>>>
>>> if (TREE_CODE (op1) == SSA_NAME)
>>> vr1 = *(get_value_range (op1));
>>> - else if (is_gimple_min_invariant (op1))
>>> + else if (constant_range_value_p (op1))
>>> vr1.set (op1);
>>> else
>>> return false;
>>> @@ -3481,7 +3481,7 @@ test_for_singularity (enum tree_code con
>>> /* If the new min/max values have converged to a single
>value,
>>> then there is only one value which can satisfy the
>condition,
>>> return that value. */
>>> - if (operand_equal_p (min, max, 0) && is_gimple_min_invariant
>(min))
>>> + if (operand_equal_p (min, max, 0) && constant_range_value_p
>(min))
>>> return min;
>>> }
>>> return NULL;
>>> @@ -3554,7 +3554,7 @@ vr_values::simplify_cond_using_ranges_1
>>> && cond_code != EQ_EXPR
>>> && TREE_CODE (op0) == SSA_NAME
>>> && INTEGRAL_TYPE_P (TREE_TYPE (op0))
>>> - && is_gimple_min_invariant (op1))
>>> + && constant_range_value_p (op1))
>>> {
>>> const value_range *vr = get_value_range (op0);
>>>
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 16:41 ` Richard Biener
@ 2019-10-14 18:46 ` Aldy Hernandez
2019-10-15 10:46 ` Richard Sandiford
1 sibling, 0 replies; 14+ messages in thread
From: Aldy Hernandez @ 2019-10-14 18:46 UTC (permalink / raw)
To: Richard Biener, Richard Sandiford; +Cc: GCC Patches
On 10/14/19 12:38 PM, Richard Biener wrote:
> On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>> Richard Biener <richard.guenther@gmail.com> writes:
>>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
>>> <richard.sandiford@arm.com> wrote:
>>>>
>>>> The range-tracking code has a pretty hard-coded assumption that
>>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>>> that rather than contiually fight cases in which it can't handle
>>>> other invariants.
>>>>
>>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>>>
>>> ICK. Nobody is going to remember this new restriction and
>>> constant_range_value_p reads like constant_value_range_p ;)
>>>
>>> Btw, is_gimple_invariant_address shouldn't have been exported,
>>> it's only use could have used is_gimple_min_invariant...
>>
>> What do you think we should do instead?
>
> Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
I don't care either way, but I had originally suggested:
> I was going to suggest we normalize ranges to numerics completely before folding. That is, replacing normalize_addresses() here:
>
> *vr = op->fold_range (expr_type,
> vr0.normalize_addresses (),
> vr1.normalize_addresses ());
>
> ...into normalize_symbolics().
This will allow ranges of POLY_INTs to live throughout, but will get
dropped right before any arithmetic on them. At the least, it's a two
line change (assuming there are no hidden gotchas ;-)).
Aldy
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-14 16:41 ` Richard Biener
2019-10-14 18:46 ` Aldy Hernandez
@ 2019-10-15 10:46 ` Richard Sandiford
2019-10-15 11:26 ` Richard Biener
2019-10-17 8:17 ` Christophe Lyon
1 sibling, 2 replies; 14+ messages in thread
From: Richard Sandiford @ 2019-10-15 10:46 UTC (permalink / raw)
To: Richard Biener; +Cc: GCC Patches
Richard Biener <richard.guenther@gmail.com> writes:
> On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>>Richard Biener <richard.guenther@gmail.com> writes:
>>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
>>> <richard.sandiford@arm.com> wrote:
>>>>
>>>> The range-tracking code has a pretty hard-coded assumption that
>>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>>>> ADDR_EXPR". It seems better to add a predicate specifically for
>>>> that rather than contiually fight cases in which it can't handle
>>>> other invariants.
>>>>
>>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>>>
>>> ICK. Nobody is going to remember this new restriction and
>>> constant_range_value_p reads like constant_value_range_p ;)
>>>
>>> Btw, is_gimple_invariant_address shouldn't have been exported,
>>> it's only use could have used is_gimple_min_invariant...
>>
>>What do you think we should do instead?
>
> Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
OK, how about this? Aldy's suggestion would be fine by me too,
but I thought I'd try this first given Aldy's queasiness about
allowing POLY_INT_CSTs further in.
The main case in which this gives useful ranges is a lower bound
of A + B * X becoming A when B >= 0. E.g.:
(1) [32 + 16X, 100] -> [32, 100]
(2) [32 + 16X, 32 + 16X] -> [32, MAX]
But the same thing can be useful for the upper bound with negative
X coefficients.
We can revisit this later if keeping a singleton range for (2)
would be better.
Tested as before.
Richard
2019-10-15 Richard Sandiford <richard.sandiford@arm.com>
gcc/
PR middle-end/92033
* poly-int.h (constant_lower_bound_with_limit): New function.
(constant_upper_bound_with_limit): Likewise.
* doc/poly-int.texi: Document them.
* tree-vrp.c (value_range_base::set): Convert POLY_INT_CST bounds
into the worst-case INTEGER_CST bounds.
Index: gcc/poly-int.h
===================================================================
--- gcc/poly-int.h 2019-07-10 19:41:26.395898027 +0100
+++ gcc/poly-int.h 2019-10-15 11:30:14.099625553 +0100
@@ -1528,6 +1528,29 @@ constant_lower_bound (const poly_int_pod
return a.coeffs[0];
}
+/* Return the constant lower bound of A, given that it is no less than B. */
+
+template<unsigned int N, typename Ca, typename Cb>
+inline POLY_CONST_COEFF (Ca, Cb)
+constant_lower_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
+{
+ if (known_ge (a, b))
+ return a.coeffs[0];
+ return b;
+}
+
+/* Return the constant upper bound of A, given that it is no greater
+ than B. */
+
+template<unsigned int N, typename Ca, typename Cb>
+inline POLY_CONST_COEFF (Ca, Cb)
+constant_upper_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
+{
+ if (known_le (a, b))
+ return a.coeffs[0];
+ return b;
+}
+
/* Return a value that is known to be no greater than A and B. This
will be the greatest lower bound for some indeterminate values but
not necessarily for all. */
Index: gcc/doc/poly-int.texi
===================================================================
--- gcc/doc/poly-int.texi 2019-03-08 18:14:25.333011645 +0000
+++ gcc/doc/poly-int.texi 2019-10-15 11:30:14.099625553 +0100
@@ -803,6 +803,18 @@ the assertion is known to hold.
@item constant_lower_bound (@var{a})
Assert that @var{a} is nonnegative and return the smallest value it can have.
+@item constant_lower_bound_with_limit (@var{a}, @var{b})
+Return the least value @var{a} can have, given that the context in
+which @var{a} appears guarantees that the answer is no less than @var{b}.
+In other words, the caller is asserting that @var{a} is greater than or
+equal to @var{b} even if @samp{known_ge (@var{a}, @var{b})} doesn't hold.
+
+@item constant_upper_bound_with_limit (@var{a}, @var{b})
+Return the greatest value @var{a} can have, given that the context in
+which @var{a} appears guarantees that the answer is no greater than @var{b}.
+In other words, the caller is asserting that @var{a} is less than or equal
+to @var{b} even if @samp{known_le (@var{a}, @var{b})} doesn't hold.
+
@item lower_bound (@var{a}, @var{b})
Return a value that is always less than or equal to both @var{a} and @var{b}.
It will be the greatest such value for some indeterminate values
Index: gcc/tree-vrp.c
===================================================================
--- gcc/tree-vrp.c 2019-10-14 09:04:54.515259320 +0100
+++ gcc/tree-vrp.c 2019-10-15 11:30:14.099625553 +0100
@@ -727,6 +727,24 @@ value_range_base::set (enum value_range_
return;
}
+ /* Convert POLY_INT_CST bounds into worst-case INTEGER_CST bounds. */
+ if (POLY_INT_CST_P (min))
+ {
+ tree type_min = vrp_val_min (TREE_TYPE (min), true);
+ widest_int lb
+ = constant_lower_bound_with_limit (wi::to_poly_widest (min),
+ wi::to_widest (type_min));
+ min = wide_int_to_tree (TREE_TYPE (min), lb);
+ }
+ if (POLY_INT_CST_P (max))
+ {
+ tree type_max = vrp_val_max (TREE_TYPE (max), true);
+ widest_int ub
+ = constant_upper_bound_with_limit (wi::to_poly_widest (max),
+ wi::to_widest (type_max));
+ max = wide_int_to_tree (TREE_TYPE (max), ub);
+ }
+
/* Nothing to canonicalize for symbolic ranges. */
if (TREE_CODE (min) != INTEGER_CST
|| TREE_CODE (max) != INTEGER_CST)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-15 10:46 ` Richard Sandiford
@ 2019-10-15 11:26 ` Richard Biener
2019-10-17 8:17 ` Christophe Lyon
1 sibling, 0 replies; 14+ messages in thread
From: Richard Biener @ 2019-10-15 11:26 UTC (permalink / raw)
To: Richard Sandiford; +Cc: GCC Patches
On Tue, Oct 15, 2019 at 12:35 PM Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >>Richard Biener <richard.guenther@gmail.com> writes:
> >>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
> >>> <richard.sandiford@arm.com> wrote:
> >>>>
> >>>> The range-tracking code has a pretty hard-coded assumption that
> >>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
> >>>> ADDR_EXPR". It seems better to add a predicate specifically for
> >>>> that rather than contiually fight cases in which it can't handle
> >>>> other invariants.
> >>>>
> >>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
> >>>
> >>> ICK. Nobody is going to remember this new restriction and
> >>> constant_range_value_p reads like constant_value_range_p ;)
> >>>
> >>> Btw, is_gimple_invariant_address shouldn't have been exported,
> >>> it's only use could have used is_gimple_min_invariant...
> >>
> >>What do you think we should do instead?
> >
> > Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
>
> OK, how about this? Aldy's suggestion would be fine by me too,
> but I thought I'd try this first given Aldy's queasiness about
> allowing POLY_INT_CSTs further in.
>
> The main case in which this gives useful ranges is a lower bound
> of A + B * X becoming A when B >= 0. E.g.:
>
> (1) [32 + 16X, 100] -> [32, 100]
> (2) [32 + 16X, 32 + 16X] -> [32, MAX]
>
> But the same thing can be useful for the upper bound with negative
> X coefficients.
>
> We can revisit this later if keeping a singleton range for (2)
> would be better.
>
> Tested as before.
Works for me.
Richard.
> Richard
>
>
> 2019-10-15 Richard Sandiford <richard.sandiford@arm.com>
>
> gcc/
> PR middle-end/92033
> * poly-int.h (constant_lower_bound_with_limit): New function.
> (constant_upper_bound_with_limit): Likewise.
> * doc/poly-int.texi: Document them.
> * tree-vrp.c (value_range_base::set): Convert POLY_INT_CST bounds
> into the worst-case INTEGER_CST bounds.
>
> Index: gcc/poly-int.h
> ===================================================================
> --- gcc/poly-int.h 2019-07-10 19:41:26.395898027 +0100
> +++ gcc/poly-int.h 2019-10-15 11:30:14.099625553 +0100
> @@ -1528,6 +1528,29 @@ constant_lower_bound (const poly_int_pod
> return a.coeffs[0];
> }
>
> +/* Return the constant lower bound of A, given that it is no less than B. */
> +
> +template<unsigned int N, typename Ca, typename Cb>
> +inline POLY_CONST_COEFF (Ca, Cb)
> +constant_lower_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
> +{
> + if (known_ge (a, b))
> + return a.coeffs[0];
> + return b;
> +}
> +
> +/* Return the constant upper bound of A, given that it is no greater
> + than B. */
> +
> +template<unsigned int N, typename Ca, typename Cb>
> +inline POLY_CONST_COEFF (Ca, Cb)
> +constant_upper_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
> +{
> + if (known_le (a, b))
> + return a.coeffs[0];
> + return b;
> +}
> +
> /* Return a value that is known to be no greater than A and B. This
> will be the greatest lower bound for some indeterminate values but
> not necessarily for all. */
> Index: gcc/doc/poly-int.texi
> ===================================================================
> --- gcc/doc/poly-int.texi 2019-03-08 18:14:25.333011645 +0000
> +++ gcc/doc/poly-int.texi 2019-10-15 11:30:14.099625553 +0100
> @@ -803,6 +803,18 @@ the assertion is known to hold.
> @item constant_lower_bound (@var{a})
> Assert that @var{a} is nonnegative and return the smallest value it can have.
>
> +@item constant_lower_bound_with_limit (@var{a}, @var{b})
> +Return the least value @var{a} can have, given that the context in
> +which @var{a} appears guarantees that the answer is no less than @var{b}.
> +In other words, the caller is asserting that @var{a} is greater than or
> +equal to @var{b} even if @samp{known_ge (@var{a}, @var{b})} doesn't hold.
> +
> +@item constant_upper_bound_with_limit (@var{a}, @var{b})
> +Return the greatest value @var{a} can have, given that the context in
> +which @var{a} appears guarantees that the answer is no greater than @var{b}.
> +In other words, the caller is asserting that @var{a} is less than or equal
> +to @var{b} even if @samp{known_le (@var{a}, @var{b})} doesn't hold.
> +
> @item lower_bound (@var{a}, @var{b})
> Return a value that is always less than or equal to both @var{a} and @var{b}.
> It will be the greatest such value for some indeterminate values
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c 2019-10-14 09:04:54.515259320 +0100
> +++ gcc/tree-vrp.c 2019-10-15 11:30:14.099625553 +0100
> @@ -727,6 +727,24 @@ value_range_base::set (enum value_range_
> return;
> }
>
> + /* Convert POLY_INT_CST bounds into worst-case INTEGER_CST bounds. */
> + if (POLY_INT_CST_P (min))
> + {
> + tree type_min = vrp_val_min (TREE_TYPE (min), true);
> + widest_int lb
> + = constant_lower_bound_with_limit (wi::to_poly_widest (min),
> + wi::to_widest (type_min));
> + min = wide_int_to_tree (TREE_TYPE (min), lb);
> + }
> + if (POLY_INT_CST_P (max))
> + {
> + tree type_max = vrp_val_max (TREE_TYPE (max), true);
> + widest_int ub
> + = constant_upper_bound_with_limit (wi::to_poly_widest (max),
> + wi::to_widest (type_max));
> + max = wide_int_to_tree (TREE_TYPE (max), ub);
> + }
> +
> /* Nothing to canonicalize for symbolic ranges. */
> if (TREE_CODE (min) != INTEGER_CST
> || TREE_CODE (max) != INTEGER_CST)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-15 10:46 ` Richard Sandiford
2019-10-15 11:26 ` Richard Biener
@ 2019-10-17 8:17 ` Christophe Lyon
2019-10-17 8:23 ` Richard Sandiford
1 sibling, 1 reply; 14+ messages in thread
From: Christophe Lyon @ 2019-10-17 8:17 UTC (permalink / raw)
To: Richard Sandiford; +Cc: Richard Biener, GCC Patches
On Tue, 15 Oct 2019 at 12:36, Richard Sandiford
<richard.sandiford@arm.com> wrote:
>
> Richard Biener <richard.guenther@gmail.com> writes:
> > On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
> >>Richard Biener <richard.guenther@gmail.com> writes:
> >>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
> >>> <richard.sandiford@arm.com> wrote:
> >>>>
> >>>> The range-tracking code has a pretty hard-coded assumption that
> >>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
> >>>> ADDR_EXPR". It seems better to add a predicate specifically for
> >>>> that rather than contiually fight cases in which it can't handle
> >>>> other invariants.
> >>>>
> >>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
> >>>
> >>> ICK. Nobody is going to remember this new restriction and
> >>> constant_range_value_p reads like constant_value_range_p ;)
> >>>
> >>> Btw, is_gimple_invariant_address shouldn't have been exported,
> >>> it's only use could have used is_gimple_min_invariant...
> >>
> >>What do you think we should do instead?
> >
> > Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
>
> OK, how about this? Aldy's suggestion would be fine by me too,
> but I thought I'd try this first given Aldy's queasiness about
> allowing POLY_INT_CSTs further in.
>
> The main case in which this gives useful ranges is a lower bound
> of A + B * X becoming A when B >= 0. E.g.:
>
> (1) [32 + 16X, 100] -> [32, 100]
> (2) [32 + 16X, 32 + 16X] -> [32, MAX]
>
> But the same thing can be useful for the upper bound with negative
> X coefficients.
>
> We can revisit this later if keeping a singleton range for (2)
> would be better.
>
> Tested as before.
>
> Richard
>
>
Hi Richard,
This patch did improve aarch64 results quite a lot, however, there are
still a few failures that used to pass circa r276650:
gcc.target/aarch64/sve/loop_add_6.c -march=armv8.2-a+sve
scan-assembler \\tfsub\\tz[0-9]+\\.d, p[0-7]/m
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tadd\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
z[0-9]+\\.b\\n 1
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tadd\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
z[0-9]+\\.d\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tadd\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
z[0-9]+\\.h\\n 1
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tadd\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
z[0-9]+\\.s\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tand\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
z[0-9]+\\.b\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tand\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
z[0-9]+\\.d\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tand\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
z[0-9]+\\.h\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tand\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
z[0-9]+\\.s\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\teor\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
z[0-9]+\\.b\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\teor\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
z[0-9]+\\.d\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\teor\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
z[0-9]+\\.h\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\teor\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
z[0-9]+\\.s\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tfadd\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
z[0-9]+\\.d\\n 1
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tfadd\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
z[0-9]+\\.h\\n 1
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\tfadd\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
z[0-9]+\\.s\\n 1
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\torr\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
z[0-9]+\\.b\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\torr\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
z[0-9]+\\.d\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\torr\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
z[0-9]+\\.h\\n 2
gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
scan-assembler-times \\torr\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
z[0-9]+\\.s\\n 2
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tfsub\\tz[0-9]+\\.d, p[0-7]/m 1
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tfsub\\tz[0-9]+\\.s, p[0-7]/m 1
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tsub\\tz[0-9]+\\.b, p[0-7]/m 1
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tsub\\tz[0-9]+\\.d, p[0-7]/m 2
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tsub\\tz[0-9]+\\.h, p[0-7]/m 1
gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
scan-assembler-times \\tsub\\tz[0-9]+\\.s, p[0-7]/m 2
Just to make sure you are aware of these :-)
Thanks,
Christophe
> 2019-10-15 Richard Sandiford <richard.sandiford@arm.com>
>
> gcc/
> PR middle-end/92033
> * poly-int.h (constant_lower_bound_with_limit): New function.
> (constant_upper_bound_with_limit): Likewise.
> * doc/poly-int.texi: Document them.
> * tree-vrp.c (value_range_base::set): Convert POLY_INT_CST bounds
> into the worst-case INTEGER_CST bounds.
>
> Index: gcc/poly-int.h
> ===================================================================
> --- gcc/poly-int.h 2019-07-10 19:41:26.395898027 +0100
> +++ gcc/poly-int.h 2019-10-15 11:30:14.099625553 +0100
> @@ -1528,6 +1528,29 @@ constant_lower_bound (const poly_int_pod
> return a.coeffs[0];
> }
>
> +/* Return the constant lower bound of A, given that it is no less than B. */
> +
> +template<unsigned int N, typename Ca, typename Cb>
> +inline POLY_CONST_COEFF (Ca, Cb)
> +constant_lower_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
> +{
> + if (known_ge (a, b))
> + return a.coeffs[0];
> + return b;
> +}
> +
> +/* Return the constant upper bound of A, given that it is no greater
> + than B. */
> +
> +template<unsigned int N, typename Ca, typename Cb>
> +inline POLY_CONST_COEFF (Ca, Cb)
> +constant_upper_bound_with_limit (const poly_int_pod<N, Ca> &a, const Cb &b)
> +{
> + if (known_le (a, b))
> + return a.coeffs[0];
> + return b;
> +}
> +
> /* Return a value that is known to be no greater than A and B. This
> will be the greatest lower bound for some indeterminate values but
> not necessarily for all. */
> Index: gcc/doc/poly-int.texi
> ===================================================================
> --- gcc/doc/poly-int.texi 2019-03-08 18:14:25.333011645 +0000
> +++ gcc/doc/poly-int.texi 2019-10-15 11:30:14.099625553 +0100
> @@ -803,6 +803,18 @@ the assertion is known to hold.
> @item constant_lower_bound (@var{a})
> Assert that @var{a} is nonnegative and return the smallest value it can have.
>
> +@item constant_lower_bound_with_limit (@var{a}, @var{b})
> +Return the least value @var{a} can have, given that the context in
> +which @var{a} appears guarantees that the answer is no less than @var{b}.
> +In other words, the caller is asserting that @var{a} is greater than or
> +equal to @var{b} even if @samp{known_ge (@var{a}, @var{b})} doesn't hold.
> +
> +@item constant_upper_bound_with_limit (@var{a}, @var{b})
> +Return the greatest value @var{a} can have, given that the context in
> +which @var{a} appears guarantees that the answer is no greater than @var{b}.
> +In other words, the caller is asserting that @var{a} is less than or equal
> +to @var{b} even if @samp{known_le (@var{a}, @var{b})} doesn't hold.
> +
> @item lower_bound (@var{a}, @var{b})
> Return a value that is always less than or equal to both @var{a} and @var{b}.
> It will be the greatest such value for some indeterminate values
> Index: gcc/tree-vrp.c
> ===================================================================
> --- gcc/tree-vrp.c 2019-10-14 09:04:54.515259320 +0100
> +++ gcc/tree-vrp.c 2019-10-15 11:30:14.099625553 +0100
> @@ -727,6 +727,24 @@ value_range_base::set (enum value_range_
> return;
> }
>
> + /* Convert POLY_INT_CST bounds into worst-case INTEGER_CST bounds. */
> + if (POLY_INT_CST_P (min))
> + {
> + tree type_min = vrp_val_min (TREE_TYPE (min), true);
> + widest_int lb
> + = constant_lower_bound_with_limit (wi::to_poly_widest (min),
> + wi::to_widest (type_min));
> + min = wide_int_to_tree (TREE_TYPE (min), lb);
> + }
> + if (POLY_INT_CST_P (max))
> + {
> + tree type_max = vrp_val_max (TREE_TYPE (max), true);
> + widest_int ub
> + = constant_upper_bound_with_limit (wi::to_poly_widest (max),
> + wi::to_widest (type_max));
> + max = wide_int_to_tree (TREE_TYPE (max), ub);
> + }
> +
> /* Nothing to canonicalize for symbolic ranges. */
> if (TREE_CODE (min) != INTEGER_CST
> || TREE_CODE (max) != INTEGER_CST)
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: Add a constant_range_value_p function (PR 92033)
2019-10-17 8:17 ` Christophe Lyon
@ 2019-10-17 8:23 ` Richard Sandiford
0 siblings, 0 replies; 14+ messages in thread
From: Richard Sandiford @ 2019-10-17 8:23 UTC (permalink / raw)
To: Christophe Lyon; +Cc: Richard Biener, GCC Patches
Christophe Lyon <christophe.lyon@linaro.org> writes:
> On Tue, 15 Oct 2019 at 12:36, Richard Sandiford
> <richard.sandiford@arm.com> wrote:
>>
>> Richard Biener <richard.guenther@gmail.com> writes:
>> > On October 14, 2019 2:32:43 PM GMT+02:00, Richard Sandiford <richard.sandiford@arm.com> wrote:
>> >>Richard Biener <richard.guenther@gmail.com> writes:
>> >>> On Fri, Oct 11, 2019 at 4:42 PM Richard Sandiford
>> >>> <richard.sandiford@arm.com> wrote:
>> >>>>
>> >>>> The range-tracking code has a pretty hard-coded assumption that
>> >>>> is_gimple_min_invariant is equivalent to "INTEGER_CST or invariant
>> >>>> ADDR_EXPR". It seems better to add a predicate specifically for
>> >>>> that rather than contiually fight cases in which it can't handle
>> >>>> other invariants.
>> >>>>
>> >>>> Tested on aarch64-linux-gnu and x86_64-linux-gnu. OK to install?
>> >>>
>> >>> ICK. Nobody is going to remember this new restriction and
>> >>> constant_range_value_p reads like constant_value_range_p ;)
>> >>>
>> >>> Btw, is_gimple_invariant_address shouldn't have been exported,
>> >>> it's only use could have used is_gimple_min_invariant...
>> >>
>> >>What do you think we should do instead?
>> >
>> > Just handle POLY_INT_CST in a few place to quickly enough drop to varying.
>>
>> OK, how about this? Aldy's suggestion would be fine by me too,
>> but I thought I'd try this first given Aldy's queasiness about
>> allowing POLY_INT_CSTs further in.
>>
>> The main case in which this gives useful ranges is a lower bound
>> of A + B * X becoming A when B >= 0. E.g.:
>>
>> (1) [32 + 16X, 100] -> [32, 100]
>> (2) [32 + 16X, 32 + 16X] -> [32, MAX]
>>
>> But the same thing can be useful for the upper bound with negative
>> X coefficients.
>>
>> We can revisit this later if keeping a singleton range for (2)
>> would be better.
>>
>> Tested as before.
>>
>> Richard
>>
>>
> Hi Richard,
>
> This patch did improve aarch64 results quite a lot, however, there are
> still a few failures that used to pass circa r276650:
> gcc.target/aarch64/sve/loop_add_6.c -march=armv8.2-a+sve
> scan-assembler \\tfsub\\tz[0-9]+\\.d, p[0-7]/m
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tadd\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
> z[0-9]+\\.b\\n 1
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tadd\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
> z[0-9]+\\.d\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tadd\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
> z[0-9]+\\.h\\n 1
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tadd\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
> z[0-9]+\\.s\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tand\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
> z[0-9]+\\.b\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tand\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
> z[0-9]+\\.d\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tand\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
> z[0-9]+\\.h\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tand\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
> z[0-9]+\\.s\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\teor\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
> z[0-9]+\\.b\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\teor\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
> z[0-9]+\\.d\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\teor\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
> z[0-9]+\\.h\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\teor\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
> z[0-9]+\\.s\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tfadd\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
> z[0-9]+\\.d\\n 1
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tfadd\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
> z[0-9]+\\.h\\n 1
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\tfadd\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
> z[0-9]+\\.s\\n 1
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\torr\\tz[0-9]+\\.b, p[0-7]/m, z[0-9]+\\.b,
> z[0-9]+\\.b\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\torr\\tz[0-9]+\\.d, p[0-7]/m, z[0-9]+\\.d,
> z[0-9]+\\.d\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\torr\\tz[0-9]+\\.h, p[0-7]/m, z[0-9]+\\.h,
> z[0-9]+\\.h\\n 2
> gcc.target/aarch64/sve/reduc_1.c -march=armv8.2-a+sve
> scan-assembler-times \\torr\\tz[0-9]+\\.s, p[0-7]/m, z[0-9]+\\.s,
> z[0-9]+\\.s\\n 2
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tfsub\\tz[0-9]+\\.d, p[0-7]/m 1
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tfsub\\tz[0-9]+\\.s, p[0-7]/m 1
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tsub\\tz[0-9]+\\.b, p[0-7]/m 1
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tsub\\tz[0-9]+\\.d, p[0-7]/m 2
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tsub\\tz[0-9]+\\.h, p[0-7]/m 1
> gcc.target/aarch64/sve/reduc_5.c -march=armv8.2-a+sve
> scan-assembler-times \\tsub\\tz[0-9]+\\.s, p[0-7]/m 2
>
> Just to make sure you are aware of these :-)
Yeah, aware of them :-) These were UNRESOLVED before the patch due
to the VRP ICEs. I assume they're from the recent reduction changes,
but I haven't had chance to look at them in detail yet.
Thanks,
Richard
^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2019-10-17 8:17 UTC | newest]
Thread overview: 14+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-10-11 14:45 Add a constant_range_value_p function (PR 92033) Richard Sandiford
2019-10-13 16:22 ` Aldy Hernandez
2019-10-14 8:34 ` Richard Sandiford
2019-10-14 10:01 ` Aldy Hernandez
2019-10-14 12:32 ` Richard Sandiford
2019-10-14 12:53 ` Aldy Hernandez
2019-10-14 11:44 ` Richard Biener
2019-10-14 12:49 ` Richard Sandiford
2019-10-14 16:41 ` Richard Biener
2019-10-14 18:46 ` Aldy Hernandez
2019-10-15 10:46 ` Richard Sandiford
2019-10-15 11:26 ` Richard Biener
2019-10-17 8:17 ` Christophe Lyon
2019-10-17 8:23 ` Richard Sandiford
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).