Re: [tree-ssa] Loop header copying on trees

[law at redhat dot com]

In message <20040213202125.GA22142@atrey.karlin.mff.cuni.cz>, Zdenek Dvorak wri
tes:
 >> > *** common.opt	13 Feb 2004 00:10:27 -0000	1.14.2.19
 >> > --- common.opt	13 Feb 2004 11:27:39 -0000
 >> > *************** ftree-ccp
 >> > *** 711,716 ****
 >> > --- 711,720 ----
 >> >   Common
 >> >   Enable SSA-CCP optimization on trees
 >> >   
 >> > + ftree-ch
 >> > + Common
 >> > + Enable loop header copying on trees
 >> > + 
 >> >
 >> Include a small blurb describing why header copying is beneficial. 
 >> Also, do we really want to control it with a flag?  Some of the -f flags
 >> we have in tree-ssa should probably disappear.
 >
 >Why? IMHO having a possibility to control precisely which passes are run
 >is useful.
As your data have shown, the ability to turn on/off the tree based loop
header copying for -Os is valuable.  So let's go ahead and keep the 
-ftree-ch switch.

If you want to go ahead and turn the switch off for -Os, please consider
such a change pre-approved.

Otherwise it looks pretty good.  It's a little weaker than the RTL code
in that it can not copy exit tests using "||", but that's something we
can consider adding after you install the main patch.  Yours does copy through
other cases where the exit test has multiple basic blocks (which is good
since that was a major failing of the rtlopt branch version that I looked
at while back).

You've got a trivial spacing typo in alloc_rbi_pool.  There should only be
one space after the equal sign in the statement

rbi_pool =  create_alloc_pool (...)

In cfglayout.c you added the following line:

 >+ extern bool cfg_layout_can_duplicate_bb_p (basic_block);

Shouldn't we be getting that from a header file somewhere?  We generally
frown upon exteral declarations in .c files.  Similarly for

 > + extern basic_block cfg_layout_duplicate_bb (basic_block);

Any chance you could fix the this typo in cfglayout (I know it's not your
typo, but I just happened to notice it while reviewing your changes):

 >    /* Our algorithm depends on fact that there are now dead jumptables
 >       around the code.  */

Shouldn't "now" be "no"?

It's probably worth a trivial comment in mark_defs_for_rewrite that you
do not need to mark VDEF_OPs for rewriting as they will always have the
same underlying variable as the VDEF_RESULT, which is marked for rewriting.

Overall it looks pretty good to me.  I think once you made the edits 
Diego and I have requested, another bootstrap/test cycle and you should
install the patch (ie, I don't think it needs another review cycle unless
you end up making some unexpected major changes).

jeff