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 <f...@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.pan...@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
