Re: [patch] tree-ssa-phiopt.c: Update a comment about the pass

[Kazu Hirata <kazu at cs dot umass dot edu>]

Hi Jeff,

> > Attached is a patch to update a comment about the pass to reflect the
> > way PHI-OPT currently operates in.
> > 
> > I've removed the following sentence
> > 
> >   bb1 will become unreachable and bb0 and bb2 will almost always be
> >   merged into a single block.
> > 
> > because I don't think this is true at least without copy propagation,
> > DCE, and CFG cleanup. 
> The statement is true -- the statement does not say when the blocks
> will be merged, just that they will be merged (by later optimizations).
> The right thing to do would have been to update the comment to reflect
> that later optimizations will eventually result in bb0 and bb2 being
> merged.

OK.  I'd put something like

  If the PHI node at the merge point of the "then" and "else" arms of
  a COND_EXPR has a single argument after these transformations, then
  copy propagation and DCE, followed by CFG cleanup, will merge bb0
  and bb2 into a single block.

> >  Note that the current PHI-OPT does not clean up
> > a degenerate PHI node of the form
> > 
> >   x = PHI <x' (bb0)>;
> > 
> > that it may create as a result of transformations.
> Why not?  That's the whole point of PHI-OPT.  If it's no longer 
> performing that optimization, then something as probably gone
> wrong.

As a part of an early TCB merge, Andrew Pinski changed the way PHI-OPT
operates in.

  http://gcc.gnu.org/ml/gcc-patches/2005-03/msg00548.html

In the new style, we are no longer restricted to optimizing away an
if-then-else construct with their arms merging at a PHI node with
exactly two arguments.  For example, we can handle

     bb0:
      if (y == 123) goto bb2; else goto bb1;
     bb1:
     bb2:
      x = PHI <0 (bb1), 1 (bb0), ...>;

and optimize it to

     bb0:
      x' = y == 123;
      goto bb2;
     bb2:
      x = PHI <x' (bb0), ...>;

where "..." represents zero or more PHI arguments.

In the old style, designed by you, we were restricted to a PHI node
with exactly two arguments, but we did optimized away the PHI node and
replaced it with an ordinary assignment.  In the above example
(without extra PHI arguments), we would have

     bb0:
      x = y == 123;
      goto bb2;
     bb2:

so it used to be trivial for merge_seq_blocks to merge these two
blocks.  Now it is not.  Even when we have two arguments, we have a
degenerate PHI node blocking a block merge opportunity.

I have not thought about what to do about this.  We have a number of
options.

  Leave the pass as is

  Special case the "two PHI argument" case

  Run a limited copy-prop inside PHI-OPT and eliminate a degenerate
  PHI node.

  Run a full-fledged copy-prop and DCE

  :

> I would rather you not commit this kind of comment change as obvious
> since it's far from obvious IMHO.

OK.

Kazu Hirata