Re: [PATCH, pretty-ipa merge 1/4] Function parameter manipulation infrastructer through param notes

[Richard Guenther <richard dot guenther at gmail dot com>]

On Fri, Jun 26, 2009 at 3:19 PM, Jan Hubicka<hubicka@ucw.cz> wrote:
>> 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
> here.
>> +{
>> + ?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
> somewhere?
>> +
>> + ? ? ? /* 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?

It should be enough.  Make sure to copy them from the old call stmt
before inserting them to the insn stream (just use gimple_set_vdef,
gimple_set_vuse).

Richard.

> Honza
>