Hi Richard,

On 12/08/16 20:43, Richard Biener wrote:
> On Wed, Aug 3, 2016 at 3:17 AM, kugan <kugan.vivekanandarajah@linaro.org> wrote:

[SNIP]

>
> diff --git a/gcc/common.opt b/gcc/common.opt
> index 8a292ed..7028cd4 100644
> --- a/gcc/common.opt
> +++ b/gcc/common.opt
> @@ -2482,6 +2482,10 @@ ftree-vrp
>  Common Report Var(flag_tree_vrp) Init(0) Optimization
>  Perform Value Range Propagation on trees.
>
> +fdisable-tree-evrp
> +Common Report Var(flag_disable_early_vrp) Init(0) Optimization
> +Disable Early Value Range Propagation on trees.
> +
>
> no please, this is automatically supported via -fdisable-

I am now having -ftree-evrp which is enabled all the time. But This will 
only be used for disabling the early-vrp. That is, early-vrp will be run 
when ftree-vrp is enabled and ftree-evrp is not explicitly disabled. Is 
this OK?

>
> @@ -1728,11 +1736,12 @@ extract_range_from_assert (value_range *vr_p, tree expr)
>      always false.  */
>
>  static void
> -extract_range_from_ssa_name (value_range *vr, tree var)
> +extract_range_from_ssa_name (value_range *vr, bool dom_p, tree var)
>  {
>    value_range *var_vr = get_value_range (var);
>
> -  if (var_vr->type != VR_VARYING)
> +  if (var_vr->type != VR_VARYING
> +      && (!dom_p || var_vr->type != VR_UNDEFINED))
>      copy_value_range (vr, var_vr);
>    else
>      set_value_range (vr, VR_RANGE, var, var, NULL);
>
> why do you need these changes?  I think I already told you you need to
> initialize the lattice to sth else than VR_UNDEFINED and that you can't
> fully re-use update_value_range.  If you don't want to do that then instead
> of doing changes all over the place do it in get_value_range and have a
> global flag.

I have now added a global early_vrp_p and use this to initialize 
VR_INITIALIZER and get_value_range default to VR_VARYING.

>
>
> @@ -3594,7 +3643,8 @@ extract_range_from_cond_expr (value_range *vr,
> gassign *stmt)
>     on the range of its operand and the expression code.  */
>
>  static void
> -extract_range_from_comparison (value_range *vr, enum tree_code code,
> +extract_range_from_comparison (value_range *vr,
> +                              enum tree_code code,
>                                tree type, tree op0, tree op1)
>  {
>    bool sop = false;
>
> remove these kind of no-op changes.

Done.

>
> +/* Initialize local data structures for VRP.  If DOM_P is true,
> +   we will be calling this from early_vrp where value range propagation
> +   is done by visiting stmts in dominator tree.  ssa_propagate engine
> +   is not used in this case and that part of the ininitialization will
> +   be skipped.  */
>
>  static void
> -vrp_initialize (void)
> +vrp_initialize (bool dom_p)
>  {
>    basic_block bb;
>
> @@ -6949,6 +7010,9 @@ vrp_initialize (void)
>    vr_phi_edge_counts = XCNEWVEC (int, num_ssa_names);
>    bitmap_obstack_initialize (&vrp_equiv_obstack);
>
> +  if (dom_p)
> +    return;
> +
>
> split the function instead.
>
> @@ -7926,7 +7992,8 @@ vrp_visit_switch_stmt (gswitch *stmt, edge *taken_edge_p)
>     If STMT produces a varying value, return SSA_PROP_VARYING.  */
>
>  static enum ssa_prop_result
> -vrp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p)
> +vrp_visit_stmt_worker (gimple *stmt, bool dom_p,  edge *taken_edge_p,
> +                      tree *output_p)
>  {
>    tree def;
>    ssa_op_iter iter;
> @@ -7940,7 +8007,7 @@ vrp_visit_stmt (gimple *stmt, edge
> *taken_edge_p, tree *output_p)
>    if (!stmt_interesting_for_vrp (stmt))
>      gcc_assert (stmt_ends_bb_p (stmt));
>    else if (is_gimple_assign (stmt) || is_gimple_call (stmt))
> -    return vrp_visit_assignment_or_call (stmt, output_p);
> +    return vrp_visit_assignment_or_call (stmt, dom_p, output_p);
>    else if (gimple_code (stmt) == GIMPLE_COND)
>      return vrp_visit_cond_stmt (as_a <gcond *> (stmt), taken_edge_p);
>    else if (gimple_code (stmt) == GIMPLE_SWITCH)
> @@ -7954,6 +8021,12 @@ vrp_visit_stmt (gimple *stmt, edge
> *taken_edge_p, tree *output_p)
>    return SSA_PROP_VARYING;
>  }
>
> +static enum ssa_prop_result
> +vrp_visit_stmt (gimple *stmt, edge *taken_edge_p, tree *output_p)
> +{
> +  return vrp_visit_stmt_worker (stmt, false, taken_edge_p, output_p);
> +}
>
> as said the refactoring that would be appreciated is to split out the
> update_value_range calls
> from the worker functions so you can call the respective functions
> from the DOM implementations.
> That they are globbed in vrp_visit_stmt currently is due to the API of
> the SSA propagator.
Sent this as separate patch for easy reviewing and testing.

>
> @@ -8768,6 +8842,12 @@ vrp_visit_phi_node (gphi *phi)
>               fprintf (dump_file, "\n");
>             }
>
> +         if (dom_p && vr_arg.type == VR_UNDEFINED)
> +           {
> +             set_value_range_to_varying (&vr_result);
> +             break;
> +           }
> +
>
> eh...  ok, so another way to attack this is, instead of initializing
> the lattice to sth else
> than VR_UNDEFINED, make sure to drop the lattice to varying for all PHI args on
> yet unvisited incoming edges (you're not doing optimistic VRP).  That's the only
> place you _have_ to do it.

Even when it is initialize (as I am doing now), we can still end up with 
VR_UNDEFINED during the propagation.  I have just left the above so that 
we dont end up with the wrong VR.

I also noticed that g++.dg/warn/pr33738.C testcase is now failing. This 
is because, with early-vrp setting value range ccp2 is optimizing 
without issuing a warning. I will look into it.

bootstrap and regression testing is in progress.

Thanks,
Kugan