[PATCH] Generalized predicate/condition for parameter reference in IPA (PR ipa/91088)

[Martin Jambor]

Hi,

On Fri, Jul 12 2019, Feng Xue OS wrote:
> Current IPA-cp only generates cost-evaluating predicate for conditional statement like
> "if (param cmp const_val)", it is too simple and conservative. This patch generalizes the
> process to handle the form as T(param), a mathematical transformation on the function
> parameter, in which the parameter occurs once, and other operands are constant value.

thanks for working on this.  I cannot approve this, but I have had a
brief look and have the following comments:
>
> ----
> diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> index 3d92250b520..0110446e09e 100644
> --- a/gcc/ChangeLog
> +++ b/gcc/ChangeLog
> @@ -1,3 +1,25 @@
> +2019-07-12  Feng Xue  <fxue@os.amperecomputing.com>
> +
> +	PR ipa/91088
> +	* ipa-predicat.h (struct expr_eval_op): New struct.
> +	(expr_eval_ops): New typedef.
> +	(struct condition): Add param_ops member.
> +	(add_condition): Add param_ops parameter.
> +	* ipa-predicat.c (expr_eval_ops_equal_p): New function.
> +	(predicate::add_clause): Add param_ops comparison.
> +	(dump_condition): Add debug dump for param_ops.
> +	(remap_after_inlining): Add param_ops argument to call to
> +	add_condition.
> +	(add_condition): Add parameter param_ops.
> +	* ipa-fnsummary.c (evaluate_conditions_for_known_args): Fold
> +	parameter expressions using param_ops.
> +	(decompose_param_expr):  New function.
> +	(set_cond_stmt_execution_predicate): Use call to decompose_param_expr
> +	to replace call to unmodified_parm_or_parm_agg_item.
> +	(set_switch_stmt_execution_predicate): Likewise.
> +	(inline_read_section): Read param_ops from summary stream.
> +	(ipa_fn_summary_write): Write param_ops to summary stream.
> +

(It's a bad idea to make ChangeLog entries part of the patch, it won't
apply to anyone, not even to you nowadays. )


>  2019-07-11  Sunil K Pandey  <sunil.k.pandey@intel.com>
>  
>  	PR target/90980
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index 09986211a1d..faf8bd39090 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -301,9 +301,9 @@ set_hint_predicate (predicate **p, predicate new_predicate)
>  }
>  
>  
> -/* Compute what conditions may or may not hold given invormation about
> +/* Compute what conditions may or may not hold given information about
>     parameters.  RET_CLAUSE returns truths that may hold in a specialized copy,
> -   whie RET_NONSPEC_CLAUSE returns truths that may hold in an nonspecialized
> +   while RET_NONSPEC_CLAUSE returns truths that may hold in an nonspecialized
>     copy when called in a given context.  It is a bitmask of conditions. Bit
>     0 means that condition is known to be false, while bit 1 means that condition
>     may or may not be true.  These differs - for example NOT_INLINED condition
> @@ -336,6 +336,8 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>      {
>        tree val;
>        tree res;
> +      int j;
> +      struct expr_eval_op *op;
>  
>        /* We allow call stmt to have fewer arguments than the callee function
>           (especially for K&R style programs).  So bound check here (we assume
> @@ -399,7 +401,18 @@ evaluate_conditions_for_known_args (struct cgraph_node *node,
>  	  continue;
>  	}
>  
> -      val = fold_unary (VIEW_CONVERT_EXPR, TREE_TYPE (c->val), val);
> +      for (j = 0; vec_safe_iterate (c->param_ops, j, &op); j++)
> +	{
> +	  if (!op->val)
> +	    val = fold_unary (op->code, op->type, val);
> +	  else if (op->val_is_rhs)
> +	    val = fold_binary_to_constant (op->code, op->type, val, op->val);
> +	  else
> +	    val = fold_binary_to_constant (op->code, op->type, op->val, val);
> +	  if (!val)
> +	    break;
> +	}
> +
>        res = val
>  	? fold_binary_to_constant (c->code, boolean_type_node, val, c->val)
>  	: NULL;
> @@ -1177,6 +1190,105 @@ eliminated_by_inlining_prob (ipa_func_body_info *fbi, gimple *stmt)
>      }
>  }
>  
> +/* Flatten a tree expression on parameter into a set of sequential operations.
> +   we only handle expression that is a mathematical transformation on the
> +   parameter, and in the expression, parameter occurs only once, and other
> +   operands are IPA invariant.  */

I understand describing these things is difficult, but flatten is
strange way to describe what the function does.  What about somthing
like the following?

Analyze EXPR if it represents a series of simple operations performed on
a function parameter and return true if so.  FBI, STMT, INDEX_P, SIZE_P
and AGGPOS have the same meaning like in
unmodified_parm_or_parm_agg_item.  Operations on the parameter are
recorded to PARAM_OPS_P if it is not NULL.


> +
> +static bool
> +decompose_param_expr (struct ipa_func_body_info *fbi,
> +		      gimple *stmt, tree expr,
> +		      int *index_p, HOST_WIDE_INT *size_p,
> +		      struct agg_position_info *aggpos,
> +		      expr_eval_ops *param_ops_p)
> +{
> +  if (param_ops_p)
> +    *param_ops_p = NULL;
> +
> +  while (true)
> +    {
> +      gimple_rhs_class rhs_class;
> +      expr_eval_op eval_op;
> +
> +      if (unmodified_parm_or_parm_agg_item (fbi, stmt, expr, index_p,
> +					    size_p, aggpos))
> +	{
> +	  if (param_ops_p)
> +	    {
> +	      /* Find use of parameter, add a convert operation to describe
> +		 result type, which may not be same as parameter type.  */
> +	      eval_op.val_is_rhs = false;
> +	      eval_op.val = NULL_TREE;
> +	      eval_op.code = VIEW_CONVERT_EXPR;
> +	      eval_op.type = TREE_TYPE (expr);
> +
> +	      vec_safe_insert (*param_ops_p, 0, eval_op);

If we get here in the first iteration of the loop, could we not insert
anything into the vector and handle such cases in
evaluate_conditions_for_known_args like we do today (well, with
fold_convert might be better)?  It could save quite some memory and it
is important to try keep the memory footprint down in IPA summaries.

Also, I think you want a parameter to limit the maximum length of
param_ops_p, at some point someone will come with some crazy
machine-generated code that will create huge vectors.

Thanks,

Martin