[tuples] dereference POINTER_PLUS_EXPR check

[tuples] dereference POINTER_PLUS_EXPR check

[Aldy Hernandez]

Hi Diego.

Many fortran testcases are failing in verify_types_in_gimple_assign()
because, in a GIMPLE_ASSIGN, a POINTER_PLUS_EXPR can be embeddeded in
an assignment.  However, when verifying type validity, we no longer
have the type of the POINTER_PLUS_EXPR.  In the absence of this type, we
must look at the pointed-to type to determine type compatability.

This patch drills down to the dereferenced type.

With this patch, we have no ICEs that are not already present on
mainline while checking fortran.

Is this OK for the tuples branch?

Aldy

	* tree-cfg.c (verify_types_in_gimple_assign): Use the dereferenced
	type when checking the validity of a POINTER_PLUS_EXPR.

Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 130313)
+++ tree-cfg.c	(working copy)
@@ -3558,16 +3558,37 @@ verify_types_in_gimple_assign (gimple st
 	    error ("invalid operands in pointer plus expression");
 	    return true;
 	  }
-	if (!POINTER_TYPE_P (rhs1_type)
-	    || !useless_type_conversion_p (lhs_type, rhs1_type)
+
+	if (!POINTER_TYPE_P (lhs_type)
+	    || !POINTER_TYPE_P (rhs1_type))
+	  {
+	    error ("type mismatch in pointer plus expression");
+	    return true;
+	  }
+
+	/* Drill down to get to the pointed-to type.  */
+	{
+	  tree lhs_type_orig = lhs_type;
+	  tree rhs1_type_orig = rhs1_type;
+
+	  while (POINTER_TYPE_P (rhs1_type)
+		 || TREE_CODE (rhs1_type) == ARRAY_TYPE)
+	    rhs1_type = TREE_TYPE (rhs1_type);
+
+	  while (POINTER_TYPE_P (lhs_type)
+		 || TREE_CODE (lhs_type) == ARRAY_TYPE)
+	    lhs_type = TREE_TYPE (lhs_type);
+
+	if (!useless_type_conversion_p (lhs_type, rhs1_type)
 	    || !useless_type_conversion_p (sizetype, rhs2_type))
 	  {
 	    error ("type mismatch in pointer plus expression");
-	    debug_generic_stmt (lhs_type);
-	    debug_generic_stmt (rhs1_type);
+	    debug_generic_stmt (lhs_type_orig);
+	    debug_generic_stmt (rhs1_type_orig);
 	    debug_generic_stmt (rhs2_type);
 	    return true;
 	  }
+	}
 
 	return false;
       } 

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Diego Novillo]

On 11/21/07 15:46, Aldy Hernandez wrote:

> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c	(revision 130313)
> +++ tree-cfg.c	(working copy)
> @@ -3558,16 +3558,37 @@ verify_types_in_gimple_assign (gimple st
>  	    error ("invalid operands in pointer plus expression");
>  	    return true;
>  	  }
> -	if (!POINTER_TYPE_P (rhs1_type)
> -	    || !useless_type_conversion_p (lhs_type, rhs1_type)
> +
> +	if (!POINTER_TYPE_P (lhs_type)
> +	    || !POINTER_TYPE_P (rhs1_type))
> +	  {
> +	    error ("type mismatch in pointer plus expression");
> +	    return true;
> +	  }
> +
> +	/* Drill down to get to the pointed-to type.  */

Maybe we could make this a helper in tree.c.  Something like 
pointed_to_type().  I don't think one exists already, but I may be wrong.

OK with that change.  Thanks.


Diego.

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Aldy Hernandez]

> Maybe we could make this a helper in tree.c.  Something like 
> pointed_to_type().  I don't think one exists already, but I may be wrong.

Things are a bit cleaner now too, bonus!

Is this OK?

	* tree.c (pointed_to_type): New.
	* tree.h (pointed_to_type): Protoize.
	* tree-cfg.c (verify_types_in_gimple_assign): Use the dereferenced
	type when checking the validity of a POINTER_PLUS_EXPR.

Index: tree.c
===================================================================
--- tree.c	(revision 130313)
+++ tree.c	(working copy)
@@ -8833,4 +8833,14 @@ block_nonartificial_location (tree block
   return ret;
 }
 
+/* Given a tree type T, drill down to the type it points to and return
+   it.  */
+tree
+pointed_to_type (tree t)
+{
+  while (POINTER_TYPE_P (t) || TREE_CODE (t) == ARRAY_TYPE)
+    t = TREE_TYPE (t);
+  return t;
+}
+
 #include "gt-tree.h"
Index: tree.h
===================================================================
--- tree.h	(revision 130313)
+++ tree.h	(working copy)
@@ -4918,6 +4918,7 @@ extern tree *tree_block (tree);
 extern tree *generic_tree_operand (tree, int);
 extern tree *generic_tree_type (tree);
 extern location_t *block_nonartificial_location (tree);
+extern tree pointed_to_type (tree);
 
 /* In function.c */
 extern void expand_main_function (void);
Index: tree-cfg.c
===================================================================
--- tree-cfg.c	(revision 130313)
+++ tree-cfg.c	(working copy)
@@ -3558,8 +3558,16 @@ verify_types_in_gimple_assign (gimple st
 	    error ("invalid operands in pointer plus expression");
 	    return true;
 	  }
-	if (!POINTER_TYPE_P (rhs1_type)
-	    || !useless_type_conversion_p (lhs_type, rhs1_type)
+
+	if (!POINTER_TYPE_P (lhs_type)
+	    || !POINTER_TYPE_P (rhs1_type))
+	  {
+	    error ("type mismatch in pointer plus expression");
+	    return true;
+	  }
+
+	if (!useless_type_conversion_p (pointed_to_type (lhs_type),
+					pointed_to_type (rhs1_type))
 	    || !useless_type_conversion_p (sizetype, rhs2_type))
 	  {
 	    error ("type mismatch in pointer plus expression");

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Richard Guenther]

On Nov 21, 2007 10:52 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > Maybe we could make this a helper in tree.c.  Something like
> > pointed_to_type().  I don't think one exists already, but I may be wrong.
>
> Things are a bit cleaner now too, bonus!
>
> Is this OK?

No, sorry.  It doesn't make sense to check the pointed-to types.  Or do you
say the modify expr embeds a de-reference?

Richard.

>         * tree.c (pointed_to_type): New.
>         * tree.h (pointed_to_type): Protoize.
>         * tree-cfg.c (verify_types_in_gimple_assign): Use the dereferenced
>         type when checking the validity of a POINTER_PLUS_EXPR.
>
> Index: tree.c
> ===================================================================
> --- tree.c      (revision 130313)
> +++ tree.c      (working copy)
> @@ -8833,4 +8833,14 @@ block_nonartificial_location (tree block
>    return ret;
>  }
>
> +/* Given a tree type T, drill down to the type it points to and return
> +   it.  */
> +tree
> +pointed_to_type (tree t)
> +{
> +  while (POINTER_TYPE_P (t) || TREE_CODE (t) == ARRAY_TYPE)
> +    t = TREE_TYPE (t);
> +  return t;
> +}
> +
>  #include "gt-tree.h"
> Index: tree.h
> ===================================================================
> --- tree.h      (revision 130313)
> +++ tree.h      (working copy)
> @@ -4918,6 +4918,7 @@ extern tree *tree_block (tree);
>  extern tree *generic_tree_operand (tree, int);
>  extern tree *generic_tree_type (tree);
>  extern location_t *block_nonartificial_location (tree);
> +extern tree pointed_to_type (tree);
>
>  /* In function.c */
>  extern void expand_main_function (void);
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 130313)
> +++ tree-cfg.c  (working copy)
> @@ -3558,8 +3558,16 @@ verify_types_in_gimple_assign (gimple st
>             error ("invalid operands in pointer plus expression");
>             return true;
>           }
> -       if (!POINTER_TYPE_P (rhs1_type)
> -           || !useless_type_conversion_p (lhs_type, rhs1_type)
> +
> +       if (!POINTER_TYPE_P (lhs_type)
> +           || !POINTER_TYPE_P (rhs1_type))
> +         {
> +           error ("type mismatch in pointer plus expression");
> +           return true;
> +         }
> +
> +       if (!useless_type_conversion_p (pointed_to_type (lhs_type),
> +                                       pointed_to_type (rhs1_type))
>             || !useless_type_conversion_p (sizetype, rhs2_type))
>
>           {
>             error ("type mismatch in pointer plus expression");
>

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Aldy Hernandez]

On Wed, Nov 21, 2007 at 10:57:30PM +0100, Richard Guenther wrote:
> On Nov 21, 2007 10:52 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > > Maybe we could make this a helper in tree.c.  Something like
> > > pointed_to_type().  I don't think one exists already, but I may be wrong.
> >
> > Things are a bit cleaner now too, bonus!
> >
> > Is this OK?
> 
> No, sorry.  It doesn't make sense to check the pointed-to types.  Or do you
> say the modify expr embeds a de-reference?

The modify_expr now embeds a POINTER_PLUS_EXPR.  

Where previously we had:

	(MODIFY_EXPR lhs (POINTER_PLUS_EXPR rhs1 rhs2))

We now have:

	(GIMPLE_ASSIGN lhs rhs1 rhs2)

with the POINTER_PLUS_EXPR in the subcode of the GIMPLE_ASSIGN.  We have
collapsed the MODIFY_EXPR and the POINTER_PLUS_EXPR nodes into one
tuple.

Previously, we would verify type sanity by checking the type of the LHS
(which should be a pointer) with the type of the P_P_E.  We no longer
have the P_P_E with its type, so now we must look at the pointed-to
types of both lhs and rhs1.

Why do you think it doesn't make sense to look at the pointed-to types?

Aldy

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Richard Guenther]

On Nov 21, 2007 11:58 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On Wed, Nov 21, 2007 at 10:57:30PM +0100, Richard Guenther wrote:
> > On Nov 21, 2007 10:52 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > > > Maybe we could make this a helper in tree.c.  Something like
> > > > pointed_to_type().  I don't think one exists already, but I may be wrong.
> > >
> > > Things are a bit cleaner now too, bonus!
> > >
> > > Is this OK?
> >
> > No, sorry.  It doesn't make sense to check the pointed-to types.  Or do you
> > say the modify expr embeds a de-reference?
>
> The modify_expr now embeds a POINTER_PLUS_EXPR.
>
> Where previously we had:
>
>         (MODIFY_EXPR lhs (POINTER_PLUS_EXPR rhs1 rhs2))
>
> We now have:
>
>         (GIMPLE_ASSIGN lhs rhs1 rhs2)
>
> with the POINTER_PLUS_EXPR in the subcode of the GIMPLE_ASSIGN.  We have
> collapsed the MODIFY_EXPR and the POINTER_PLUS_EXPR nodes into one
> tuple.

Yes.  This is what I thought.

> Previously, we would verify type sanity by checking the type of the LHS
> (which should be a pointer) with the type of the P_P_E.

Previously we had an additional type available, the type of the result
of the P_P_E.  What we checked is that the first argument of the PPE
is a pointer, that the second argument is convertible to sizetype and
that the first argument is convertible to the result type.

With tuples the result type is now simply the type of the lhs of the
MODIFY_EXPR.
Previously for the GIMPLE_MODIFY_STMT it hold that

  useless_type_conversion (lhs, rhs)

and for the P_P_E we had

  useless_type_conversion (rhs, op0)

now due to the transitiveness of u_t_c, we now simply need to check

  useless_type_conversion (lhs, op0)

which in your notation is lhs and rhs1.  Which is exactly what the code did
before your patch.

>  We no longer
> have the P_P_E with its type, so now we must look at the pointed-to
> types of both lhs and rhs1.

Why are the pointed-to types relevant at all?  Type relations for the
pointed-to types don't necessary translate to the relations of the
pointer types.
For example useless_type_conversion (void *, int *) == true but
useless_type_conversion (void, int) is certainly not true.

> Why do you think it doesn't make sense to look at the pointed-to types?

Because - pointed-to types don't have anything to do with the type validity
of a P_P_E.

Richard.

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Aldy Hernandez]

> now due to the transitiveness of u_t_c, we now simply need to check
> 
>   useless_type_conversion (lhs, op0)
> 
> which in your notation is lhs and rhs1.  Which is exactly what the code did
> before your patch.

...

> Why are the pointed-to types relevant at all?  Type relations for the
> pointed-to types don't necessary translate to the relations of the
> pointer types.
> For example useless_type_conversion (void *, int *) == true but
> useless_type_conversion (void, int) is certainly not true.

This is the failed testcase that prompted the patch:

(gdb) ptt lhs_type
 <pointer_type 0x2aaaaf571600
    type <integer_type 0x2aaaaf559240 character(kind=1) public unsigned string-flag QI

...

(gdb) ptt rhs1_type
 <reference_type 0x2aaaaf5e6240
    type <array_type 0x2aaaaf5e6180
        type <integer_type 0x2aaaaf559240 character(kind=1) public unsigned string-flag QI

u_t_c(lhs_type, rhs1_type) fails because it doesn't look far enough
down, but you're saying it shouldn't.  Is u_t_c wrong then?

Aldy

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Andrew Pinski]

On 11/21/07, Aldy Hernandez <aldyh@redhat.com> wrote:
> u_t_c(lhs_type, rhs1_type) fails because it doesn't look far enough
> down, but you're saying it shouldn't.  Is u_t_c wrong then?

No it is correct.  So we have char* = char[]& .  That is incorrect
where are the cast or address expression.

We should get  b = &(*a)[0] + c (or b = &(*a)[c] ).

-- Pinski

[FORTRAN mainline] dereference POINTER_PLUS_EXPR check (was tuples)

[Aldy Hernandez]

On Wed, Nov 21, 2007 at 05:09:22PM -0800, Andrew Pinski wrote:
> On 11/21/07, Aldy Hernandez <aldyh@redhat.com> wrote:
> > u_t_c(lhs_type, rhs1_type) fails because it doesn't look far enough
> > down, but you're saying it shouldn't.  Is u_t_c wrong then?
> 
> No it is correct.  So we have char* = char[]& .  That is incorrect
> where are the cast or address expression.

Thanks for the help guys.

The culprit is the fortran FE which is building a P_P_E with no regards
to type.  The patch below uses [void *] when building BUILT_IN_MEMSET.
It has been tested on x86-linux, with no regressions on the Fortran test
cases.

OK for *mainline*, as it suffers from the same problem?

Aldy

        * fortran/trans-expr.c (gfc_trans_string_copy): Use "void *" * when
        building a memmove.

=== fortran/trans-expr.c
==================================================================
--- fortran/trans-expr.c        (revision 130317)
+++ fortran/trans-expr.c        (local)
@@ -2701,7 +2701,7 @@
   tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
                          3, dest, src, slen);
 
-  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
+  tmp4 = fold_build2 (POINTER_PLUS_EXPR, ptr_type_node, dest,
                      fold_convert (sizetype, slen));
   tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,
                          tmp4, 

Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check (was tuples)

[Richard Guenther]

On Nov 22, 2007 6:23 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> On Wed, Nov 21, 2007 at 05:09:22PM -0800, Andrew Pinski wrote:
> > On 11/21/07, Aldy Hernandez <aldyh@redhat.com> wrote:
> > > u_t_c(lhs_type, rhs1_type) fails because it doesn't look far enough
> > > down, but you're saying it shouldn't.  Is u_t_c wrong then?
> >
> > No it is correct.  So we have char* = char[]& .  That is incorrect
> > where are the cast or address expression.
>
> Thanks for the help guys.
>
> The culprit is the fortran FE which is building a P_P_E with no regards
> to type.  The patch below uses [void *] when building BUILT_IN_MEMSET.
> It has been tested on x86-linux, with no regressions on the Fortran test
> cases.

It should use the type of DEST, all pointers are trivially convertible
to void *,
which is the formal argument of memset (in the ChangeLog you have memmove).

Not that it makes a big difference here (in the end we try to be type-exact in
one expression tree, but as the expression result type will vanish with tuples,
there won't a difference for both cases).

I wonder if the problem you see has been fixed with the fix for PR31608 which
went in a few days ago?

Richard.

> OK for *mainline*, as it suffers from the same problem?
>
> Aldy
>
>         * fortran/trans-expr.c (gfc_trans_string_copy): Use "void *" * when
>         building a memmove.
>
> === fortran/trans-expr.c
> ==================================================================
> --- fortran/trans-expr.c        (revision 130317)
> +++ fortran/trans-expr.c        (local)
> @@ -2701,7 +2701,7 @@
>    tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
>                           3, dest, src, slen);
>
> -  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
> +  tmp4 = fold_build2 (POINTER_PLUS_EXPR, ptr_type_node, dest,
>                       fold_convert (sizetype, slen));
>    tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,
>                           tmp4,
>
>

Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check (was tuples)

[Aldy Hernandez]

> It should use the type of DEST, all pointers are trivially convertible
> to void *,
> which is the formal argument of memset (in the ChangeLog you have memmove).
> 
> Not that it makes a big difference here (in the end we try to be type-exact in
> one expression tree, but as the expression result type will vanish with tuples,
> there won't a difference for both cases).

Fixed below.

> I wonder if the problem you see has been fixed with the fix for PR31608 which
> went in a few days ago?

No.  Latest mainline still exhibited the same problem.  The P_P_E was
still wrong.

I know Tobias approved the patch, but let me know if this updated one
using the type of [dest] is what you wanted.

No regressions on fortran tests.

Aldy

	* trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
	memset.
 
--- trans-expr.c	(revision 130355)
+++ trans-expr.c	(local)
@@ -2701,7 +2701,7 @@ gfc_trans_string_copy (stmtblock_t * blo
   tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
 			  3, dest, src, slen);
 
-  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
+  tmp4 = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest,
 		      fold_convert (sizetype, slen));
   tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,
 			  tmp4, 

Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check (was tuples)

[Richard Guenther]

On Nov 23, 2007 1:27 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > It should use the type of DEST, all pointers are trivially convertible
> > to void *,
> > which is the formal argument of memset (in the ChangeLog you have memmove).
> >
> > Not that it makes a big difference here (in the end we try to be type-exact in
> > one expression tree, but as the expression result type will vanish with tuples,
> > there won't a difference for both cases).
>
> Fixed below.
>
> > I wonder if the problem you see has been fixed with the fix for PR31608 which
> > went in a few days ago?
>
> No.  Latest mainline still exhibited the same problem.  The P_P_E was
> still wrong.
>
> I know Tobias approved the patch, but let me know if this updated one
> using the type of [dest] is what you wanted.

Yes, thanks.

Richard.

> No regressions on fortran tests.
>
> Aldy
>
>         * trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
>         memset.
>
> --- trans-expr.c        (revision 130355)
> +++ trans-expr.c        (local)
> @@ -2701,7 +2701,7 @@ gfc_trans_string_copy (stmtblock_t * blo
>    tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
>                           3, dest, src, slen);
>
> -  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
> +  tmp4 = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest,
>
>                       fold_convert (sizetype, slen));
>    tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,
>                           tmp4,
>

libgomp failure (was: Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check)

[Tobias Burnus]

Aldy Hernandez wrote:
>> It should use the type of DEST, all pointers are trivially convertible
>> to void *,
>> which is the formal argument of memset (in the ChangeLog you have memmove).
>>
>> Not that it makes a big difference here (in the end we try to be type-exact in
>> one expression tree, but as the expression result type will vanish with tuples,
>> there won't a difference for both cases).
>>     
> Fixed below.
>   
This patch seems to cause a lot of libgomp regressions.
See: http://gcc.gnu.org/ml/fortran/2007-11/msg00209.html

For libgomp.fortran/character1.f90 with "gfortran -fopenmp":

libgomp.fortran/character1.f90: In function 'test':
libgomp.fortran/character1.f90:39: internal compiler error: in
omp_add_variable, at gimplify.c:4677

(without "-fopenmp" there is no failure)


If I revert the following patch, the ICE is gone.


r130371 | aldyh | 2007-11-23 11:50:45 +0100 (Fri, 23 Nov 2007) | 2 lines

+       * trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
+       memset.
> 	* trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
> 	memset.
>  
> --- trans-expr.c	(revision 130355)
> +++ trans-expr.c	(local)
> @@ -2701,7 +2701,7 @@ gfc_trans_string_copy (stmtblock_t * blo
>    tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
>  			  3, dest, src, slen);
>  
> -  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
> +  tmp4 = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest,
>  		      fold_convert (sizetype, slen));
>    tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,
>  			  tmp4, 
>
>   


Tobias

Re: libgomp failure (was: Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check)

[Aldy Hernandez]

> This patch seems to cause a lot of libgomp regressions.
> See: http://gcc.gnu.org/ml/fortran/2007-11/msg00209.html

Arghhh, I thought a check-fortran in the gcc directory would check all
of fortran.  I forgot about gomp.

Sorry about this, I will either fix or revert the patch before my day is
done, today.

Aldy

Re: libgomp failure (was: Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check)

[Aldy Hernandez]

Hi folks.  Hi Jakub.

I've found what the cause of the problem reported below, but I am unsure
as to how to fix it.  Perhaps Jakub can shed some light.

With the patch below, the type of the P_P_E is now:

    type <reference_type 0x2aaaaab90240
        type <array_type 0x2aaaaab90180 type ...

gfc_omp_privatize_by_reference is now being called with a temporary of
the above type.  This function returns TRUE for any REFERENCE_TYPE.
Without the patch, it obviously returned pchar_type_node which returned
false.

My naive solution is to use PTR_TYPE_NODE instead of TREE_TYPE(dest) in
my original patch, but Richard G had commented that we should ideally be
type-exact in the expression tree.  Do folks care if I make this
PTR_TYPE_NODE, or is a fix in gfc_omp_privatize_by_reference in order?

Thanks.
Aldy

> This patch seems to cause a lot of libgomp regressions.
> See: http://gcc.gnu.org/ml/fortran/2007-11/msg00209.html
...
> +       * trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
> +       memset.
> > 	* trans-expr.c (gfc_trans_string_copy): Use "void *" when building a
> > 	memset.
> >  
> > --- trans-expr.c	(revision 130355)
> > +++ trans-expr.c	(local)
> > @@ -2701,7 +2701,7 @@ gfc_trans_string_copy (stmtblock_t * blo
> >    tmp3 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
> >  			  3, dest, src, slen);
> >  
> > -  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
> > +  tmp4 = fold_build2 (POINTER_PLUS_EXPR, TREE_TYPE (dest), dest,
> >  		      fold_convert (sizetype, slen));
> >    tmp4 = build_call_expr (built_in_decls[BUILT_IN_MEMSET], 3,

Re: libgomp failure (was: Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check)

[Richard Guenther]

On Nov 25, 2007 4:04 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi folks.  Hi Jakub.
>
> I've found what the cause of the problem reported below, but I am unsure
> as to how to fix it.  Perhaps Jakub can shed some light.
>
> With the patch below, the type of the P_P_E is now:
>
>     type <reference_type 0x2aaaaab90240
>         type <array_type 0x2aaaaab90180 type ...
>
> gfc_omp_privatize_by_reference is now being called with a temporary of
> the above type.  This function returns TRUE for any REFERENCE_TYPE.
> Without the patch, it obviously returned pchar_type_node which returned
> false.
>
> My naive solution is to use PTR_TYPE_NODE instead of TREE_TYPE(dest) in
> my original patch, but Richard G had commented that we should ideally be
> type-exact in the expression tree.  Do folks care if I make this
> PTR_TYPE_NODE, or is a fix in gfc_omp_privatize_by_reference in order?

Uh, that it handles REFERENCE_TYPEs as special (to get value semantics
here I guess) and you now trip over this looks like there is a lantent bug
either to use a REFERENCE_TYPE for the array in the first place or that it
not properly privatizes arrays that are passed by value.

So I think this needs further investigation anyway.  But yes, Jakub maybe knows
about the history of this function.

Richard.

[PATCH] Fix fortran libgomp failures

[Jakub Jelinek]

On Sun, Nov 25, 2007 at 11:04:24AM -0400, Aldy Hernandez wrote:
> I've found what the cause of the problem reported below, but I am unsure
> as to how to fix it.  Perhaps Jakub can shed some light.
> 
> With the patch below, the type of the P_P_E is now:
> 
>     type <reference_type 0x2aaaaab90240
>         type <array_type 0x2aaaaab90180 type ...
> 
> gfc_omp_privatize_by_reference is now being called with a temporary of
> the above type.  This function returns TRUE for any REFERENCE_TYPE.
> Without the patch, it obviously returned pchar_type_node which returned
> false.
> 
> My naive solution is to use PTR_TYPE_NODE instead of TREE_TYPE(dest) in
> my original patch, but Richard G had commented that we should ideally be
> type-exact in the expression tree.  Do folks care if I make this
> PTR_TYPE_NODE, or is a fix in gfc_omp_privatize_by_reference in order?

Here is a patch that does both.  It is just plain wrong to use
say REFERENCE_TYPE in the memset call, we have there a pointer/reference
to some array and we do
memmove (dest, src, slen);
memset (dest + slen, ' ', dlen - slen);
but dest + slen arithmetics is not adding some multiple of the *dest
sizes, but just some number of bytes.  So, either we should use
pchar_type_node, or pvoid_type_node.  I chose the latter because memmove and
memset arguments have void * types.

REFERENCE_TYPEs emitted by gfortran FE so far were always what
gfc_omp_privatize_by_reference expected - parameters or results (passed
indirectly, so parameters again), so just returning true always was ok.
With the first hunk no change is needed either, but just to make Richi
happy I've changed it as well to make sure REFERENCE_TYPE is only special
on parameters (which can be PARM_DECL, or e.g. for return types VAR_DECL),
some PARM_DECL have DECL_ARTIFICIAL set, fortunately all VAR_DECLs which
need this treatment are not DECL_ARTIFICIAL.

Regtested on x86_64-linux, ok for trunk?

2007-11-27  Jakub Jelinek  <jakub@redhat.com>

	* trans-expr.c (gfc_trans_string_copy): Convert both dest and
	src to void *.

	* trans-openmp.c (gfc_omp_privatize_by_reference): For REFERENCE_TYPE
	pass by reference only PARM_DECLs or non-artificial decls.

--- gcc/fortran/trans-expr.c.jj	2007-11-26 11:02:23.000000000 +0100
+++ gcc/fortran/trans-expr.c	2007-11-27 09:04:31.000000000 +0100
@@ -2708,7 +2708,10 @@ gfc_trans_string_copy (stmtblock_t * blo
 
      We're now doing it here for better optimization, but the logic
      is the same.  */
-  
+
+  dest = fold_convert (pvoid_type_node, dest);
+  src = fold_convert (pvoid_type_node, src);
+
   /* Truncate string if source is too long.  */
   cond2 = fold_build2 (GE_EXPR, boolean_type_node, slen, dlen);
   tmp2 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
--- gcc/fortran/trans-openmp.c.jj	2007-08-27 10:15:33.000000000 +0200
+++ gcc/fortran/trans-openmp.c	2007-11-27 09:39:40.000000000 +0100
@@ -44,7 +44,8 @@ gfc_omp_privatize_by_reference (const_tr
 {
   tree type = TREE_TYPE (decl);
 
-  if (TREE_CODE (type) == REFERENCE_TYPE)
+  if (TREE_CODE (type) == REFERENCE_TYPE
+      && (!DECL_ARTIFICIAL (decl) || TREE_CODE (decl) == PARM_DECL))
     return true;
 
   if (TREE_CODE (type) == POINTER_TYPE)

	Jakub

Re: [PATCH] Fix fortran libgomp failures

[Richard Guenther]

On Nov 27, 2007 10:32 AM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Sun, Nov 25, 2007 at 11:04:24AM -0400, Aldy Hernandez wrote:
> > I've found what the cause of the problem reported below, but I am unsure
> > as to how to fix it.  Perhaps Jakub can shed some light.
> >
> > With the patch below, the type of the P_P_E is now:
> >
> >     type <reference_type 0x2aaaaab90240
> >         type <array_type 0x2aaaaab90180 type ...
> >
> > gfc_omp_privatize_by_reference is now being called with a temporary of
> > the above type.  This function returns TRUE for any REFERENCE_TYPE.
> > Without the patch, it obviously returned pchar_type_node which returned
> > false.
> >
> > My naive solution is to use PTR_TYPE_NODE instead of TREE_TYPE(dest) in
> > my original patch, but Richard G had commented that we should ideally be
> > type-exact in the expression tree.  Do folks care if I make this
> > PTR_TYPE_NODE, or is a fix in gfc_omp_privatize_by_reference in order?
>
> Here is a patch that does both.  It is just plain wrong to use
> say REFERENCE_TYPE in the memset call, we have there a pointer/reference
> to some array and we do
> memmove (dest, src, slen);
> memset (dest + slen, ' ', dlen - slen);
> but dest + slen arithmetics is not adding some multiple of the *dest
> sizes, but just some number of bytes.  So, either we should use
> pchar_type_node, or pvoid_type_node.  I chose the latter because memmove and
> memset arguments have void * types.

I don't think there is anything wrong with using a REFERENCE_TYPE to
some array in this case (arithmetics always operate in bytes, not elements),
but obviously using void * types also works here (and is completely equivalent,
you'll see the casts removed as useless and the original pointer types
propagated)

> REFERENCE_TYPEs emitted by gfortran FE so far were always what
> gfc_omp_privatize_by_reference expected - parameters or results (passed
> indirectly, so parameters again), so just returning true always was ok.
> With the first hunk no change is needed either, but just to make Richi
> happy I've changed it as well to make sure REFERENCE_TYPE is only special
> on parameters (which can be PARM_DECL, or e.g. for return types VAR_DECL),
> some PARM_DECL have DECL_ARTIFICIAL set, fortunately all VAR_DECLs which
> need this treatment are not DECL_ARTIFICIAL.
>
> Regtested on x86_64-linux, ok for trunk?

Fine with me, even if I think the trans-expr.c part is unneeded.

Richard.

> 2007-11-27  Jakub Jelinek  <jakub@redhat.com>
>
>         * trans-expr.c (gfc_trans_string_copy): Convert both dest and
>         src to void *.
>
>         * trans-openmp.c (gfc_omp_privatize_by_reference): For REFERENCE_TYPE
>         pass by reference only PARM_DECLs or non-artificial decls.
>
> --- gcc/fortran/trans-expr.c.jj 2007-11-26 11:02:23.000000000 +0100
> +++ gcc/fortran/trans-expr.c    2007-11-27 09:04:31.000000000 +0100
> @@ -2708,7 +2708,10 @@ gfc_trans_string_copy (stmtblock_t * blo
>
>       We're now doing it here for better optimization, but the logic
>       is the same.  */
> -
> +
> +  dest = fold_convert (pvoid_type_node, dest);
> +  src = fold_convert (pvoid_type_node, src);
> +
>    /* Truncate string if source is too long.  */
>    cond2 = fold_build2 (GE_EXPR, boolean_type_node, slen, dlen);
>    tmp2 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
> --- gcc/fortran/trans-openmp.c.jj       2007-08-27 10:15:33.000000000 +0200
> +++ gcc/fortran/trans-openmp.c  2007-11-27 09:39:40.000000000 +0100
> @@ -44,7 +44,8 @@ gfc_omp_privatize_by_reference (const_tr
>  {
>    tree type = TREE_TYPE (decl);
>
> -  if (TREE_CODE (type) == REFERENCE_TYPE)
> +  if (TREE_CODE (type) == REFERENCE_TYPE
> +      && (!DECL_ARTIFICIAL (decl) || TREE_CODE (decl) == PARM_DECL))
>      return true;
>
>    if (TREE_CODE (type) == POINTER_TYPE)
>
>         Jakub
>

Re: [PATCH] Fix fortran libgomp failures

[Tobias Burnus]

Richard Guenther wrote:
>> On Nov 27, 2007 10:32 AM, Jakub Jelinek <jakub@redhat.com> wrote:
>>
>> Regtested on x86_64-linux, ok for trunk?
>
> Fine with me, even if I think the trans-expr.c part is unneeded.

It is also fine with me from the Fortran side (w/ and w/o the
trans-expr.c part).

Tobias

>> 2007-11-27  Jakub Jelinek  <jakub@redhat.com>
>>
>>         * trans-expr.c (gfc_trans_string_copy): Convert both dest and
>>         src to void *.
>>
>>         * trans-openmp.c (gfc_omp_privatize_by_reference): For REFERENCE_TYPE
>>         pass by reference only PARM_DECLs or non-artificial decls.
>>
>> --- gcc/fortran/trans-expr.c.jj 2007-11-26 11:02:23.000000000 +0100
>> +++ gcc/fortran/trans-expr.c    2007-11-27 09:04:31.000000000 +0100
>> @@ -2708,7 +2708,10 @@ gfc_trans_string_copy (stmtblock_t * blo
>>
>>       We're now doing it here for better optimization, but the logic
>>       is the same.  */
>> -
>> +
>> +  dest = fold_convert (pvoid_type_node, dest);
>> +  src = fold_convert (pvoid_type_node, src);
>> +
>>    /* Truncate string if source is too long.  */
>>    cond2 = fold_build2 (GE_EXPR, boolean_type_node, slen, dlen);
>>    tmp2 = build_call_expr (built_in_decls[BUILT_IN_MEMMOVE],
>> --- gcc/fortran/trans-openmp.c.jj       2007-08-27 10:15:33.000000000 +0200
>> +++ gcc/fortran/trans-openmp.c  2007-11-27 09:39:40.000000000 +0100
>> @@ -44,7 +44,8 @@ gfc_omp_privatize_by_reference (const_tr
>>  {
>>    tree type = TREE_TYPE (decl);
>>
>> -  if (TREE_CODE (type) == REFERENCE_TYPE)
>> +  if (TREE_CODE (type) == REFERENCE_TYPE
>> +      && (!DECL_ARTIFICIAL (decl) || TREE_CODE (decl) == PARM_DECL))
>>      return true;
>>
>>    if (TREE_CODE (type) == POINTER_TYPE)
>>

Re: [FORTRAN mainline] dereference POINTER_PLUS_EXPR check (was tuples)

[Tobias Burnus]

Aldy Hernandez wrote:
> The culprit is the fortran FE which is building a P_P_E with no regards
> to type.  The patch below uses [void *] when building BUILT_IN_MEMSET.
> It has been tested on x86-linux, with no regressions on the Fortran test
> cases.
> 
> OK for *mainline*, as it suffers from the same problem?

This patch is OK (from the FE side). (Unless a ME maintainer has
objections.)


>         * fortran/trans-expr.c (gfc_trans_string_copy): Use "void *" * when
>         building a memmove.

Use "* trans-expr.c" and place it into fortran/ChangeLog.

> +++ fortran/trans-expr.c        (local)
> -  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
> +  tmp4 = fold_build2 (POINTER_PLUS_EXPR, ptr_type_node, dest,

Tobias


PS: Please CC the next time to fortran@ as the gfortraners don't
regularily read gcc-patches.

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Richard Guenther]

On Nov 22, 2007 1:58 AM, Aldy Hernandez <aldyh@redhat.com> wrote:
> > now due to the transitiveness of u_t_c, we now simply need to check
> >
> >   useless_type_conversion (lhs, op0)
> >
> > which in your notation is lhs and rhs1.  Which is exactly what the code did
> > before your patch.
>
> ...
>
> > Why are the pointed-to types relevant at all?  Type relations for the
> > pointed-to types don't necessary translate to the relations of the
> > pointer types.
> > For example useless_type_conversion (void *, int *) == true but
> > useless_type_conversion (void, int) is certainly not true.
>
> This is the failed testcase that prompted the patch:
>
> (gdb) ptt lhs_type
>  <pointer_type 0x2aaaaf571600
>     type <integer_type 0x2aaaaf559240 character(kind=1) public unsigned string-flag QI
>
> ...
>
> (gdb) ptt rhs1_type
>  <reference_type 0x2aaaaf5e6240
>     type <array_type 0x2aaaaf5e6180
>         type <integer_type 0x2aaaaf559240 character(kind=1) public unsigned string-flag QI
>
> u_t_c(lhs_type, rhs1_type) fails because it doesn't look far enough
> down, but you're saying it shouldn't.  Is u_t_c wrong then?

Andrew is right, this is a type mismatch.

Richard.

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Diego Novillo]

On 11/21/07 18:25, Richard Guenther wrote:

> Previously we had an additional type available, the type of the result
> of the P_P_E.

Right.  This is not available anymore.  The types of _EXPR trees on the 
RHS of assignments and/or conditionals is no longer available with the 
tuple representation.

> What we checked is that the first argument of the PPE
> is a pointer, that the second argument is convertible to sizetype and
> that the first argument is convertible to the result type.

Yes, and we still check this.

> With tuples the result type is now simply the type of the lhs of the
> MODIFY_EXPR.

Yes.

>> Why do you think it doesn't make sense to look at the pointed-to types?
> 
> Because - pointed-to types don't have anything to do with the type validity
> of a P_P_E.

Yeah, good point.  I was wrong in my initial analysis.  The expression 
that we were failing to validate needed to have casts and I missed that.

Sorry for leading you down this rat hole, Aldy.


Diego.

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Aldy Hernandez]

> Sorry for leading you down this rat hole, Aldy.

No worries, I have found the culprit in the Fortran front-end, where we
build the P_P_E with no regards to type:

  tmp4 = fold_build2 (POINTER_PLUS_EXPR, pchar_type_node, dest,
                      fold_convert (sizetype, slen));

(This problem is present in mainline as well).

I'm working on a patch.

Aldy

Re: [tuples] dereference POINTER_PLUS_EXPR check

[Richard Guenther]

On Nov 21, 2007 9:46 PM, Aldy Hernandez <aldyh@redhat.com> wrote:
> Hi Diego.
>
> Many fortran testcases are failing in verify_types_in_gimple_assign()
> because, in a GIMPLE_ASSIGN, a POINTER_PLUS_EXPR can be embeddeded in
> an assignment.  However, when verifying type validity, we no longer
> have the type of the POINTER_PLUS_EXPR.  In the absence of this type, we
> must look at the pointed-to type to determine type compatability.
>
> This patch drills down to the dereferenced type.

Huh, this looks wrong.  The type of the POINTER_PLUS_EXPR is the type
of the LHS.  Which means the check as it is now is correct.

Richard.

> With this patch, we have no ICEs that are not already present on
> mainline while checking fortran.
>
> Is this OK for the tuples branch?
>
> Aldy
>
>         * tree-cfg.c (verify_types_in_gimple_assign): Use the dereferenced
>         type when checking the validity of a POINTER_PLUS_EXPR.
>
> Index: tree-cfg.c
> ===================================================================
> --- tree-cfg.c  (revision 130313)
> +++ tree-cfg.c  (working copy)
> @@ -3558,16 +3558,37 @@ verify_types_in_gimple_assign (gimple st
>             error ("invalid operands in pointer plus expression");
>             return true;
>           }
> -       if (!POINTER_TYPE_P (rhs1_type)
> -           || !useless_type_conversion_p (lhs_type, rhs1_type)
> +
> +       if (!POINTER_TYPE_P (lhs_type)
> +           || !POINTER_TYPE_P (rhs1_type))
> +         {
> +           error ("type mismatch in pointer plus expression");
> +           return true;
> +         }
> +
> +       /* Drill down to get to the pointed-to type.  */
> +       {
> +         tree lhs_type_orig = lhs_type;
> +         tree rhs1_type_orig = rhs1_type;
> +
> +         while (POINTER_TYPE_P (rhs1_type)
> +                || TREE_CODE (rhs1_type) == ARRAY_TYPE)
> +           rhs1_type = TREE_TYPE (rhs1_type);
> +
> +         while (POINTER_TYPE_P (lhs_type)
> +                || TREE_CODE (lhs_type) == ARRAY_TYPE)
> +           lhs_type = TREE_TYPE (lhs_type);
> +
> +       if (!useless_type_conversion_p (lhs_type, rhs1_type)
>             || !useless_type_conversion_p (sizetype, rhs2_type))
>           {
>             error ("type mismatch in pointer plus expression");
> -           debug_generic_stmt (lhs_type);
> -           debug_generic_stmt (rhs1_type);
> +           debug_generic_stmt (lhs_type_orig);
> +           debug_generic_stmt (rhs1_type_orig);
>             debug_generic_stmt (rhs2_type);
>             return true;
>           }
> +       }
>
>         return false;
>        }
>