Re: [CFG] Loop represenation & new loop optimizer works

[Jan Hubicka <jh at suse dot cz>]

> Hello.
> 
> > > This patch implements new loop represenation. It also makes some changes
> > > to support moving to new loop optimizer.
> > 
> > I guess it is better to place it in cfgloopanal.c I made for such a simple
> > loop code to separate fucntion contstruct_preheaders.
> > 
> > > + /* Used by dfs_enumerate_from; keep this one zero!  */
> > > + #define BB_VISITED		8
> > 
> > We generally don't polute flags by pass dependent data.  Can't you
> > use the AUX field to store the flag as it is done in cfgcleanup/ifcvt?
> 
> This is not pass dependent; dfs_enumerate_from is basis for get_loop_body and
> is used on few other functions that are used in situations aux field is already
> reserved for some pass specific data (and in more than one pass, so I can't add
> my flag to them).

OK then.
> > Why exactly this is needed? The basic block structure is zeroed before
> > used, so they should be 0 by default.
> 
> Then something doesn't work, as it happened to me that these fields had
> non-NULL values just after find_basic_blocks and it made problems in
> cfgcleanup.

Hmm, in case the life info has been already built, the values are allocated.
The is the case after GCSE, but it should not be problem, as the code will
just maitain dead copies around.  Where exactly did it crashed?
> > Can't you use the BB_NEW flags I created recently? They should give you
> > info about newly created basic block you need to update loop structure for.
> > It needs search trought all basic blocks instead of only those changed to
> > find the falgs having potential time complexity problem, but searching
> > single flag in BB is cheap and I believe it is cleaner and easier
> > to understand interface.
> 
> I thought it too slow to have to pass through all bbs after emiting few
> on some edge; but this isn't critical, so I can rework it this way if
> you want to.

Please.  We do the pass trought all BBS anyway in the commit_edge_insertions.
How often do you need the single commit?
Perhaps it can be done by exporting split_edge...
> > Isn't exactlyt he same code needed at different places too? 
> > I guess it should be possible to update datastructure after general BB
> > is removed and then I think we should have function doing that.
> 
> I don't need exactly this anywhere just now; and I don't think I will
> (updating dominators after removing some bbs/edges is generally hard and
> differs from place to place a lot. I could make some generic code for this,
> but in simple cases like this it probably isn't useful).
OK.
> 
> > Also perhaps we can get the loop infromation from BB structure that contains
> > pointer to loop it is contained in instead of passing it everwhere.
> > That would make possible to teach delete_block do the trick w/o altering
> > the current interface.  The loop structure can be seen as property of CFG
> > graph, as we see CFG as the property of insn chain.
> 
> I pass loops here because of loops->cfg.dom field (which isn't used now;
> but it is probable that I will need to pass something like this once
> Pavel is done with reworking structure for dominators).
OK.
> 
> > > !   if (e)
> > > !    {
> > > !      if (e->flags & EDGE_FALLTHRU)
> > > !        redirect_edge_succ (e, new_bb);
> > > !      else
> > > !        redirect_edge_and_branch (e, new_bb);
> > > !    }
> > Perhaps you should put the other conditional updates of frequency to this
> > condtiional to avoid ?: construct.
> 
> I don't understand?

There are some e ? EDGE_FREQUENCY (e) : 0  expressions earlier in the
function that can be avoided if the whole frequency updating code is
just conditionalized by if (e).

Honza
> 
> Zdenek