Re: Improvement to loop optimization in presence of inlining

[Mark Mitchell <mark at markmitchell dot com>]

>>>>> "Jeffrey" == Jeffrey A Law <law@hurl.cygnus.com> writes:

    >>  OK, here's an improved version.  This version should not
    >> through debugging information into total disarray.  Jeff, OK?
    >> 
    >> Sun Jun 28 18:25:20 1998 Mark Mitchell <mark@markmitchell.com>
    >> 
    >> * jump.c (duplicate_loop_exit_test): Don't refuse to copy a
    >> section of code just because it contains
    >> NOTE_INSN_BLOCK_{BEG,END}.  * stmt.c (expand_end_loop):
    >> Likewise.  Also, don't refuse to move CALL_INSNs or
    >> CODE_LABELs.  When moving code, don't move
    >> NOTE_INSN_BLOCK_{BEG,END}.

    Jeffrey> These comments apply to the changes in both files.

    Jeffrey> So this leaves the BLOCK_BEG/BLOCK_END notes in-place
    Jeffrey> since they're not copied later in
    Jeffrey> duplicate_loop_exit_test.  Doesn't this scrogg debugging
    Jeffrey> too?  If so, we might want to only allow them in the

Right.  My original patch, though, would have scrogged it for the
whole file; this version only scrogs debugging the exit test for the
loop in question.

    Jeffrey> stream if optimize > 1.

Sure; I'm happy to make that change.

    Jeffrey> The other question is could the loop rotation create a
    Jeffrey> situation where the compiler would later delete those
    Jeffrey> notes?  And if so, is that safe considering the
    Jeffrey> relationship that has to be kept between the rtl and tree
    Jeffrey> structures for block scope info?

The compiler must never, ever delete NOTE_INSN_BLOCK_{BEG,END} notes,
and I don't think it does at present.  (Actually, I think this is a
misfeature; generated assembly tends to look like this in the presence
of lots of inlines:

  .LBB1
  .LBB2
  .LBB3
  .LBE3
  .LBE2
  .LBE1

i.e., lots of labels with nothing in them, and then debugging
information about the variables used in those blocks, even though
no-one will ever actually be in the block!  This wastes space in the
objects, and makes the assembly harder to read.  I've been planning to
fix this, but later; it's just annoying, not wrong.)

    Jeffrey> You may also need to avoid some of this code
    Jeffrey> restructuring in the presense of EH notes.  I believe
    Jeffrey> it's a bug that this code doesn't respect EH notes at the
    Jeffrey> current time.  In fact, I think there was a bug report
    Jeffrey> recently about some of the code in jump.c not honoring EH
    Jeffrey> notes, but I haven't looked at it in any detail yet.

OK, there are two issues here:

  o Was the code already broken?
  o Does my patch break it further?

Also, we should look at both changes independently.  First, let's
consider expand_end_loop:

Here's the loop before the transformation:

         start_label:
         if (test) goto end_label;
	 body;
	 goto start_label;
	 end_label;

Here's afterwards:

         goto start_label;
         newstart_label:
	 body;
	 start_label:
	 if (test) goto end_label;
	 goto newstart_label;
	 end_label;

If `test' contains partial EH-regions, there's a problem.  My patch
doesn't check for this, but neither did the earlier code.  
I agree that we should check this.  In other words, if we have:

 while ( ( { try {
               if (cond ()) 0;	
	       else {
	         bar();
	         1;
	       }
            } catch (...) { 
              1;
            } )) {
   body;
 }

This isn't legal C++, but here's what it's supposed to mean: if cond()
is true, stop looping.  Otherwise, call bar, and keep looping.  In
addition, if cond throws an exception, catch it and keep looping. Such
constructs are certainy legal in LISP.

We should not move the test since then the EH-region for the try-block
would be broken up.  (In this case we would the EH_BEG note for the
`try' and `if cond()' but not the call to bar() or the EH_END note.)

So, let's suppose test contains only entire EH-regions, but that it is
itself nested in some region.  Then, the EH-region must start outside
the loop (since `test' includes everything from the start of the loop.
Therefore, in any sane language :-), the end of the exception region
must also be outside the loop, i.e., you can't write:

  try {
    while (1) {
      catch (...) {
      } 
    }
  }

in any lanugage that I know.  So, there's no problem in that case.

Now, on to jump.c.  I think there should be a 1:1 correspondence
between EH_REGIONs and catch handlers, so no-one should ever duplicate
an EH_REGION.  Of course, jump.c doesn't do this; it doesn't duplicate
any notes.  It also doesn't move any CALL_INSNs, which are the only
things that can throw exceptions, though, so there's no danger.

In summary, I would:

  o Adjust my change to stmt.c so as to not copy partial EH-regions in
    stmt.c.
  o Only move BLOCK_BEG/BLOCK_END notes if optimize > 1 so as not
    to screw up debugging with -O1.

May I check in the patch with those changes?

-- 
Mark Mitchell 			mark@markmitchell.com
Mark Mitchell Consulting	http://www.markmitchell.com