Hi,
On Mon, Nov 11, 2013 at 10:15:24AM -0700, Aldy Hernandez wrote:
> On 11/07/13 10:58, Martin Jambor wrote:
>
> >Sorry for the delay. I'd just like to re-iterate one comment from my
> >previous email which is that I do not think tree-sra.c should export
> >any function to the outside world apart from the entry points of the
> >passes (yes, there is already build_ref_for_offset which I admit is
> >entirely my creation but that should be moved elswhere too).
> >
> >So please put sra_ipa_get_adjustment_candidate to ipa-prop.c. I see
> >that it requires get_ssa_base_param to be moved there as well but
> >since IPA-SRA uses it in different places, it would need exporting
> >too, which would be weird because it does not really do anything with
> >parameters. Since the function is so trivial, I would even suggest
> >introducing another private copy to ipa-prop.c (and leaving the
> >original without the new parameter). Alternatively, you can move the
> >function to tree.c but for that it looks too specialized.
>
> Done.
>
> Note that I didn't have to move
> ipa_sra_modify_function_body, since it wasn't used outside of
> tree-sra.c, and omp-low.c has its own version. Instead I just
> removed it from ipa-prop.h where I had inadvertently placed it.
>
Thanks. I only have two comments about ipa_get_adjustment_candidate:
> commit c7602b7b88a9e85c7e3076e38d471be5bc728089
> Author: Aldy Hernandez <al...@redhat.com>
> Date: Mon Nov 11 10:10:47 2013 -0700
>
> * ipa-prop.c (get_ssa_base_param): New.
> * ipa-prop.h (ipa_modify_expr): Rename from ipa_sra_modify_expr.
> Remove ipa_sra_modify_function_body.
> (ipa_get_adjustment_candidate): Rename from
> sra_ipa_get_adjustment_candidate.
> * omp-low.c (ipa_simd_modify_stmt_ops): Rename
> sra_ipa_get_adjustment_candidate to ipa_get_adjustment_candidate.
> * tree-sra.c (get_ssa_base_param): Remove default_def argument.
> (create_access): Remove lass argument to get_ssa_base_param.
> (disqualify_base_of_expr): Same.
> (sra_ipa_get_adjustment_candidate): Rename to
> ipa_get_adjustment_candidate and move to ipa-prop.c.
> (sra_ipa_modify_expr): Rename to ipa_modify_expr and move to
> ipa-prop.c.
> (sra_ipa_modify_assign): Rename sra_ipa_modify_expr to
> ipa_modify_expr.
> (ipa_sra_modify_function_body): Same.
>
> diff --git a/gcc/ipa-prop.c b/gcc/ipa-prop.c
> index 76e9b61..042d623 100644
> --- a/gcc/ipa-prop.c
> +++ b/gcc/ipa-prop.c
> @@ -3756,6 +3756,124 @@ ipa_modify_call_arguments (struct cgraph_edge *cs,
> gimple stmt,
> free_dominance_info (CDI_DOMINATORS);
> }
>
> +/* If the expression *EXPR should be replaced by a reduction of a parameter,
> do
> + so. ADJUSTMENTS is a pointer to a vector of adjustments. CONVERT
> + specifies whether the function should care about type incompatibility the
> + current and new expressions. If it is false, the function will leave
> + incompatibility issues to the caller. Return true iff the expression
> + was modified. */
> +
> +bool
> +ipa_modify_expr (tree *expr, bool convert,
> + ipa_parm_adjustment_vec adjustments)
> +{
> + struct ipa_parm_adjustment *cand
> + = ipa_get_adjustment_candidate (expr, &convert, adjustments, false);
> + if (!cand)
> + return false;
> +
> + tree src;
> + if (cand->by_ref)
> + src = build_simple_mem_ref (cand->new_decl);
> + else
> + src = cand->new_decl;
> +
> + if (dump_file && (dump_flags & TDF_DETAILS))
> + {
> + fprintf (dump_file, "About to replace expr ");
> + print_generic_expr (dump_file, *expr, 0);
> + fprintf (dump_file, " with ");
> + print_generic_expr (dump_file, src, 0);
> + fprintf (dump_file, "\n");
> + }
> +
> + if (convert && !useless_type_conversion_p (TREE_TYPE (*expr), cand->type))
> + {
> + tree vce = build1 (VIEW_CONVERT_EXPR, TREE_TYPE (*expr), src);
> + *expr = vce;
> + }
> + else
> + *expr = src;
> + return true;
> +}
> +
> +/* If T is an SSA_NAME, return NULL if it is not a default def or
> + return its base variable if it is. If IGNORE_DEFAULT_DEF is true,
> + the base variable is always returned, regardless if it is a default
> + def. Return T if it is not an SSA_NAME. */
> +
> +static tree
> +get_ssa_base_param (tree t, bool ignore_default_def)
> +{
> + if (TREE_CODE (t) == SSA_NAME)
> + {
> + if (ignore_default_def || SSA_NAME_IS_DEFAULT_DEF (t))
> + return SSA_NAME_VAR (t);
> + else
> + return NULL_TREE;
> + }
> + return t;
> +}
> +
> +/* Given an expression, return an adjustment entry specifying the
> + transformation to be done on EXPR. If no suitable adjustment entry
> + was found, returns NULL.
> +
> + If IGNORE_DEFAULT_DEF is set, consider SSA_NAMEs which are not a
> + default def, otherwise bail on them.
> +
> + If CONVERT is non-NULL,
This is not true, convert can be set even when the function might
return NULL afterwards.
> + this function will set *CONVERT if the
> + expression provided is a component reference that must be converted
> + upon return. ADJUSTMENTS is the adjustments vector. */
> +
> +ipa_parm_adjustment *
> +ipa_get_adjustment_candidate (tree *&expr, bool *convert,
> + ipa_parm_adjustment_vec adjustments,
> + bool ignore_default_def)
I find changing parameters passed by reference confusing and
error-prone, I would very much prefer if this was "tree **expr".
Either way, you should document that expr can be changed in the
function comment.
The patch is OK with me but please at least fix the comments.
Thanks,
Martin
> +{
> + if (TREE_CODE (*expr) == BIT_FIELD_REF
> + || TREE_CODE (*expr) == IMAGPART_EXPR
> + || TREE_CODE (*expr) == REALPART_EXPR)
> + {
> + expr = &TREE_OPERAND (*expr, 0);
> + if (convert)
> + *convert = true;
> + }
> +
> + HOST_WIDE_INT offset, size, max_size;
> + tree base = get_ref_base_and_extent (*expr, &offset, &size, &max_size);
> + if (!base || size == -1 || max_size == -1)
> + return NULL;
> +
> + if (TREE_CODE (base) == MEM_REF)
> + {
> + offset += mem_ref_offset (base).low * BITS_PER_UNIT;
> + base = TREE_OPERAND (base, 0);
> + }
> +
> + base = get_ssa_base_param (base, ignore_default_def);
> + if (!base || TREE_CODE (base) != PARM_DECL)
> + return NULL;
> +
> + struct ipa_parm_adjustment *cand = NULL;
> + unsigned int len = adjustments.length ();
> + for (unsigned i = 0; i < len; i++)
> + {
> + struct ipa_parm_adjustment *adj = &adjustments[i];
> +
> + if (adj->base == base
> + && (adj->offset == offset || adj->op == IPA_PARM_OP_REMOVE))
> + {
> + cand = adj;
> + break;
> + }
> + }
> +
> + if (!cand || cand->op == IPA_PARM_OP_COPY || cand->op ==
> IPA_PARM_OP_REMOVE)
> + return NULL;
> + return cand;
> +}
> +
> /* Return true iff BASE_INDEX is in ADJUSTMENTS more than once. */
>
> static bool
