On Tue, Nov 28, 2017 at 12:35 AM, Martin Jambor <mjam...@suse.cz> wrote:
> Hi,
>
> sorry for getting so late to this. However...
>
> On Tue, Nov 14 2017, Richard Biener wrote:
>> On Tue, Nov 14, 2017 at 10:31 AM, Prathamesh Kulkarni
>> <prathamesh.kulka...@linaro.org> wrote:
>>> On 3 November 2017 at 15:38, Richard Biener <richard.guent...@gmail.com>
>>> wrote:
>>>> On Fri, Nov 3, 2017 at 6:15 AM, Prathamesh Kulkarni
>>>> <prathamesh.kulka...@linaro.org> wrote:
>>>>> Hi Martin,
>>>>> As mentioned in PR, the issue here for propagating value of 'm' from
>>>>> f_c1 to foo() is that the jump function operation is FLOAT_EXPR, and
>>>>> the type of input param 'm' is int, so fold_unary() doesn't do the
>>>>> conversion to real_type. The attached patch fixes that by calling
>>>>> fold_convert if operation is FLOAT_EXPR / FIX_TRUNC_EXPR /
>>>>> CONVERT_EXPR and converts it to the type of corresponding parameter in
>>>>> callee.
>>>>>
>>>>> There are still two issues:
>>>>> a) Using NOP_EXPR for early_exit in ipa_get_jf_pass_through_result.
>>>>> I suppose we need to change to some other code to indicate that there
>>>>> is no operation ?
>>>>> b) Patch does not passing param_type from all callers.
>>>>> I suppose we could fix these incrementally ?
>>>>>
>>>>> Bootstrap+tested on x86_64-unknown-linux-gnu.
>>>>> OK for trunk ?
>>>>
>>>> This doesn't look like a well designed fix. Both fold_unary and
>>>> fold_binary
>>>> calls get a possibly bogus type and you single out only a few ops.
>>>>
>>>> Either _fully_ list a set of operations that are know to have matching
>>>> input/output types or always require param_type to be non-NULL.
>>>>
>>>> For a) simply remove the special-casing and merge it with CONVERT_EXPR
>>>> handling (however it will end up looking).
>>>>
>>>> Please don't use fold_convert, using fold_unary is fine.
>>> Hi Richard,
>>> Sorry for the delay. In the attached version, parm_type is made non
>>> NULL in ipa_get_jf_pass_through_result().
>>>
>>> ipa_get_jf_pass_through_result() is called from two
>>> places - propagate_vals_across_pass_through() and ipa_value_from_jfunc().
>>> However it appears ipa_value_from_jfunc() is called from multiple
>>> functions and it's
>>> hard to detect parm_type in the individual callers. So I have passed
>>> correct parm_type from propagate_vals_across_pass_through(), and kept
>>> the old behavior for ipa_value_from_jfunc().
>>>
>>> Would it be OK to fix that incrementally ?
>>
>> I don't think it is good to do that. If we can't get the correct type
>> then we have
>
> ...I agree with Richi that it is better to fix all the potential type
> issues like in the patch below. Because we now have the types almost
> everywhere - notable exceptions are K&R C programs and functions with
> variable number of parameters - we might just as well use them, and so I
> went over other uses of ipa_get_jf_pass_through_result and made them
> look for the appropriate type too.
>
> However, because there are cases where we just do not have the type at
> hand, we have to deal with it by only going forward if we know the
> operation at hand does not change type. I have added a special
> predicate for this purpose to tree.c but I am opened to suggestions for
> better place or name or how/whether to integrate them to gimple
> verifier.
>
> Bootstrapped and tested on x86_64-linux. If the new tree.c predicate is
> deemed OK, I'd like to propose to commit this to trunk (and then
> backport it to the gcc 7 branch).
>
> What do you think?
Ok with nits below...
> Martin
>
>
> 2017-11-27 Prathamesh Kulkarni <prathamesh.kulka...@linaro.org>
> Martin Jambor <mjam...@suse.cz>
>
> PR ipa/82808
> * tree.h (tree_operation_preserves_op1_type_p): Declare
> * tree.c (tree_operation_preserves_op1_type_p): New function.
> * ipa-prop.h (ipa_get_type): Allow i to be out of bounds.
> (ipa_value_from_jfunc): Adjust declaration.
> * ipa-cp.c (ipa_get_jf_pass_through_result): New parameter RES_TYPE.
> Use it as result type for arithmetics, unless it is NULL in which case
> be more conservative.
> (ipa_value_from_jfunc): New parameter PARM_TYPE, pass it to
> ipa_get_jf_pass_through_result.
> (propagate_vals_across_pass_through): Likewise.
> (propagate_scalar_across_jump_function): New parameter PARM_TYPE, pass
> is to propagate_vals_across_pass_through.
> (propagate_constants_across_call): Pass PARM_TYPE to
> propagate_scalar_across_jump_function.
> (find_more_scalar_values_for_callers_subset): Pass parameter type to
> ipa_value_from_jfunc.
> (cgraph_edge_brings_all_scalars_for_node): Likewise.
> * ipa-fnsummary.c (evaluate_properties_for_edge): Renamed parms_info
> to caller_parms_info, pass parameter type to ipa_value_from_jfunc.
> * ipa-prop.c (try_make_edge_direct_simple_call): New parameter
> target_type, pass it to ipa_value_from_jfunc.
> (update_indirect_edges_after_inlining): Pass parameter type to
> try_make_edge_direct_simple_call.
>
> testsuite/
> * gcc.dg/ipa/pr82808.c: New test.
> ---
> gcc/ipa-cp.c | 67
> +++++++++++++++++++++++---------------
> gcc/ipa-fnsummary.c | 14 ++++----
> gcc/ipa-prop.c | 21 ++++++++----
> gcc/ipa-prop.h | 5 +--
> gcc/testsuite/gcc.dg/ipa/pr82808.c | 27 +++++++++++++++
> gcc/tree.c | 46 ++++++++++++++++++++++++++
> gcc/tree.h | 1 +
> 7 files changed, 140 insertions(+), 41 deletions(-)
> create mode 100644 gcc/testsuite/gcc.dg/ipa/pr82808.c
>
> diff --git a/gcc/ipa-cp.c b/gcc/ipa-cp.c
> index 05228d0582d..100a09a46c7 100644
> --- a/gcc/ipa-cp.c
> +++ b/gcc/ipa-cp.c
> @@ -1220,33 +1220,38 @@ initialize_node_lattices (struct cgraph_node *node)
> }
>
> /* Return the result of a (possibly arithmetic) pass through jump function
> - JFUNC on the constant value INPUT. Return NULL_TREE if that cannot be
> + JFUNC on the constant value INPUT. RES_TYPE is the type of the parameter
> + to which the result is passed. Return NULL_TREE if that cannot be
> determined or be considered an interprocedural invariant. */
>
> static tree
> -ipa_get_jf_pass_through_result (struct ipa_jump_func *jfunc, tree input)
> +ipa_get_jf_pass_through_result (struct ipa_jump_func *jfunc, tree input,
> + tree res_type)
> {
> - tree restype, res;
> + tree res;
>
> if (ipa_get_jf_pass_through_operation (jfunc) == NOP_EXPR)
> return input;
> if (!is_gimple_ip_invariant (input))
> return NULL_TREE;
>
> - if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> - == tcc_unary)
> - res = fold_unary (ipa_get_jf_pass_through_operation (jfunc),
> - TREE_TYPE (input), input);
> - else
> + tree_code opcode = ipa_get_jf_pass_through_operation (jfunc);
> + if (!res_type)
> {
> - if (TREE_CODE_CLASS (ipa_get_jf_pass_through_operation (jfunc))
> - == tcc_comparison)
> - restype = boolean_type_node;
> + if (TREE_CODE_CLASS (opcode) == tcc_comparison)
> + res_type = boolean_type_node;
> + else if (tree_operation_preserves_op1_type_p (opcode))
> + res_type = TREE_TYPE (input);
> else
> - restype = TREE_TYPE (input);
> - res = fold_binary (ipa_get_jf_pass_through_operation (jfunc), restype,
> - input, ipa_get_jf_pass_through_operand (jfunc));
> + return NULL_TREE;
> }
> +
> + if (TREE_CODE_CLASS (opcode) == tcc_unary)
> + res = fold_unary (opcode, res_type, input);
> + else
> + res = fold_binary (opcode, res_type, input,
> + ipa_get_jf_pass_through_operand (jfunc));
> +
> if (res && !is_gimple_ip_invariant (res))
> return NULL_TREE;
>
> @@ -1275,10 +1280,12 @@ ipa_get_jf_ancestor_result (struct ipa_jump_func
> *jfunc, tree input)
> /* Determine whether JFUNC evaluates to a single known constant value and if
> so, return it. Otherwise return NULL. INFO describes the caller node or
> the one it is inlined to, so that pass-through jump functions can be
> - evaluated. */
> + evaluated. PARM_TYPE is the type of the parameter to which the result is
> + passed. */
>
> tree
> -ipa_value_from_jfunc (struct ipa_node_params *info, struct ipa_jump_func
> *jfunc)
> +ipa_value_from_jfunc (struct ipa_node_params *info, struct ipa_jump_func
> *jfunc,
> + tree parm_type)
> {
> if (jfunc->type == IPA_JF_CONST)
> return ipa_get_jf_constant (jfunc);
> @@ -1312,7 +1319,7 @@ ipa_value_from_jfunc (struct ipa_node_params *info,
> struct ipa_jump_func *jfunc)
> return NULL_TREE;
>
> if (jfunc->type == IPA_JF_PASS_THROUGH)
> - return ipa_get_jf_pass_through_result (jfunc, input);
> + return ipa_get_jf_pass_through_result (jfunc, input, parm_type);
> else
> return ipa_get_jf_ancestor_result (jfunc, input);
> }
> @@ -1562,12 +1569,14 @@ ipcp_lattice<valtype>::add_value (valtype newval,
> cgraph_edge *cs,
>
> /* Propagate values through a pass-through jump function JFUNC associated
> with
> edge CS, taking values from SRC_LAT and putting them into DEST_LAT.
> SRC_IDX
> - is the index of the source parameter. */
> + is the index of the source parameter. PARM_TYPE is the type of the
> + parameter to which the result is passed. */
>
> static bool
> propagate_vals_across_pass_through (cgraph_edge *cs, ipa_jump_func *jfunc,
> ipcp_lattice<tree> *src_lat,
> - ipcp_lattice<tree> *dest_lat, int src_idx)
> + ipcp_lattice<tree> *dest_lat, int src_idx,
> + tree parm_type)
> {
> ipcp_value<tree> *src_val;
> bool ret = false;
> @@ -1581,7 +1590,8 @@ propagate_vals_across_pass_through (cgraph_edge *cs,
> ipa_jump_func *jfunc,
> else
> for (src_val = src_lat->values; src_val; src_val = src_val->next)
> {
> - tree cstval = ipa_get_jf_pass_through_result (jfunc, src_val->value);
> + tree cstval = ipa_get_jf_pass_through_result (jfunc, src_val->value,
> + parm_type);
>
> if (cstval)
> ret |= dest_lat->add_value (cstval, cs, src_val, src_idx);
> @@ -1622,12 +1632,14 @@ propagate_vals_across_ancestor (struct cgraph_edge
> *cs,
> }
>
> /* Propagate scalar values across jump function JFUNC that is associated with
> - edge CS and put the values into DEST_LAT. */
> + edge CS and put the values into DEST_LAT. PARM_TYPE is the type of the
> + parameter to which the result is passed. */
>
> static bool
> propagate_scalar_across_jump_function (struct cgraph_edge *cs,
> struct ipa_jump_func *jfunc,
> - ipcp_lattice<tree> *dest_lat)
> + ipcp_lattice<tree> *dest_lat,
> + tree param_type)
> {
> if (dest_lat->bottom)
> return false;
> @@ -1662,7 +1674,7 @@ propagate_scalar_across_jump_function (struct
> cgraph_edge *cs,
>
> if (jfunc->type == IPA_JF_PASS_THROUGH)
> ret = propagate_vals_across_pass_through (cs, jfunc, src_lat,
> - dest_lat, src_idx);
> + dest_lat, src_idx,
> param_type);
> else
> ret = propagate_vals_across_ancestor (cs, jfunc, src_lat, dest_lat,
> src_idx);
> @@ -2279,7 +2291,8 @@ propagate_constants_across_call (struct cgraph_edge *cs)
> else
> {
> ret |= propagate_scalar_across_jump_function (cs, jump_func,
> - &dest_plats->itself);
> + &dest_plats->itself,
> + param_type);
> ret |= propagate_context_across_jump_function (cs, jump_func, i,
> &dest_plats->ctxlat);
> ret
> @@ -3857,6 +3870,7 @@ find_more_scalar_values_for_callers_subset (struct
> cgraph_node *node,
> tree newval = NULL_TREE;
> int j;
> bool first = true;
> + tree type = ipa_get_type (info, i);
>
> if (ipa_get_scalar_lat (info, i)->bottom || known_csts[i])
> continue;
> @@ -3876,7 +3890,7 @@ find_more_scalar_values_for_callers_subset (struct
> cgraph_node *node,
> break;
> }
> jump_func = ipa_get_ith_jump_func (IPA_EDGE_REF (cs), i);
> - t = ipa_value_from_jfunc (IPA_NODE_REF (cs->caller), jump_func);
> + t = ipa_value_from_jfunc (IPA_NODE_REF (cs->caller), jump_func,
> type);
> if (!t
> || (newval
> && !values_equal_for_ipcp_p (t, newval))
> @@ -4352,7 +4366,8 @@ cgraph_edge_brings_all_scalars_for_node (struct
> cgraph_edge *cs,
> if (i >= ipa_get_cs_argument_count (args))
> return false;
> jump_func = ipa_get_ith_jump_func (args, i);
> - t = ipa_value_from_jfunc (caller_info, jump_func);
> + t = ipa_value_from_jfunc (caller_info, jump_func,
> + ipa_get_type (dest_info, i));
> if (!t || !values_equal_for_ipcp_p (val, t))
> return false;
> }
> diff --git a/gcc/ipa-fnsummary.c b/gcc/ipa-fnsummary.c
> index 4b5cd70ea85..df8386d9f44 100644
> --- a/gcc/ipa-fnsummary.c
> +++ b/gcc/ipa-fnsummary.c
> @@ -444,15 +444,16 @@ evaluate_properties_for_edge (struct cgraph_edge *e,
> bool inline_p,
> && !e->call_stmt_cannot_inline_p
> && ((clause_ptr && info->conds) || known_vals_ptr ||
> known_contexts_ptr))
> {
> - struct ipa_node_params *parms_info;
> + struct ipa_node_params *caller_parms_info, *callee_pi;
> struct ipa_edge_args *args = IPA_EDGE_REF (e);
> struct ipa_call_summary *es = ipa_call_summaries->get (e);
> int i, count = ipa_get_cs_argument_count (args);
>
> if (e->caller->global.inlined_to)
> - parms_info = IPA_NODE_REF (e->caller->global.inlined_to);
> + caller_parms_info = IPA_NODE_REF (e->caller->global.inlined_to);
> else
> - parms_info = IPA_NODE_REF (e->caller);
> + caller_parms_info = IPA_NODE_REF (e->caller);
> + callee_pi = IPA_NODE_REF (e->callee);
>
> if (count && (info->conds || known_vals_ptr))
> known_vals.safe_grow_cleared (count);
> @@ -464,7 +465,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool
> inline_p,
> for (i = 0; i < count; i++)
> {
> struct ipa_jump_func *jf = ipa_get_ith_jump_func (args, i);
> - tree cst = ipa_value_from_jfunc (parms_info, jf);
> + tree cst = ipa_value_from_jfunc (caller_parms_info, jf,
> + ipa_get_type (callee_pi, i));
>
> if (!cst && e->call_stmt
> && i < (int)gimple_call_num_args (e->call_stmt))
> @@ -483,8 +485,8 @@ evaluate_properties_for_edge (struct cgraph_edge *e, bool
> inline_p,
> known_vals[i] = error_mark_node;
>
> if (known_contexts_ptr)
> - (*known_contexts_ptr)[i] = ipa_context_from_jfunc (parms_info, e,
> - i, jf);
> + (*known_contexts_ptr)[i]
> + = ipa_context_from_jfunc (caller_parms_info, e, i, jf);
> /* TODO: When IPA-CP starts propagating and merging aggregate jump
> functions, use its knowledge of the caller too, just like the
> scalar case above. */
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 4b3c2361418..31879a747c0 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3207,19 +3207,20 @@ try_decrement_rdesc_refcount (struct ipa_jump_func
> *jfunc)
>
> /* Try to find a destination for indirect edge IE that corresponds to a
> simple
> call or a call of a member function pointer and where the destination is a
> - pointer formal parameter described by jump function JFUNC. If it can be
> - determined, return the newly direct edge, otherwise return NULL.
> + pointer formal parameter described by jump function JFUNC. TARGET_TYPE is
> + the type of the parameter to which the result of JFUNC is passed. If it
> can
> + be determined, return the newly direct edge, otherwise return NULL.
> NEW_ROOT_INFO is the node info that JFUNC lattices are relative to. */
>
> static struct cgraph_edge *
> try_make_edge_direct_simple_call (struct cgraph_edge *ie,
> - struct ipa_jump_func *jfunc,
> + struct ipa_jump_func *jfunc, tree
> target_type,
> struct ipa_node_params *new_root_info)
> {
> struct cgraph_edge *cs;
> tree target;
> bool agg_contents = ie->indirect_info->agg_contents;
> - tree scalar = ipa_value_from_jfunc (new_root_info, jfunc);
> + tree scalar = ipa_value_from_jfunc (new_root_info, jfunc, target_type);
> if (agg_contents)
> {
> bool from_global_constant;
> @@ -3397,7 +3398,7 @@ update_indirect_edges_after_inlining (struct
> cgraph_edge *cs,
> {
> struct ipa_edge_args *top;
> struct cgraph_edge *ie, *next_ie, *new_direct_edge;
> - struct ipa_node_params *new_root_info;
> + struct ipa_node_params *new_root_info, *inlined_node_info;
> bool res = false;
>
> ipa_check_create_edge_args ();
> @@ -3405,6 +3406,7 @@ update_indirect_edges_after_inlining (struct
> cgraph_edge *cs,
> new_root_info = IPA_NODE_REF (cs->caller->global.inlined_to
> ? cs->caller->global.inlined_to
> : cs->caller);
> + inlined_node_info = IPA_NODE_REF (cs->callee->function_symbol ());
>
> for (ie = node->indirect_calls; ie; ie = next_ie)
> {
> @@ -3445,8 +3447,13 @@ update_indirect_edges_after_inlining (struct
> cgraph_edge *cs,
> new_direct_edge = try_make_edge_direct_virtual_call (ie, jfunc,
> ctx);
> }
> else
> - new_direct_edge = try_make_edge_direct_simple_call (ie, jfunc,
> - new_root_info);
> + {
> + tree target_type = ipa_get_type (inlined_node_info, param_index);
> + new_direct_edge = try_make_edge_direct_simple_call (ie, jfunc,
> + target_type,
> + new_root_info);
> + }
> +
> /* If speculation was removed, then we need to do nothing. */
> if (new_direct_edge && new_direct_edge != ie
> && new_direct_edge->callee == spec_target)
> diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h
> index bd8ae3e8dae..c3624052bbc 100644
> --- a/gcc/ipa-prop.h
> +++ b/gcc/ipa-prop.h
> @@ -464,7 +464,8 @@ ipa_get_param (struct ipa_node_params *info, int i)
> static inline tree
> ipa_get_type (struct ipa_node_params *info, int i)
> {
> - gcc_checking_assert (info->descriptors);
> + if (vec_safe_length (info->descriptors) <= (unsigned) i)
> + return NULL;
> tree t = (*info->descriptors)[i].decl_or_type;
> if (!t)
> return NULL;
> @@ -773,7 +774,7 @@ void ipcp_write_transformation_summaries (void);
> void ipcp_read_transformation_summaries (void);
> int ipa_get_param_decl_index (struct ipa_node_params *, tree);
> tree ipa_value_from_jfunc (struct ipa_node_params *info,
> - struct ipa_jump_func *jfunc);
> + struct ipa_jump_func *jfunc, tree type);
> unsigned int ipcp_transform_function (struct cgraph_node *node);
> ipa_polymorphic_call_context ipa_context_from_jfunc (ipa_node_params *,
> cgraph_edge *,
> diff --git a/gcc/testsuite/gcc.dg/ipa/pr82808.c
> b/gcc/testsuite/gcc.dg/ipa/pr82808.c
> new file mode 100644
> index 00000000000..9c95d0b6ed7
> --- /dev/null
> +++ b/gcc/testsuite/gcc.dg/ipa/pr82808.c
> @@ -0,0 +1,27 @@
> +/* { dg-options "-O2" } */
> +/* { dg-do run } */
> +
> +static void __attribute__((noinline))
> +foo (double *a, double x)
> +{
> + *a = x;
> +}
> +
> +static double __attribute__((noinline))
> +f_c1 (int m, double *a)
> +{
> + foo (a, m);
> + return *a;
> +}
> +
> +int
> +main (){
> + double data;
> + double ret = 0 ;
> +
> + if ((ret = f_c1 (2, &data)) != 2)
> + {
> + __builtin_abort ();
> + }
> + return 0;
> +}
> diff --git a/gcc/tree.c b/gcc/tree.c
> index 7efd644fb27..2a25c657f8b 100644
> --- a/gcc/tree.c
> +++ b/gcc/tree.c
> @@ -13893,6 +13893,52 @@ arg_size_in_bytes (const_tree type)
> return TYPE_EMPTY_P (type) ? size_zero_node : size_in_bytes (type);
> }
>
> +/* Return true if unary or binary operation specified with CODE has to have
> the
> + same result type as its first operand. */
> +
> +bool
> +tree_operation_preserves_op1_type_p (tree_code code)
expr_type_first_operand_type_p ()
> +{
> + gcc_checking_assert (TREE_CODE_CLASS (code) == tcc_unary
> + || TREE_CODE_CLASS (code) == tcc_binary);
Drop this assert, there are at least tcc_expression codes we might want
to handle in the future, the default: return false should be a good fallback.
Adjust the function comment accordingly.
Ok with those changes.
Thanks,
Richard.
> + switch (code)
> + {
> + case NEGATE_EXPR:
> + case ABS_EXPR:
> + case BIT_NOT_EXPR:
> + case PAREN_EXPR:
> + case CONJ_EXPR:
> +
> + case PLUS_EXPR:
> + case MINUS_EXPR:
> + case MULT_EXPR:
> + case TRUNC_DIV_EXPR:
> + case CEIL_DIV_EXPR:
> + case FLOOR_DIV_EXPR:
> + case ROUND_DIV_EXPR:
> + case TRUNC_MOD_EXPR:
> + case CEIL_MOD_EXPR:
> + case FLOOR_MOD_EXPR:
> + case ROUND_MOD_EXPR:
> + case RDIV_EXPR:
> + case EXACT_DIV_EXPR:
> + case MIN_EXPR:
> + case MAX_EXPR:
> + case BIT_IOR_EXPR:
> + case BIT_XOR_EXPR:
> + case BIT_AND_EXPR:
> +
> + case LSHIFT_EXPR:
> + case RSHIFT_EXPR:
> + case LROTATE_EXPR:
> + case RROTATE_EXPR:
> + return true;
> +
> + default:
> + return false;
> + }
> +}
> +
> /* List of pointer types used to declare builtins before we have seen their
> real declaration.
>
> diff --git a/gcc/tree.h b/gcc/tree.h
> index c2cabfc7529..5183f3da42a 100644
> --- a/gcc/tree.h
> +++ b/gcc/tree.h
> @@ -5457,6 +5457,7 @@ extern bool is_redundant_typedef (const_tree);
> extern bool default_is_empty_record (const_tree);
> extern HOST_WIDE_INT arg_int_size_in_bytes (const_tree);
> extern tree arg_size_in_bytes (const_tree);
> +extern bool tree_operation_preserves_op1_type_p (tree_code);
>
> extern location_t
> set_source_range (tree expr, location_t start, location_t finish);
> --
> 2.15.0
>
>
>
