This is the mail archive of the
mailing list for the GCC project.
Re: [PATCH, pretty-ipa merge 1/4] Function parameter manipulation infrastructer through param notes
> Index: mine/gcc/ipa-prop.h
> --- mine.orig/gcc/ipa-prop.h
> +++ mine/gcc/ipa-prop.h
> @@ -389,4 +389,71 @@ void ipa_print_all_params (FILE *);
> void ipa_print_node_jump_functions (FILE *f, struct cgraph_node *node);
> void ipa_print_all_jump_functions (FILE * f);
> +/* Structure to describe transformations of formal parameters and actual
> + arguments. Each instance describes one new parameter and they are meant to
> + be stored in a vector. Additionally, most users will probably want to store
> + notes about parameters that are being removed altogether so that SSA names
> + belonging to them can be replaced by SSA names of an artificial
> + variable. */
> +struct ipa_parm_note
I am not sure if ipa_parm_adjustment would not be better name, it is not
very informative to have "note". But I don't have any strong opinions
> + tree base; /* The original PARM_DECL itself, helpful for
> + processing of the body of the function
> + itself. Intended for traversing function
> + bodies. ipa_modify_formal_parameters,
> + ipa_modify_call_arguments and
> + ipa_combine_notes ignore this and use
> + base_index. ipa_modify_formal_parameters
> + actually sets this. */
I think GNU formating wants you to put the comment before field, see
function.h for similar structure.
> +typedef struct ipa_parm_note ipa_parm_note_t;
> +DEF_VEC_O (ipa_parm_note_t);
> +DEF_VEC_ALLOC_O (ipa_parm_note_t, heap);
> +VEC(tree, heap) *ipa_get_vector_of_formal_parms (tree fndecl);
> +void ipa_modify_formal_parameters (tree fndecl, VEC (ipa_parm_note_t, heap) *,
Since we are usually working on the vector of notes, perhaps giving it
named type would help. That VEC (...,..) syntax is quite ugly.
> + else if (!note->remove_param)
When removing first parameter of method (i.e. this argument), one needs
to turn method to function (same way as skip_args does). Is that done
> + /* FIXME Either make dump_struct public and move it to a more
> + appropriate place or remove the following dump. */
What is so wrong about dump here? If it is, what about actualy realing
the FIXME by removing it?
> + if (!useless_type_conversion_p (type, TREE_TYPE (expr)))
> + expr = fold_convert (type, expr);
Are you sure we always pass here fold_convertible_p? If not, one would
need to care about VCE generation here.
> + /* Carry all the flags to the new GIMPLE_CALL. */
> + gimple_call_set_chain (new_stmt, gimple_call_chain (stmt));
> + gimple_call_set_tail (new_stmt, gimple_call_tail_p (stmt));
> + gimple_call_set_cannot_inline (new_stmt,
> + gimple_call_cannot_inline_p (stmt));
Hmm, perhaps we should ice on trying to modify CANNOT_INLINE call. THey are
no (ab)used for other means too, but in general this means that
parameters of the function are so confused that no one knows what they
really should do.
This will add some extra work into local IPA_SRA and IPA-CP to validate
that all callers are !CANNOT_INLINE.
> + gimple_call_set_from_thunk (new_stmt, gimple_call_from_thunk_p (stmt));
Hmm, set_from_thunk makes calls.c to pass stuff via invisible refrences.
I am not quite sure what happens when clonning thunked function and
redirecting the thunk, but I would guess, we could just ICE here if we
do so, it don't seem to work well.
> + update_ssa (TODO_update_ssa);
When do we need update of SSA? Isn't it enough to just move the VOPs?