Re: more hot-cold partitioning problems

[Richard Henderson <rth at redhat dot com>]

On Tue, Apr 13, 2004 at 10:31:41PM -0700, Caroline Tice wrote:
> On April 6, Geoff Keating approved the patch.  Bearing in mind what
> Zack had said, **I waited three days to commit the patch**, in case
> somebody wanted to say "Wait a minute!  What about blah?".

Unfortunately I missed Geoff's message.

> What I did *not* do (and in retrospect I can see that I should have) 
> was to run the SPEC tests with "-g".  I tend not to do that as it
> slows down performance, which is what SPEC is usually about.

This had better not be true.  If so, you've found a bug.  Using -g
should not change generated code in ANY way for ANY reason.  Why?
Because it means that you can no longer debug optimized code and be
sure that you're actually debugging the optimized code.

> > (1) The biggest problem is that the end of the function is not
> >     in the same section as the start of the function.  Which
> >     means that the symbolic arithmetic associated with the .size
> >     directive crosses sections, which is a hard assembler error.
> 
> I repeat, this seems to only be a problem if you compile with "-g".  

This is false.  I myself ran into this problem attempting to 
profilebootstrap gcc with your option.  Without -g.

> >	(a) We need to switch back to the section that was in
> >	    effect at the start of the function at the end.
> >	    This is easily done at the end of final().
> >
> 
> I had already done this for stabs (and then forgot that I needed to do 
> it for other debugging formats).  I will certainly take care of this.

Changing sections at the end of the function shouldn't be handled 
in debug info.  It should be handled generically.

> >	(d) For functions which have been assigned to linkonce
> >	    sections, I'm not sure this optimization makes sense
> >	    as-is.  Certainly we could apply this, but we'd want
> >	    to not place all of the cold paths into .text.unlikely,
> >	    but rather into a linkonce section directly associated
> >	    with the one that the main body of the function resides.
> 
> I have to confess ignorance here.  I know nothing about linkonce
> sections.  I could use some more detailed guidance here.

To begin with, I think you should handle this as with DECL_SECTION_NAME,
and simply disable your optimization.  Of course, that will disable your
optimization for a good deal of c++ code, so we should revisit the problem
at some point.

> My approach is not to disable the optimization entirely in the absence
> of named sections, but to disable anything to do with actually writing
> separate sections.  This will leave active the small bit that causes
> reorder_basic_blocks to put all the hot blocks together before all the
> cold blocks.

Hum.  Seems to me that sort of optimization should be active anytime
-freorder-blocks is active.

> Again, you are wrong, I tested it thoroughly on the two architectures I 
> have access to, and I have never encountered an ICE with it.

Huh?  But the two targets that you have hardware for don't use it.
Or did you disable support for long branches temporarily?  If so,
I apologize.  But I'm confused as to why any target would accept a
VOIDmode LABEL_REF...

> I object here!!  I DID NOT define those values;  they were already 
> defined in the system without the dots long before my patch was created!

> > (6) Repeat after me: "Instruction stream searching is wrong, m'kay?"
> 
> Could you explain why?

It's slow.  And it's doubly unnecessary here because you have
all the information needed at the point you insert the notes
in the first place.

> >     The NOTE_INSN_UNLIKELY_EXECUTED_CODE note (or a suitably renamed
> >     equivalent) should contain all of the information you need to
> >     switch between sections.  You should not have to search forward
> >     from NOTE_INSN_BASIC_BLOCK to discover if a subsequent note
> >     exists.
> >
> 
> The only way I could do this would be to make the
> NOTE_INSN_UNLIKELY_EXECUTED_CODE note be the very first thing in any
> basic block to which it belongs.

No, the other way to solve the problem is to put the note between
basic blocks.  Make it not part of a block at all.  There are lots
of notes that work this way.

> >     Plus, as I noted in my previous review, triggering a section
> >     change on NOTE_INSN_BASIC_BLOCK means that the label that came
> >     before the NOTE_INSN_BASIC_BLOCK note would have been emitted
> >     to the wrong section.
> >
> 
> No, this does not happen.  Have you actually looked at the
> assembly code generated and verified that this is happening for you?

It appears not to be happening.  And I theorized as to why 
in the section below.

> If you examine the patch I committed, the code in final_scan_insn,
> where dealing with a label instruction *also* scans ahead for the
> NOTE_INSN_UNLIKELY_EXECUTED_CODE
> note, and makes sure to change to the correct section before outputting 
> the label.
> (lines 1918-1931 in final.c).

Ah, I see it now.

> With regards to the clustering of hot/cold blocks,  I have found (on
> examining the assembly files from the SPEC tests) that
> *most of the time* all the hot blocks come before all the cold blocks.
> But this is not true all the time.

When not?  And can we change this?



r~