This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: IPA-SRA misc trivia observations


Hi,

thanks for the meticulous review.


On Wed, Jun 24, 2009 at 04:02:43PM +0200, Bernhard Reutner-Fischer wrote:
> Hi,
> 
> 0) Please run CSiBE so we can see if or how much IPA-SRA regresses for -Os
> 

Will do. 

> 1)
> [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|

OK

> 
> 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
it.

> 
> 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.  */
> just
> /* 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|

Yeah.

> 
> ipa_dump_param_notes(): Yet another odd final return in a void function.

Removed.

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. 

Thanks,

Martin


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]