Re: [tree-ssa] DCE with control dependence again (with numbers, for a change)

[law at redhat dot com]

In message <200401300028.17894.stevenb@suse.de>, Steven Bosscher writes:
 >
 >--Boundary-00=_RcZGAzlH9ShZSOP
 >Content-Type: text/plain;
 >  charset="iso-8859-1"
 >Content-Transfer-Encoding: 7bit
 >Content-Disposition: inline
 >
 >On Tuesday 20 January 2004 22:19, law@redhat.com wrote:
 >>  >> #include "bitmap.h"
 >>  >>
 >>  >> bitmap.h is already included by tree-flow.h a few lines later.
 >
 >I have removed bitmap.h from the included files.
Cool.

 >> I think you can just remove the case statement for GOTO_EXPR.
 >>
 >> The goto will still get marked later in the same routine via:
 >>
 >>   /* If the statement has volatile operands, it needs to be preserved. 
 >> Same for statements that can alter control flow in unpredictable ways.  */
 >> if (ann->has_volatile_ops
 >>
 >>       || is_ctrl_altering_stmt (stmt))
 >>
 >>     {
 >>       mark_stmt_necessary (stmt, true);
 >>       return;
 >>     }
 >>
 >
 >Hmm I don't think it will..  According to is_ctrl_altering_stmt(), a
 >goto is not a "potentially control altering statement", but...
Wow.  Amazing.  Methinks there's a project just waiting for a volunteer
to clean up that little mess in tree-cfg.c

 >
 >> I'd leave the latter hunk in for now -- while I think it could ultimately
 >> be removed, I believe doing so will require adjustments elsewhere -- and
 >> I don't really want to hold up the improved tree-ssa-dom.c on this trivial
 >> issue.
 >
 >... I've changed this hunk so that simple GOTOs are not marked, and
 >others (nonlocal, computed) are marked and added to the worklist:
 >
 >    case GOTO_EXPR:
 >      if (! simple_goto_p (stmt))
 >        mark_stmt_necessary (stmt, true);
 >      return;
 >
 >Does that look reasonable?
Quite reasonable.

It would be interesting to know if we encountered any simple gotos since
they're supposedly trimmed out by Zdenek's code.  [ Note, this is not
something you need to do to get the code integrated. ]




 >
 >>  >>   /* If we have determined that a conditional branch statement
 >>  >> contributes nothing to the program, then we not only remove it, but
 >>  >> change the flowgraph so that the block points directly to the immediate
 >>  >> post-dominator.  The flow graph will remove the blocks we are
 >>  >> circumventing, and this block will then simply fall-thru to the
 >>  >> post-dominator.  This prevents us from having to add any branch
 >>  >> instuctions to replace the conditional statement.  */
 >>  >>
 >>  >> "The flow graph will remove ..." -> "The blocks we are circumventing
 >>  >> will be removed by cleanup_cfg as they will have become unreachable."
 >>  >> or something like that.  Think of "the flow graph" as a passive noun --
 >>  >> it's a datastructure.
 >>  >
 >>  >Ah yes, the comment is a literal copy of the old RTL ssa-dce code.
 >>
 >> Then it's even possible that's my thinko.  I certainly recognized some
 >> of the other comments in the updated tree-ssa-dce.c
 >
 >It now says what you suggested:
 >
 >  /* If we have determined that a conditional branch statement contributes
 >     nothing to the program, then we not only remove it, but we also change
 >     the flow graph so that the current block will simply fall-thru to its
 >     immediate post-dominator.  The blocks we are circumventing will be
 >     removed by cleaup_cfg if this change in the flow graph makes them
 >     unreachable.  */
 >
 >
 >> It seems to me that you need to clear the statistics in tree_dce_init.  A
 >> simple
 >
 >Done.
Cool.

 >
 >
 >There are two additional changes:
 >
 >First of all, I've changed the code so that we only mark the blocks that
 >a statement is control dependent on if we have not done so before for
 >another statement in the same basic block.  This makes us a bit faster
 >(fewer calls, and perhaps better cache behavior?) on complicated flow
 >graphs like the one for interpret.ii without computed goto factoring.
 >Like so:
 >
 >      if (aggressive)
 >        {
 >          /* Mark the last statements of the basic blocks that the block
 >             containint `i' is control dependent on, but only if we haven't
 >             already done so.  */
 >          basic_block bb = bb_for_stmt (i);
 >          if (! (bb->flags & BB_VISITED))
 >            {
 >              bb->flags |= BB_VISITED;
 >              mark_control_dependent_edges_necessary (bb, el);
 >            }
 >        }
 >
 >and similar code for edges feeding PHI args.  BB_VISITED is cleared in
 >find_obviously_necessary_stmts ().
Good.


 >The second change is that the pass has its own name now: cddce.  This
 >was necessary to get the tree dumps right for the pass manager.  I
 >would have preferred to avoid this change, but there is no other way
 >to tell for the pass that this is the last time it will run on the
 >current function.
I was wondering how that worked....


 >
 >Has been through the usual testing on i686-pc-linux-gnu, of course.
Approved.

There's some minor typos in tree-ssa-dce.c.   No idea if they are new or
old, but if you could correct them when you check in the new tree-ssa-dce.c
it would be greatly appreciated.

"containint"  --> "containing"
"necessery"   --> "necessary"
"obvously"    --> "obviously"
"preven"      --> "prevent"



Thanks,
jeff