Re: [tuples][patch] Convert pass_ccp and pass_store_ccp to tuples

[Diego Novillo <dnovillo at google dot com>]

On 3/3/08 11:50 PM, Bill Maddox wrote:

> Testing revealed a handful of failures that appear to be pass-order
> dependencies, or, in one case, a pre-existing issue with mainline.
> There is a net increase of 79 passing tests on the C tests and no
> regressions elsewhere.

Is this the -fno-tree-dce problem we chatted about earlier?  That bug is 
also present in mainline, could you file a bugzilla PR for it?

> +/* FIXME tuples.  This function appears to be called only for
> +   assignments, calls, conditionalss, and switches, due to the

s/conditionalss/conditionals/

> +   logic in visit_stmt, carried over from the pre-tuples code.
> +   The function purports to handle return statements, but they
> +   should never bee seen, and produce no propagatable value.  */

s/bee/be/

> +  /* FIXME tuples.  Why is this case handled at all?  */
> +  if (gimple_code (stmt) == GIMPLE_RETURN
> +      && gimple_return_retval (stmt) != NULL_TREE
> +      && is_gimple_min_invariant (gimple_return_retval (stmt)))
>      return CONSTANT;

Let's remove this code and assert that likely_value is not being called 
with anything other than assignments, calls and conditionals.

> @@ -888,134 +906,192 @@ ccp_visit_phi_node (tree phi)
>     operands are constants.
>  
>     If simplification is possible, return the simplified RHS,
> -   otherwise return the original RHS.  */
> +   otherwise return the original LHS or NULL_TREE.  */

You mean RHS here?

> +/* FIXME tuples.  It would be preferable to consistently
> +   return either NULL_TREE or the original RHS.  Currently,
> +   we attempt to follow the pre-tuples behavior.  */

Just returning NULL when folding isn't possible is fine.  Just make sure 
the callers handle it.

> +              /* Substitute operands with their values and try to fold.  */
> +              replace_uses_in (stmt, NULL, const_val);
> +              /* FIXME tuples.  */
> +#if 0
> +              retval = fold_call_expr (rhs, false);
> +#else
> +              retval = NULL_TREE;

This should call fold_gimple_call, but that is a destructive folder, so 
we need to figure out something different here.  For now, let's just 
remove this handler, since as you note below, it is never invoked.

> +      /* FIXME tuples.  There is no original RHS to return here.  */
> +      return NULL_TREE;

That's fine.  Returning NULL_TREE is OK, as long as we fix all the 
callers to handle this.  No need to have a FIXME note.

> +  /* FIXME tuples.  This is the only place that we call ccp_fold.
> +     Since likely_value never returns CONSTANT for function calls,
> +     the attempt to handle them in ccp_fold is inoperative.  */
>    if (likelyvalue == CONSTANT)
>      simplified = ccp_fold (stmt);

Ah, yes.  Let's do this: in ccp_fold assert that we are not given a 
GIMPLE_CALL.  There is a note in likely_value saying that for some call 
sites we may want to return CONSTANT.  When that spot is fixed, the 
assert in ccp_fold() will trigger.


> +        /* FIXME tuples.  It appears that we should be able to optimize
> +           computed GOTOs here as well, but the the original pre-tuples
> +           code did not do so, so we defer.  */

No 'FIXME tuples' here, please.  If it's an existing limitation, use 
'FIXME', but we are using 'FIXME tuples' to mark spots that need to be 
fixed before merging into mainline.

> +/* FIXME tuples.  Some builtins expand into inline code that may
> +   not be valid in GIMPLE.  Callers must take care.  */

This was true before tuples, so no 'FIXME tuples' here.

> @@ -512,6 +512,9 @@ ssa_prop_init (void)
>  
>        for (si = gsi_start_bb (bb); !gsi_end_p (si); gsi_next (&si))
>  	gimple_set_plf (gsi_stmt (si), STMT_IN_SSA_EDGE_WORKLIST, false);
> +    
> +      for (si = gsi_start (phi_nodes (bb)); !gsi_end_p (si); gsi_next (&si))
> +	gimple_set_plf (gsi_stmt (si), STMT_IN_SSA_EDGE_WORKLIST, false);

This is not needed.  PHI nodes are always simulated. 
STMT_IN_SSA_EDGE_WORKLIST should not affect PHI nodes.  If this fixed 
something, it probably papered over the real bug.

>  
>    rhs = gimple_assign_rhs1 (stmt);
>  
> +  STRIP_NOPS (rhs);
> +

Not needed.  An operand never has NOPs.

> +tree
> +get_lhs (gimple stmt)
> +{
> +  enum tree_code code = gimple_code (stmt);
> +
> +  if (code == GIMPLE_ASSIGN)
> +    return gimple_assign_lhs (stmt);
> +  else if (code == GIMPLE_CALL)
> +    return gimple_call_lhs (stmt);
> +  else
> +    gcc_unreachable();
> +}
> +
> +
> +/* Set the LHS of a statement that performs an assignment,
> +   either a GIMPLE_ASSIGN or a GIMPLE_CALL.  */
> +
> +void
> +set_lhs (gimple stmt, tree lhs)
> +{
> +  enum tree_code code = gimple_code (stmt);
> +
> +  if (code == GIMPLE_ASSIGN)
> +    return gimple_assign_set_lhs (stmt, lhs);
> +  else if (code == GIMPLE_CALL)
> +    return gimple_call_set_lhs (stmt, lhs);
> +  else
> +    gcc_unreachable();
> +}
> +

These two may be needed in other optimizers.  Move them to gimple.c and 
rename gimple_get_lhs(), gimple_set_lhs().  Also, I think we probably 
don't want to assert for things different than GIMPLE_ASSIGN and 
GIMPLE_CALL.

> +/* FIXME tuples.  The charter of this function is unclear.  It was
> +   introduced to replace occurrences of unshare_expr in cases where
> +   a statement is copied temporarily in order to present a "before
> +   and after" diagnostic, e.g., showing folding in substitute_and_fold.
> +   In that case, uses occuring in the saved statement were linked from
> +   definitions elsewhere, confusing code that expected no such uses to
> +   exist.  It might be preferable to rewrite such diagnostics to simply
> +   dump the "before" diagnostic to a string, rather than retaining a
> +   statement for later processing.  */

Agreed.  This can be done in a later patch.


Thanks.  Diego.