Re: [patch] MIN/MAX_EXPR transform in phiopts

[Jeffrey A Law <law at redhat dot com>]

On Fri, 2005-03-11 at 09:52 +0100, Zdenek Dvorak wrote:
> Hello,
> 
> this patch makes ivopts turn
> 
>    if (a < b)
>      a = b;
> 
> into
> 
>    a = MAX_EXPR (a, b)
> 
> and
> 
>    if (a < 0)
>      a = 0;
>    else if (a > 10)
>      a = 10;
> 
> into
> 
>    a = MAX_EXPR (0, MIN_EXPR (a, 10))
> 
> It also makes several related cleanups to phiopts:
> 
> 1) To handle nested conditionals (like in the later transforms), phiopts
>    pass the blocks using FOR_EACH_BB_REVERSE, so that the inner
>    conditions are tested and collapsed first.  This is not reliable,
>    since the order of the blocks in the list may be arbitrary.
>    The patch forces a correct ordering -- the one in that a basic
>    block with a single predecessor is guaranteed to be processed
>    before the predecessor.
Good.

> 
> 2) abs_replacement is made to use last_and_only_stmt instead of
>    nontrivially looking loop to pass over the statements of the
>    middle block.
Yea.  No idea what we were thinking with that loop.  Maybe it
pre-dates last_and_only_stmt.  Who knows.   Killing the loop
is definitely good.

> 
> 3) On a few places, build is replaced with build1/build2.
More goodness.

> 
> Bootstrapped & regtested on i686 and ppc.
> 
> Btw.: yesterday I noticed Steven mentioning in a mailing list that he has
> a similar patch; sorry for the work duplication, I had already the patch
> in testing at the moment :-(
> 
> Zdenek
> 
> 	* tree-ssa-phiopt.c (minmax_replacement, blocks_in_phiopt_order):
> 	New functions.
> 	(tree_ssa_phiopt): Use blocks_in_phiopt_order and minmax_replacement.
> 	Remove unused removed_phis variable.
> 	(conditional_replacement): Use build1/build2.
> 	(abs_replacement): Use last_and_only_stmt and build1/build2.
> 
> 	* gcc.dg/tree-ssa/phi-opt-5.c: New test.
Basically it looks good.  A minor nit:



> !       /* Walk the predecessors of x as long as they have precisely one
> ! 	 predecessor and add them to the list, so that they get stored
> ! 	 after x.  */
> !       for (y = x, np = 1;
> ! 	   EDGE_COUNT (y->preds) == 1
> ! 	   && !(EDGE_PRED (y, 0)->src->flags & BB_VISITED);
> ! 	   y = EDGE_PRED (y, 0)->src)
> ! 	np++;
> !       for (y = x, i = n - np;
> ! 	   EDGE_COUNT (y->preds) == 1
> ! 	   && !(EDGE_PRED (y, 0)->src->flags & BB_VISITED);
> ! 	   y = EDGE_PRED (y, 0)->src, i++)
This should be using the new primitives for testing if a block
as a single predecessor and extracting the single predecessor.
Right?

Consider the patch pre-approved with that simple change.

jeff