Pointer Plus Patch

Diego Novillo dnovillo@google.com
Wed Jun 13 20:25:00 GMT 2007


On 6/13/07 4:47 AM, Andrew Pinski wrote:

> What should I do about the ChangeLog, should I keep ChangeLog.ptr or
> write a new one and when I commit, commit it with that one?

I would write a new one.  The branch really did only one (big) change,
so it will be better if we had a single large CL entry.

> Should I just go ahead and merge it tomorrow or wait for review of the
> patch?

Perhaps wait a day or two?  The DF merge is still showing some issues

>         || POINTER_TYPE_P (TREE_TYPE (op0))
>         || POINTER_TYPE_P (TREE_TYPE (op1)))
>       {
> !       if (code == MIN_EXPR || code == MAX_EXPR)
>   	{
> ! 	  /* For MIN/MAX expressions with pointers, we only care about
> ! 	     nullness, if both are non null, then the result is nonnull.
> ! 	     If both are null, then the result is null. Other wise they
> ! 	     are varrying.  */

s/non null/non-NULL/  (actually, I'm not sure what the convention is
here, we seem to use a variety of forms).
s/Other wise/Otherwise/
s/varrying/varying/

>   
> + @itemx POINTER_PLUS_EXPR
> + This node represents pointer arithmetic.  The first operand is always
> + a pointer/reference type.  The second operand is always an unsigned
> + integer type compatiable with sizetype.  This is the only binary

s/compatiable/compatible/

> + arthemetic operand that can operate on pointer types.

s/arthemetic/arithmetic/

> !   
> !   type1 = type;
> !   if (POINTER_TYPE_P (type))
> !     type1 = sizetype;
>     /* Now the hard part; we must formulate the assumption(s) as expressions, and

Vertical spacing.

> *************** number_of_iterations_le (tree type, affi
> *** 1062,1067 ****
> --- 1069,1077 ----
>   			 bounds *bnds)
>   {
>     tree assumption;
> +   tree type1 = type;
> +   if (POINTER_TYPE_P (type))
> +     type1 = sizetype;

It worries me a little bit that we are straight out using sizetype here.
Shouldn't we at least assert that type1 and type are compatible?  This
happens regularly in this file.  This needs at least a comment
clarifying why it's safe to do this in the case of pointers.

> Index: java/ChangeLog.ptr
> ===================================================================
> *** java/ChangeLog.ptr	(.../trunk/gcc)	(revision 0)
> --- java/ChangeLog.ptr	(.../branches/pointer_plus/gcc)	(revision 125672)

Move to java/ChangeLog

> --- tree-scalar-evolution.c	(.../branches/pointer_plus/gcc)	(revision 125672)
> *************** add_to_evolution_1 (unsigned loop_nb, tr
> *** 665,672 ****
>   	    }
>   
>   	  to_add = chrec_convert (type, to_add, at_stmt);
> ! 	  right = chrec_convert (type, right, at_stmt);
> ! 	  right = chrec_fold_plus (type, right, to_add);
>   	  return build_polynomial_chrec (var, left, right);
>   	}
>         else
> --- 665,672 ----
>   	    }
>   
>   	  to_add = chrec_convert (type, to_add, at_stmt);
> ! 	  right = chrec_convert_rhs (type, right, at_stmt);
> ! 	  right = chrec_fold_plus (chrec_type (right), right, to_add);

Hmm, why change 'type' to 'chrec_type (right)' on the second call to
chrec_fold_plus?  An unrelated bug fix or did it pop up because of ptr_plus?

>   	return chrec_dont_know;
>   
>         left = chrec_before;
> !       right = chrec_convert_rhs (chrec_type (left), to_add, at_stmt);

Likewise.

> --- 2048,2059 ----
>         if (CHREC_LEFT (chrec) != op0
>   	  || CHREC_RIGHT (chrec) != op1)
>   	{
> ! 	  op1 = chrec_convert_rhs (TREE_TYPE (op0), op1, NULL_TREE);

Why the change of chrec_type to TREE_TYPE?  Are we guaranteed not to be
dealing with a compiler generated chrec?


> ! 	case POINTER_PLUS_EXPR:
>   	  /* If sum of pointer + int, restrict our maximum alignment to that
>   	     imposed by the integer.  If not, we can't do any better than
>   	     ALIGN.  */

Perhaps assert that we should not be getting a PLUS_EXPR here?

>   std_expand_builtin_va_start (tree valist, rtx nextarg)
>   {
>     tree t;
> +   t = make_tree (sizetype, nextarg);
> +   t = fold_convert (ptr_type_node, t);
>   
> !   t = build2 (MODIFY_EXPR, TREE_TYPE (valist), valist,t);

s/,t/, t/

>     expand_expr (t, const0_rtx, VOIDmode, EXPAND_NORMAL);
> *************** std_gimplify_va_arg_expr (tree valist, t
> *** 4719,4732 ****
>     if (boundary > align

[ ... ]

> ! 	  tree temp = fold_convert (TREE_TYPE (dest), len);
> ! 	  temp = fold_build2 (PLUS_EXPR, TREE_TYPE (dest), dest, temp);
>   	  return fold_convert (TREE_TYPE (TREE_TYPE (fndecl)), temp);
>   	}
>       }
> --- 11628,11634 ----
>   	return omit_one_operand (TREE_TYPE (TREE_TYPE (fndecl)), dest, len);
>         else
>   	{
> ! 	  tree temp = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest, len);
>   	  return fold_convert (TREE_TYPE (TREE_TYPE (fndecl)), temp);

Mind numbing, so I mostly browsed through.  Seemed mechanical.

> *************** try_move_mult_to_index (enum tree_code c
> *** 6882,6887 ****
> --- 6896,6904 ----
>     tree itype;
>     bool mdim = false;
>   
> +   /*  Stip the nops that might be added when converting op1 to sizetype. */

s/Stip/Strip/

> Index: cp/ChangeLog.ptr
> ===================================================================
> *** cp/ChangeLog.ptr	(.../trunk/gcc)	(revision 0)
> --- cp/ChangeLog.ptr	(.../branches/pointer_plus/gcc)	(revision 125672)

Move to cp/ChangeLog.

> *************** maybe_fold_stmt_addition (tree expr)
> *** 1944,1961 ****
>     tree ptr_type = TREE_TYPE (expr);
>     tree ptd_type;
>     tree t;
> -   bool subtract = (TREE_CODE (expr) == MINUS_EXPR);
>   

Add gcc_assert (TREE_CODE (expr) == POINTER_PLUS_EXPR).  It's only
called with POINTER_PLUS_EXPR now, but we were calling it with
MINUS_EXPR before.


> ===================================================================
> *** ada/ChangeLog.ptr	(.../trunk/gcc)	(revision 0)
> --- ada/ChangeLog.ptr	(.../branches/pointer_plus/gcc)	(revision 125672)

Move to ada/ChangeLog

> ===================================================================
> *** fortran/ChangeLog.ptr	(.../trunk/gcc)	(revision 0)
> --- fortran/ChangeLog.ptr	(.../branches/pointer_plus/gcc)	(revision 125672)

Move to fortran/ChangeLog

> ===================================================================
> *** tree.def	(.../trunk/gcc)	(revision 125658)
> --- tree.def	(.../branches/pointer_plus/gcc)	(revision 125672)
> *************** DEFTREECODE (PLUS_EXPR, "plus_expr", tcc
> *** 612,617 ****
> --- 612,621 ----
>   DEFTREECODE (MINUS_EXPR, "minus_expr", tcc_binary, 2)
>   DEFTREECODE (MULT_EXPR, "mult_expr", tcc_binary, 2)
>   
> + /* Plus expression but for pointers in that the first operand is always a pointer
> +    and the second operand is an integer which is the same type as the sizetype.  */

How about:

/* Pointer addition.  The first operand is always a pointer and the
second operand is an integer of type sizetype.  */



More information about the Java-patches mailing list