Re: [PATCH 1/3] Perform fold when propagating.

[Richard Biener <richard dot guenther at gmail dot com>]

On Tue, Aug 13, 2019 at 1:24 PM Robin Dapp <rdapp@linux.ibm.com> wrote:
>
> > May I suggest to add a parameter to the substitute-and-fold engine
> > so we can do the folding on all stmts only when enabled and enable
> > it just for VRP?  That also avoids the testsuite noise.
>
> Would something along these lines do?
>
> diff --git a/gcc/tree-ssa-propagate.c b/gcc/tree-ssa-propagate.c
> index 7a8f1e037b0..6c0d743b823 100644
> --- a/gcc/tree-ssa-propagate.c
> +++ b/gcc/tree-ssa-propagate.c
> @@ -814,7 +814,6 @@ ssa_propagation_engine::ssa_propagate (void)
>    ssa_prop_fini ();
>  }
>
> -
>  /* Return true if STMT is of the form 'mem_ref = RHS', where 'mem_ref'
>     is a non-volatile pointer dereference, a structure reference or a
>     reference to a single _DECL.  Ignore volatile memory references
> @@ -1064,11 +1063,10 @@
> substitute_and_fold_dom_walker::before_dom_children (basic_block bb)
>        /* Replace real uses in the statement.  */
>        did_replace |= substitute_and_fold_engine->replace_uses_in (stmt);
>
> -      if (did_replace)
> -         gimple_set_modified (stmt, true);
> -
> -      if (fold_stmt (&i, follow_single_use_edges))
> +      /* If we made a replacement, fold the statement.  */
> +      if (did_replace ||
> substitute_and_fold_engine->should_fold_all_stmts ())
>         {
> +         fold_stmt (&i, follow_single_use_edges);
>           did_replace = true;
>           stmt = gsi_stmt (i);
>           gimple_set_modified (stmt, true);
> diff --git a/gcc/tree-ssa-propagate.h b/gcc/tree-ssa-propagate.h
> index 81b635e0787..939680f487c 100644
> --- a/gcc/tree-ssa-propagate.h
> +++ b/gcc/tree-ssa-propagate.h
> @@ -107,6 +107,13 @@ class substitute_and_fold_engine
>    bool substitute_and_fold (basic_block = NULL);
>    bool replace_uses_in (gimple *);
>    bool replace_phi_args_in (gphi *);
> +
> +  /* Users like VRP can overwrite this when they want to perform
> +     folding for every propagation.  */
> +  virtual bool should_fold_all_stmts (void)
> +    {
> +      return false;
> +    }

Since this is constant for a single invocation I'd either
add a flag param to substitute_and_fold or a bool
class member initialized at construction time.

Also do

  if ((did_replace || fold_all_stmts)
     && fold_stmt (...))
   {
   }

to avoid extra work when folding does nothing.

Otherwise yes, this woudl work.

>  };
>
>  #endif /* _TREE_SSA_PROPAGATE_H  */
> diff --git a/gcc/tree-vrp.c b/gcc/tree-vrp.c
> index e2850682da2..8c8fa6f2bec 100644
> --- a/gcc/tree-vrp.c
> +++ b/gcc/tree-vrp.c
> @@ -6271,6 +6271,9 @@ class vrp_folder : public substitute_and_fold_engine
>      { return vr_values->simplify_stmt_using_ranges (gsi); }
>   tree op_with_constant_singleton_value_range (tree op)
>      { return vr_values->op_with_constant_singleton_value_range (op); }
> +
> +  /* Enable aggressive folding in every propagation.  */
> +  bool should_fold_all_stmts (void) { return true; }
>  };
>
>  /* If the statement pointed by SI has a predicate whose value can be
>
>
> > I think it's also only necessary to fold a stmt when a (indirect) use
> > after substitution has either been folded or has (new) SSA name
> > info (range/known-bits) set?
>
> Where would this need to be changed?

It was just a random thought, doing this would need to keep track
of "changed" SSA names (in a bitmap?) and before folding
checking all uses on the stmt if they are "changed".  The overhead
for this may be higher than the folding savings we get.  The "changed"
would also need to tickle down some distance so patterns with
deeper nested subexpressions would be tried.

Richard.

>
> Regards
>  Robin
>