Path ping...

[Roger Sayle]

Hi Jan,

On Sun, 26 Nov 2006, Jan Hubicka wrote:
> http://gcc.gnu.org/ml/gcc-patches/2006-11/msg00741.html
>    Remove NOTE_INSN_FUNCTION_END

This is OK for mainline.

though a minor aesthetic comment...
>>+  /* While emitting the function end we could move end of the last basic block.
>>+   */
My suggestion is to wrap the one-line comment earlier in the line, rather
than  leave a "*/" on a line by itself.  i.e. "+    basic block.  */".


> http://gcc.gnu.org/ml/gcc-patches/2006-11/msg00270.html
>    Limit stack usage growth caused by inliner

This is OK for mainline.  Hopefully you'll get some feedback on
performance and compile-time impact once its in the tree.

> http://gcc.gnu.org/ml/gcc-patches/2006-07/msg01244.html
>    Add statistics to bitmaps

Hmm.  I'm not sure what I think about this.  I suspect that most
folks regularly add instrumentation code and assertions to gcc's source
to allow them to investigate a PR or the frequency of operations.
The question is really at what point does this debugging information
become useful to other contributors.  It's not uncommon for this
kind of reporting code to be left attached to a particular PR.
Is there any policy on this?  I've nothing against the patch which
looks acceptable, I just can't determine whether much of this data
would ever be useful to anyone other than you.

There's also Dirks issues, that the patch doesn't bootstrap as
submitted, and triggers an undiagnosed problem building libstdc++v3.



> http://gcc.gnu.org/ml/gcc-patches/2006-11/msg01352.html
>  Rewrite i386 string operation expansion

>> 	* i386-prostos.h (ix86_expand_movmem): Update prorotype.
Typo.  Files in config/i386/ should also begin "* config/i386/" in the
relevant ChangeLog entries.

This is OK for mainline.


> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01357.html
>  More memcpy folding

This is OK for mainline.


> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01371.html
>  Pass info about hotness of instruction to RTL expanders

Here's where I've got stuck whilst reviewing the memcpy/memset
patches previously, for which I apologise.  I've been investigating
attempts to use more profile information during RTL expansion myself.
Firstly, I think its probably preferrable to provide a new global
variable current_bb during RTL expansion, rather than just your
new maybe_hot_insn_p.  This can be set in expand_gimple_basic_block,
and cleared when it's done, so that expansion routines can check
"current_bb && maybe_hot_bb_p (current_bb)" as appropriate.  This
also avoids issues with your change to standard_80387_constant_p
which is called during many passes of the compiler, not just during
RTL expansion, at which point it may have bogus values.  This issue
may also have been addresses by Uros' recent change to use these
constants on more processors anyway.  Could you perhaps rework your
patch along the lines of a current_bb?  This can still be generalized
in the same way mentioned in your posting, and may be useful in other
RTL optimizer passes (once they've transitioned to CFG mode).  But
we should be able to get more information out of the current basic
block, than just whether its potentially hot.


> http://gcc.gnu.org/ml/gcc-patches/2006-10/msg01358.html
>  Infrastructure for passing memcpy/memset profiles to backend

If it's OK with you, I'd like to give some more opportunity for folks
to comment on this histogram infrastructure change, whilst the dust
settles on the above patches.  It'll also give me a chance to see
what x86 code we actually generate with all of your above changes.
Please ping this again in a while, and it you could include some
expected performance numbers that would be great.

Many thanks,

Roger
--