Re: [patch] tree-ssa-forwprop.c: Rewrite.

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

On Wed, 2005-04-06 at 11:20 -0400, Kazu Hirata wrote:
> Hi,
> 
> Attached is a patch to rewrite most of tree-ssa-forwprop.c.
If you wanted to get really ambitious you could reformulate this as a
real tree combiner pass.  All it's doing is combining an assignment
statement and a COND_EXPR when doing so results in a gimple COND_EXPR.
But I'm certainly not going to require you to do that -- your change
is a nice cleanup we get from having good access to immediate use
information.


> Now that immediate use information is readily available, we don't have
> to go through the trouble of using worklists.
Yup.  Which simplifies the implementation considerably IMHO.  I never
liked the code duplication between the pass which found "interesting"
statements and the pass which actually optimized the code.

I have a vague feeling that I wrote at least one other pass in a similar
manner to deal with the relatively high overhead of our old immediate
use information.  There may be another big cleanup just waiting for
someone to tackle it.



> I did not change any of the messages that the pass prints out to a
> dump file, so I didn't have to change the testsuite at all.
Good.


> Since I rewrote most of tree-ssa-forwprop.c, I used "cvs diff -C10".
> I hope you don't mind.
Not at all -- there's usually not a good way to provide diffs when a
file changes this much.  In this particular case it was easiest to bring
up the original and new code side by side to verify the logic was
carried forward correctly.

> 
> Tested on i686-pc-linux-gnu.  OK to apply?
> 
> Kazu Hirata
> 
> 2005-04-06  Kazu Hirata  <kazu@cs.umass.edu>
> 
> 	* tree-ssa-forwprop.c (vars,
> 	record_single_argument_cond_exprs,
> 	substitute_single_use_vars): Remove.
> 	(forward_propagate_into_cond_1, forward_propagate_into_cond):
> 	New.
> 	(tree_ssa_forward_propagate_single_use_vars): Call
> 	forward_propagate_into_cond for each COND_EXPR.
OK.  Though I might make a few comment suggestions.

In a couple places you inverted the sense of a conditional -- ie we
previously had 

  if (cond)
    {
      actions
    }

The new code looks like

  if (!cond)
    return;

Which is fine.  But the comments really need to be updated.   Consider
the patch pre-approved after fixing those minor nits.

>   
> !   /* If the condition is a lone variable or an equality test of
> !      an SSA_NAME against an integral constant, then we may have an
> !      optimizable case.
>   
> !      Note these conditions also ensure the COND_EXPR has no
> !      virtual operands or other side effects.  */
> !   if (cond_code != SSA_NAME
> !       && !((cond_code == EQ_EXPR || cond_code == NE_EXPR)
> ! 	   && TREE_CODE (TREE_OPERAND (cond, 0)) == SSA_NAME
> ! 	   && CONSTANT_CLASS_P (TREE_OPERAND (cond, 1))
> ! 	   && INTEGRAL_TYPE_P (TREE_TYPE (TREE_OPERAND (cond, 1)))))
> !     return NULL_TREE;
Here's a case where the comment probably needs updating.  The comment
should probably be something like

  If the condition is not a lone variable or an equality test of an
  SSA_NAME against an integral constant, then we do not have an
  optimizable case.

>   
> !   /* Now get the defining statement for TEST_VAR and see if it
> !      something we are interested in.  */
> !   def = SSA_NAME_DEF_STMT (test_var);
> !   if (TREE_CODE (def) != MODIFY_EXPR)
> !     return NULL_TREE;
Similarly


jeff