This is the mail archive of the
mailing list for the GCC project.
Re: IPA-SRA misc trivia observations
- From: Martin Jambor <mjambor at suse dot cz>
- To: Bernhard Reutner-Fischer <rep dot dot dot nop at gmail dot com>
- Cc: gcc-patches at gcc dot gnu dot org, Jan Hubicka <hubicka at ucw dot cz>
- Date: Wed, 24 Jun 2009 18:04:02 +0200
- Subject: Re: IPA-SRA misc trivia observations
- References: <20090624140243.GE22116@mx.loc>
thanks for the meticulous review.
On Wed, Jun 24, 2009 at 04:02:43PM +0200, Bernhard Reutner-Fischer wrote:
> 0) Please run CSiBE so we can see if or how much IPA-SRA regresses for -Os
> [PATCH, pretty-ipa merge 1/4] Function parameter manipulation infrastructer through param notes
> s|/* Structure do describe|/* Structure to describe|
> s|functions modyfying|functions modifying|
> If ipa_parm_note.base_index is zero-based, why not make it unsigned int?
> Same for the retval of count_formal_params_1() etc.
I don't think it's necessary, hopefully there's never going to be that
many parameters in a function. And we may find uses for the negative
values in the future.
> docs for count_formal_params_1():
> /* Return number of formal parameters of FNDECL. */
> sounds a little bit like type_num_arguments() but is of course a different thing.
> It seems to be used quite often in the tree, perhaps
> just add a FOR_EACH_DECL_ARGUMENT macro and use that everywhere?
It's called exactly twice so not that many times. Some iterations are
done by simply walking the list of parameters in the function
declaration. I don't like iteration macros just for the sake of them
and I can't see much value using one here.
> dump_aggregate_1(): 2 surplus empty lines
I can't see two empty lines in a row anywhere near that function.
> Perhaps make dump_aggregate_1 and dump_aggregate's "indent" param unsigned?
All sorts of other functions like print_node use int and so do I.
There's no compelling reasons to use anything else either.
> get_vector_of_formal_parm_types(): Reimplements
> type_num_arguments(). Just int count = type_num_arguments (fntype) ?
Unfortunately the function discards the trailing void type. But I
need that to built the new type. So I can't use it without changing
> ipa_modify_formal_parameters(): Sounds odd to have a final return in that void fn.
I don't find anything odd with that but have no strong opinions about
them either and so I can certainly remove such returns.
> ipa_modify_call_arguments(): Why don't you just VEC_iterate over the notes?
> I didn look close, but the last part of the function looks similar to
> gimple_call_copy_skip_args(), no?
I don't particularly like VEC_iterate but I guess I could use it here.
> /* Return true iff BASE_INDEX is in NOTES twice or more times. */
> /* Return true iff BASE_INDEX is in NOTES more than once. */
Right, that's better.
> ipa_combine_notes(): VEC_iterate ?
I need the lengths of the vectors also for other things than the
controlling the loop so I'll keep it as it is. There is enough
VEC_confusions as it is.
> A bitmap would make that one look prettier i suppose ;)
Instead of index_in_notes_multiple_times_p? I assume there's going to
be only a few parameters per function and so this is in fact cheaper
than setting up a bitmap.
> s|assuming they are menat to be|assuming they are ment to be|
> ipa_dump_param_notes(): Yet another odd final return in a void function.
I'll re-post the patch as a reply to the original one so that
everything is threaded and at one place. I hope to test the changes
overnight, so I'll probably send the new version tomorrow.