Re: [tree-ssa] CCP fixes

[Daniel Berlin <dberlin at dberlin dot org>]

On Tuesday, February 18, 2003, at 03:05  PM, law@redhat.com wrote:

> This patch contains three fixes.  One is an obvious missing abort.  
> Fun.
>
> The second is more interesting.  CCP was creating non-gimple trees.
>
> While I haven't tried to construct a testcase where the compiler would
> fault, it seems *extremely* bad to me to allow CCP to change a tree
> from gimple form into non-gimple form.
It is, but i'm a bit confused out it could end up doing this.
Or is it allowing *anything* to be considered a constant as long as 
it's TREE_CONSTANT?

> Unfortunately, fixing this causes "regressions" in the testsuite.  
> Namely
> in some of the tests which verify that we optimize certain cases of
> builtin string and stdio functions fail.
>
> They fail because gimple doesn't allow arbitrary complex expressions
> to appear as call arguments.  You can have simple constants and 
> variables.
> Something like PLUS_EXPR ... is not allowed.
This is good, not bad.
If you allow arbitrary expressions there, anything that looks into 
calls now has to be able to handle *everything*, rather than just 
simple constants and variables.
This would be a serious pain in the ass and end up making things 
special case calls (think of visiting each expression recursively.  
Either your function has to handle all cases all the time, which is a 
waste, since they don't occur anywhere but calls, or you have to call a 
seperate recursive helper just for calls that knows how to handle 
everything).

> We're really going to need to sit down and fix the interactions between
> gimplification and builtin expansion at some point in the near future.
As long as you don't make calls allow arbitrarily complex expressions, 
sure.

> The last change fixes a bug caught by the recently added sanity
> checking.  Namely we had a VARYING->CONSTANT transition.  Naughty 
> Naughty.
> Basically when given a simple copy operation, we should copy the 
> lattice
> values as well.
>
> 	* tree-ssa-ccp.c (visit_assignment): For simple copies, copy the
> 	lattice values.
> 	(defs_to_undefined): Add missing abort.
> 	(replace_uses_in): Do not do a replacement if it would create
> 	non GIMPLE trees.
>
> Index: tree-ssa-ccp.c
> ===================================================================
> RCS file: /cvs/gcc/gcc/gcc/Attic/tree-ssa-ccp.c,v
> retrieving revision 1.1.2.52
> diff -c -3 -p -r1.1.2.52 tree-ssa-ccp.c
> *** tree-ssa-ccp.c	15 Feb 2003 00:00:33 -0000	1.1.2.52
> --- tree-ssa-ccp.c	18 Feb 2003 19:56:55 -0000
> *************** visit_assignment (stmt)
> *** 519,527 ****
>       abort ();
>   #endif
>
> !   /* Evaluate the statement.  */
> !   val = evaluate_stmt (stmt);
> !
>     /* FIXME: Hack.  If this was a definition of a bitfield, we need 
> to widen
>        the constant value into the type of the destination variable.  
> This
>        should not be necessary if GCC represented bitfields properly.  
> */
> --- 519,539 ----
>       abort ();
>   #endif
>
> !   if (TREE_CODE (TREE_OPERAND (stmt, 0)) == SSA_NAME
> !       && TREE_CODE (TREE_OPERAND (stmt, 1)) == SSA_NAME)
> !     {
> !       /* For a simple copy operation, we copy the lattice
> !          values.  */
> !       value *nval = get_value (TREE_OPERAND (stmt, 1));
> !
> !       val.lattice_val = nval->lattice_val;
> !       val.const_val = nval->const_val;
> !     }
> !   else
> !     {
> !       /* Evaluate the statement.  */
> !       val = evaluate_stmt (stmt);
> !     }
>     /* FIXME: Hack.  If this was a definition of a bitfield, we need 
> to widen
>        the constant value into the type of the destination variable.  
> This
>        should not be necessary if GCC represented bitfields properly.  
> */
> *************** def_to_undefined (var)
> *** 842,847 ****
> --- 854,860 ----
>        except for values which are initialized to VARYING.  */
>     if (value->lattice_val == VARYING
>         && get_default_value (var).lattice_val != VARYING)
> +     abort ();
>   #endif
>
>     if (value->lattice_val != UNDEFINED)
> *************** replace_uses_in (stmt)
> *** 952,958 ****
>         tree *use = VARRAY_GENERIC_PTR (uses, i);
>         value *val = get_value (*use);
>
> !       if (val->lattice_val == CONSTANT)
>   	{
>   	  *use = val->const_val;
>   	  replaced = true;
> --- 965,971 ----
>         tree *use = VARRAY_GENERIC_PTR (uses, i);
>         value *val = get_value (*use);
>
> !       if (val->lattice_val == CONSTANT && is_simple_val 
> (val->const_val))
>   	{
>   	  *use = val->const_val;
>   	  replaced = true;