Re: [gomp4.1] calculate pointer offsets for depend(sink)

[Aldy Hernandez <aldyh at redhat dot com>]

On 07/21/2015 09:30 AM, Jakub Jelinek wrote:
> On Tue, Jul 21, 2015 at 09:08:27AM -0700, Aldy Hernandez wrote:
> > --- a/gcc/c/c-typeck.c
> > +++ b/gcc/c/c-typeck.c
> > @@ -12492,6 +12492,24 @@ c_finish_omp_clauses (tree clauses, bool declare_simd)
> >   	  if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
> >   	    {
> >   	      gcc_assert (TREE_CODE (t) == TREE_LIST);
> > +	      for (; t; t = TREE_CHAIN (t))
> > +		{
> > +		  tree decl = TREE_VALUE (t);
> > +		  if (TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE)
> > +		    {
> > +		      tree offset = TREE_PURPOSE (t);
> > +		      bool neg = wi::neg_p ((wide_int) offset);
> > +		      offset = fold_unary (ABS_EXPR, TREE_TYPE (offset), offset);
> > +		      tree t2 = pointer_int_sum (OMP_CLAUSE_LOCATION (c),
> > +						 neg ? MINUS_EXPR : PLUS_EXPR,
> > +						 decl, offset);
> > +		      t2 = fold_build2_loc (OMP_CLAUSE_LOCATION (c), MINUS_EXPR,
> > +					    sizetype, t2, decl);
> > +		      if (t2 == error_mark_node)
> > +			t2 = size_one_node; // ??
>
> I think we should just remove the whole OMP_CLAUSE_DEPEND_SINK clause
> in case of errors.

That was my first idea, but then I saw the discrepancy in the C and C++ 
FE's handling OMP_CLAUSE_LINEAR_STEP.  The C FE has:

	      if (s == error_mark_node)
		s = size_one_node;

whereas the C++ FE has:

		      if (t == error_mark_node)
			{
			  remove = true;
			  break;
			}

However, I have removed the clause as suggested.

> > diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
> > index 8c05936..2598433 100644
> > --- a/gcc/cp/semantics.c
> > +++ b/gcc/cp/semantics.c
> > @@ -5532,6 +5532,41 @@ finish_omp_declare_simd_methods (tree t)
> >       }
> >   }
> >
> > +/* Adjust sink depend clause to take into account pointer offsets.  */
> > +
> > +static void
> > +cp_finish_omp_clause_depend_sink (tree sink_clause)
> > +{
> > +  tree t = OMP_CLAUSE_DECL (sink_clause);
> > +  gcc_assert (TREE_CODE (t) == TREE_LIST);
> > +
> > +  /* Make sure we don't adjust things twice for templates.  */
> > +  if (processing_template_decl)
> > +    return;
> > +
> > +  for (; t; t = TREE_CHAIN (t))
> > +    {
> > +      tree decl = TREE_VALUE (t);
> > +      if (TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE)
> > +	{
> > +	  tree offset = TREE_PURPOSE (t);
> > +	  bool neg = wi::neg_p ((wide_int) offset);
> > +	  offset = fold_unary (ABS_EXPR, TREE_TYPE (offset), offset);
> > +	  decl = mark_rvalue_use (decl);
> > +	  decl = convert_from_reference (decl);
> > +	  tree t2 = pointer_int_sum (OMP_CLAUSE_LOCATION (sink_clause),
> > +				     neg ? MINUS_EXPR : PLUS_EXPR,
> > +				     decl, offset);
> > +	  t2 = fold_build2_loc (OMP_CLAUSE_LOCATION (sink_clause),
> > +				MINUS_EXPR, sizetype, t2,
> > +				decl);
> > +	  if (t2 == error_mark_node)
> > +	    t2 = size_one_node; // ??
>
> Likewise.  But then the question is if it is worth to handle
> this in a separate routine, rather in the finish_omp_clauses function.

I still like the separate function. :)

> > +	  TREE_PURPOSE (t) = t2;
> > +	}
> > +    }
> > +}
>
>
> > +
> >   /* For all elements of CLAUSES, validate them vs OpenMP constraints.
> >      Remove any elements from the list that are invalid.  */
> >
> > @@ -6150,7 +6185,7 @@ finish_omp_clauses (tree clauses, bool allow_fields, bool declare_simd)
> >   	    }
> >   	  if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
> >   	    {
> > -	      gcc_assert (TREE_CODE (t) == TREE_LIST);
> > +	      cp_finish_omp_clause_depend_sink (c);
> >   	      break;
> >   	    }
> >   	  if (TREE_CODE (t) == TREE_LIST)
> > diff --git a/gcc/testsuite/c-c++-common/gomp/sink-4.c b/gcc/testsuite/c-c++-common/gomp/sink-4.c
> > new file mode 100644
> > index 0000000..2872404
> > --- /dev/null
> > +++ b/gcc/testsuite/c-c++-common/gomp/sink-4.c
> > @@ -0,0 +1,27 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-fopenmp -fdump-tree-gimple" } */
> > +
> > +/* Test that we adjust pointer offsets for sink variables
> > +   correctly.  */
> > +
> > +typedef struct {
> > +    char stuff[400];
> > +} foo;
> > +
> > +foo *p, *q;
> > +
> > +void
> > +funk ()
> > +{
> > +  int i,j;
>
> Unused vars?  Also, I'd suggest just pass foo *beg, foo *end
> arguments.

ughh, you and your politically correct testcases ;-).  Fixed.

How about the attached?

Attachment: curr
Description: Text document