Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-14 Thread Aldy Hernandez 

On 07/13/2015 06:56 AM, Jakub Jelinek wrote:

On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:


Everything addressed except this, which I'll address as a follow-up:


If you want to spend time on something still in the FE, it would be nice to
resolve the C++ iteration var issue (i.e. increase OMP_FOR number of
arguments, so that it could have yet another (optional) vector, say
OMP_FOR_ORIG_DECLS.  If that vector would be NULL, the gimplifier would
assume that all the decls in OMP_FOR_INIT are the ones present in the
source, if it would be present, you'd use them for the variable checking
instead of the ones from OMP_FOR_INIT (but, replace them with the
decls from OMP_FOR_INIT after the checking).

There is another issue - if some iterator var has pointer type, supposedly
we want somewhere in the FEs already multiply it by the size of what they
point to (and convert to sizetype).  For C FE, it can be done already during
parsing, we should know the type of the iterator var already at that point,
for C++ FE it needs to be done only in finish_omp_clauses if
!processing_template_decl, because in templates we might not know the type.


Tested on x86-64 Linux.

Ok for branch?

commit 6ca9b034b05e3106eac6c7b9c6dc1885e1569320
Author: Aldy Hernandez al...@redhat.com
Date:   Wed Jun 24 15:08:49 2015 -0700

* coretypes.h (struct gomp_ordered): Define.
* gimple-pretty-print.c (dump_gimple_omp_block): Do not handle
GIMPLE_OMP_ORDERED.
(dump_gimple_omp_ordered): New.
(pp_gimple_stmt_1): Handle GIMPLE_OMP_ORDERED.
* gimple-walk.c (walk_gimple_op): Same.
* gimple.c (gimple_build_omp_ordered): Generate gimple_ordered.
* gimple.def (GIMPLE_OMP_ORDERED): Add clauses.
* gimple.h (struct gomp_ordered): New.
(is_a_helpergomp_ordered *::test): New.
(is_a_helpergomp_ordered *): New.
(gimple_omp_ordered_clauses): New.
(gimple_omp_ordered_clauses_ptr): New.
(gimple_omp_ordered_set_clauses): New.
* gimplify.c (struct gimplify_omp_ctx): Add iter_vars field.
(delete_omp_context): Release iter_vars.
(gimplify_scan_omp_clauses): Handle OMP_CLAUSE_DEPEND_{SINK,SOURCE}.
(gimplify_omp_for): Initialize iter_vars.
(gimplify_expr): Handle OMP_ORDERED clauses.
* omp-low.c (check_omp_nesting_restrictions): Type check
OMP_CLAUSE_DEPEND_{KIND,SOURCE}.
(lower_depend_clauses): Add cases for OMP_CLAUSE_DEPEND_{SOURCE,SINK}.
* tree-inline.c (remap_gimple_stmt): Call gimple_build_omp_ordered
with clauses.
* tree-pretty-print.c (dump_omp_clause): Handle
OMP_CLAUSE_DEPEND_SINK.
c/
* c-parser.c (c_parser_omp_clause_depend_sink): New.
(c_parser_omp_clause_depend): Call it.
* c-typeck.c (c_finish_omp_clauses): Handle
OMP_CLAUSE_DEPEND_SINK.
cp/

* parser.c (cp_parser_omp_clause_depend_sink): New.
(cp_parser_omp_clause_depend): Call it.
* pt.c (tsubst_omp_clause_decl): Handle OMP_CLAUSE_DEPEND_KIND.
(tsubst_expr): Handle OMP_ORDERED_CLAUSES.
* semantics.c (finish_omp_clauses): Handle OMP_CLAUSE_DEPEND_KIND.

diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index cd3bd5a..211857a 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -11701,6 +11701,95 @@ c_parser_omp_clause_simdlen (c_parser *parser, tree 
list)
   return c;
 }
 
+/* OpenMP 4.1:
+   vec:
+ identifier [+/- integer]
+ vec , identifier [+/- integer]
+*/
+
+static tree
+c_parser_omp_clause_depend_sink (c_parser *parser, location_t clause_loc,
+tree list)
+{
+  tree vec = NULL;
+  if (c_parser_next_token_is_not (parser, CPP_NAME)
+  || c_parser_peek_token (parser)-id_kind != C_ID_ID)
+{
+  c_parser_error (parser, expected identifier);
+  return list;
+}
+
+  while (c_parser_next_token_is (parser, CPP_NAME)
+ c_parser_peek_token (parser)-id_kind == C_ID_ID)
+{
+  tree t = lookup_name (c_parser_peek_token (parser)-value);
+  tree addend = NULL;
+
+  if (t == NULL_TREE)
+   {
+ undeclared_variable (c_parser_peek_token (parser)-location,
+  c_parser_peek_token (parser)-value);
+ t = error_mark_node;
+   }
+
+  c_parser_consume_token (parser);
+
+  if (t != error_mark_node)
+   {
+ bool neg;
+
+ if (c_parser_next_token_is (parser, CPP_MINUS))
+   neg = true;
+ else if (c_parser_next_token_is (parser, CPP_PLUS))
+   neg = false;
+ else
+   {
+ addend = integer_zero_node;
+ goto add_to_vector;
+   }
+ c_parser_consume_token (parser);
+
+ if (c_parser_next_token_is_not (parser, CPP_NUMBER))
+   {
+ c_parser_error (parser, expected integer);
+ return list;
+   }
+
+ addend = c_parser_peek_token

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-14 Thread Jakub Jelinek 
On Tue, Jul 14, 2015 at 07:06:52AM -0700, Aldy Hernandez wrote:
 On 07/13/2015 06:56 AM, Jakub Jelinek wrote:
 On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:
 
 Everything addressed except this, which I'll address as a follow-up:
 
 If you want to spend time on something still in the FE, it would be nice to
 resolve the C++ iteration var issue (i.e. increase OMP_FOR number of
 arguments, so that it could have yet another (optional) vector, say
 OMP_FOR_ORIG_DECLS.  If that vector would be NULL, the gimplifier would
 assume that all the decls in OMP_FOR_INIT are the ones present in the
 source, if it would be present, you'd use them for the variable checking
 instead of the ones from OMP_FOR_INIT (but, replace them with the
 decls from OMP_FOR_INIT after the checking).
 
 There is another issue - if some iterator var has pointer type, supposedly
 we want somewhere in the FEs already multiply it by the size of what they
 point to (and convert to sizetype).  For C FE, it can be done already during
 parsing, we should know the type of the iterator var already at that point,
 for C++ FE it needs to be done only in finish_omp_clauses if
 !processing_template_decl, because in templates we might not know the type.
 
 Tested on x86-64 Linux.
 
 Ok for branch?

Can you please fix:
Blocks of 8 spaces should be replaced with tabs.
248:+   (parser, identifier,
579:+ stmt-code == GIMPLE_OMP_ORDERED.  */
798:+   (gimple_omp_for_clauses (octx-stmt),
914:+for (k = 0; k  o; k++)
915:+  {
932:+bar (i, j, 0);
955:+bar (i, j, 0);
973:+  (s1,
(in the patch on lines starting with + replace sequences of 8 spaces with
tabs)?

Ok with that, thanks.

Jakub

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-13 Thread Jakub Jelinek 
On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:
 It looks like the C++ bits are quite similar to the C ones.  AFAICT, only
 numbers are allowed for the sink offsets, so no C++ iterators, which would
 likely complicate matters.  If they are eventually allowed, we can implement
 them as a follow up.
 
 The attached patch addresses all your concerns plus includes the C++
 implementation.  The included test passes for both languages.
 
 I can work on Fortran next if you'd like.

Please leave Fortran unresolved for now, we'll see in Autumn if we have time
for Fortran OpenMP 4.1 support, or not, there is also the possibility to
handle it like in 4.9 - 4.9.0 came with just C/C++ OpenMP 4.0 support
(and Fortran only OpenMP 3.1 support) and 4.9.1 added Fortran OpenMP 4.0 
support.

Please write ChangeLog entries and commit them into */ChangeLog.gomp files.

 +   if (c_parser_next_token_is_not (parser, CPP_NUMBER))
 + {
 +   c_parser_error (parser, expected %integer%);

I think % and % here

 +   return list;
 + }
 +
 +   addend = c_parser_peek_token (parser)-value;
 +   if (TREE_CODE (addend) != INTEGER_CST)
 + {
 +   c_parser_error (parser, expected %integer%);

and here aren't appropriate here, you don't expect integer as a keyword,
but some integer...

On the C++ FE side, please also try a testcase in g++.dg/gomp/ where
the ordered(n) loop with #pragma omp ordered depend({source,sink}) will be
in a template, to make sure pt.c does the right thing with it.

 +   if (cp_lexer_next_token_is_not (parser-lexer, CPP_NUMBER))
 + {
 +   cp_parser_error (parser, expected %integer%);
 +   return list;
 + }
 +
 +   addend = cp_lexer_peek_token (parser-lexer)-u.value;
 +   if (TREE_CODE (addend) != INTEGER_CST)
 + {
 +   cp_parser_error (parser, expected %integer%);

See above.

 @@ -365,6 +367,8 @@ new_omp_context (enum omp_region_type region_type)
  
c = XCNEW (struct gimplify_omp_ctx);
c-outer_context = gimplify_omp_ctxp;
 +  c-iter_vars.safe_push(0);
 +  c-iter_vars.pop();

As mentioned, please leave this out.

 @@ -8982,7 +8997,36 @@ gimplify_expr (tree *expr_p, gimple_seq *pre_p, 
 gimple_seq *post_p,
   }
   break;
 case OMP_ORDERED:
 - g = gimple_build_omp_ordered (body);
 + if (gimplify_omp_ctxp)
 +   for (tree c = OMP_ORDERED_CLAUSES (*expr_p);
 +c; c = OMP_CLAUSE_CHAIN (c))
 + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
 +  OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
 +   {
 + unsigned int n = 0;
 + bool fail = false;
 + for (tree decls = OMP_CLAUSE_DECL (c);
 +  decls  TREE_CODE (decls) == TREE_LIST;
 +  decls = TREE_CHAIN (decls), ++n)
 +   if (n  gimplify_omp_ctxp-iter_vars.length ()
 +TREE_VALUE (decls)
 +   != gimplify_omp_ctxp-iter_vars[n])
 + {
 +   error_at (OMP_CLAUSE_LOCATION (c),
 + variable %qE is not an iteration 
 + variable, TREE_VALUE (decls));

I think this error message will be confusing to users, if they write
#pragma omp for ordered(3)
for (int i = 0; i  10; i++)
for (int j = 0; j  10; j++)
for (int k = 0; k  10; k++)
{
#pragma omp ordered depend(sink:k-1, j+2, i-3)
#pragma omp ordered depend(source)
}
because then it will complain that k and i are not iteration
variables, when they in fact are, just in wrong order.

I believe our diagnostics doesn't have support for ngettext style
of diagnostic messages (1st vs. 2nd, 3rd, 4th ...); I wonder if
saying variable %qE is not an iteration variable of outermost loop %d, expected 
%qE,
TREE_VALUE (decls), n + 1, gimplify_omp_ctxp-iter_vars[n]
wouldn't be better or something similar.

 +   fail = true;
 + }
 + /* Avoid being too redundant.  */
 + if (!fail
 +  n != gimplify_omp_ctxp-iter_vars.length ())
 +   error_at (OMP_CLAUSE_LOCATION (c),
 +  number of variables in depend(sink) clause 
 +  does not match number of iteration variables);
 +   }
 +
 + g = gimple_build_omp_ordered (body,
 +   OMP_ORDERED_CLAUSES (*expr_p));
   break;
 case OMP_CRITICAL:
   gimplify_scan_omp_clauses (OMP_CRITICAL_CLAUSES (*expr_p),
 diff --git a/gcc/omp-low.c b/gcc/omp-low.c
 index 83677ea..3dec095 100644
 --- a/gcc/omp-low.c
 +++ b/gcc/omp-low.c
 @@ -2996,6 +2996,8 @@ scan_omp_teams (gomp_teams *stmt,

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-13 Thread Aldy Hernandez 

On 07/13/2015 06:56 AM, Jakub Jelinek wrote:

On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:



On the C++ FE side, please also try a testcase in g++.dg/gomp/ where
the ordered(n) loop with #pragma omp ordered depend({source,sink}) will be
in a template, to make sure pt.c does the right thing with it.


I assume you mean something like:

void bar (int, int, int);

templatetypename T
T baz (T arg)
{
  int i, j, k;
#pragma omp parallel for ordered(2)
  for (i=0; i  100; ++i)
for (j=0; j  100; ++j)
  {
#pragma omp ordered depend(sink:i-3,j)
bar (i, j, 0);
  }
  return arg;
}

int main()
{
  return bazint(5);
}

??

Also, was this supposed to work?:

templateint N
int foo()
{
  int i, j, k;
#pragma omp parallel for ordered(N)
  for (i=0; i  100; ++i)
for (j=0; j  100; ++j)
  {
extern void bark();
bark();
  }
}

The above was broken before I arrived.

And if this last example is supposed to work, I should probably address 
the same thing for sink offsets.



If you want to spend time on something still in the FE, it would be nice to
resolve the C++ iteration var issue (i.e. increase OMP_FOR number of
arguments, so that it could have yet another (optional) vector, say
OMP_FOR_ORIG_DECLS.  If that vector would be NULL, the gimplifier would
assume that all the decls in OMP_FOR_INIT are the ones present in the
source, if it would be present, you'd use them for the variable checking
instead of the ones from OMP_FOR_INIT (but, replace them with the
decls from OMP_FOR_INIT after the checking).

There is another issue - if some iterator var has pointer type, supposedly
we want somewhere in the FEs already multiply it by the size of what they
point to (and convert to sizetype).  For C FE, it can be done already during
parsing, we should know the type of the iterator var already at that point,
for C++ FE it needs to be done only in finish_omp_clauses if
!processing_template_decl, because in templates we might not know the type.


Sure.  As follow-ups?

Aldy

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-13 Thread Jakub Jelinek 
On Mon, Jul 13, 2015 at 10:11:35AM -0700, Aldy Hernandez wrote:
 On 07/13/2015 06:56 AM, Jakub Jelinek wrote:
 On Sat, Jul 11, 2015 at 11:35:36AM -0700, Aldy Hernandez wrote:
 
 On the C++ FE side, please also try a testcase in g++.dg/gomp/ where
 the ordered(n) loop with #pragma omp ordered depend({source,sink}) will be
 in a template, to make sure pt.c does the right thing with it.
 
 I assume you mean something like:
 
 void bar (int, int, int);
 
 templatetypename T
 T baz (T arg)
 {
   int i, j, k;

Yeah, or even better T i, j, k;
As you don't use the argument, it can be just
templatetypename T
void baz ()
{

 Also, was this supposed to work?:
 
 templateint N
 int foo()
 {
   int i, j, k;
 #pragma omp parallel for ordered(N)

It is not 100% clear, but we don't support collapse(N)
either when N is a template parameter, as it affects
parsing of the code, we require that it is a non-dependent
constant expression.

Whether depend(sink:) should allow template parameters
depends on whether it will be required to be integer constant
or integer constant expression, right now it should be the former.

 If you want to spend time on something still in the FE, it would be nice to
 resolve the C++ iteration var issue (i.e. increase OMP_FOR number of
 arguments, so that it could have yet another (optional) vector, say
 OMP_FOR_ORIG_DECLS.  If that vector would be NULL, the gimplifier would
 assume that all the decls in OMP_FOR_INIT are the ones present in the
 source, if it would be present, you'd use them for the variable checking
 instead of the ones from OMP_FOR_INIT (but, replace them with the
 decls from OMP_FOR_INIT after the checking).
 
 There is another issue - if some iterator var has pointer type, supposedly
 we want somewhere in the FEs already multiply it by the size of what they
 point to (and convert to sizetype).  For C FE, it can be done already during
 parsing, we should know the type of the iterator var already at that point,
 for C++ FE it needs to be done only in finish_omp_clauses if
 !processing_template_decl, because in templates we might not know the type.
 
 Sure.  As follow-ups?

Of course.

Jakub

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-11 Thread Aldy Hernandez 
It looks like the C++ bits are quite similar to the C ones.  AFAICT, 
only numbers are allowed for the sink offsets, so no C++ iterators, 
which would likely complicate matters.  If they are eventually allowed, 
we can implement them as a follow up.


The attached patch addresses all your concerns plus includes the C++ 
implementation.  The included test passes for both languages.


I can work on Fortran next if you'd like.

Aldy
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index cd3bd5a..50edaf6 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -11701,6 +11701,95 @@ c_parser_omp_clause_simdlen (c_parser *parser, tree 
list)
   return c;
 }
 
+/* OpenMP 4.1:
+   vec:
+ identifier [+/- integer]
+ vec , identifier [+/- integer]
+*/
+
+static tree
+c_parser_omp_clause_depend_sink (c_parser *parser, location_t clause_loc,
+tree list)
+{
+  tree vec = NULL;
+  if (c_parser_next_token_is_not (parser, CPP_NAME)
+  || c_parser_peek_token (parser)-id_kind != C_ID_ID)
+{
+  c_parser_error (parser, expected identifier);
+  return list;
+}
+
+  while (c_parser_next_token_is (parser, CPP_NAME)
+ c_parser_peek_token (parser)-id_kind == C_ID_ID)
+{
+  tree t = lookup_name (c_parser_peek_token (parser)-value);
+  tree addend = NULL;
+
+  if (t == NULL_TREE)
+   {
+ undeclared_variable (c_parser_peek_token (parser)-location,
+  c_parser_peek_token (parser)-value);
+ t = error_mark_node;
+   }
+
+  c_parser_consume_token (parser);
+
+  if (t != error_mark_node)
+   {
+ bool neg;
+
+ if (c_parser_next_token_is (parser, CPP_MINUS))
+   neg = true;
+ else if (c_parser_next_token_is (parser, CPP_PLUS))
+   neg = false;
+ else
+   {
+ addend = integer_zero_node;
+ goto add_to_vector;
+   }
+ c_parser_consume_token (parser);
+
+ if (c_parser_next_token_is_not (parser, CPP_NUMBER))
+   {
+ c_parser_error (parser, expected %integer%);
+ return list;
+   }
+
+ addend = c_parser_peek_token (parser)-value;
+ if (TREE_CODE (addend) != INTEGER_CST)
+   {
+ c_parser_error (parser, expected %integer%);
+ return list;
+   }
+ if (neg)
+   {
+ bool overflow;
+ wide_int offset = wi::neg (addend, overflow);
+ addend = wide_int_to_tree (TREE_TYPE (addend), offset);
+ if (overflow)
+   warning_at (c_parser_peek_token (parser)-location,
+   OPT_Woverflow,
+   overflow in implicit constant conversion);
+   }
+ c_parser_consume_token (parser);
+
+   add_to_vector:
+ vec = tree_cons (addend, t, vec);
+
+ if (c_parser_next_token_is_not (parser, CPP_COMMA))
+   break;
+
+ c_parser_consume_token (parser);
+   }
+}
+
+  tree u = build_omp_clause (clause_loc, OMP_CLAUSE_DEPEND);
+  OMP_CLAUSE_DEPEND_KIND (u) = OMP_CLAUSE_DEPEND_SINK;
+  OMP_CLAUSE_DECL (u) = nreverse (vec);
+  OMP_CLAUSE_CHAIN (u) = list;
+  return u;
+}
+
 /* OpenMP 4.0:
depend ( depend-kind: variable-list )
 
@@ -11708,10 +11797,9 @@ c_parser_omp_clause_simdlen (c_parser *parser, tree 
list)
  in | out | inout
 
OpenMP 4.1:
-   depend ( depend-loop-kind [ : vec ] )
+   depend ( source )
 
-   depend-loop-kind:
- source | sink  */
+   depend ( sink  : vec )  */
 
 static tree
 c_parser_omp_clause_depend (c_parser *parser, tree list)
@@ -11754,16 +11842,19 @@ c_parser_omp_clause_depend (c_parser *parser, tree 
list)
   return c;
 }
 
-  /* FIXME: Handle OMP_CLAUSE_DEPEND_SINK.  */
-
   if (!c_parser_require (parser, CPP_COLON, expected %:%))
 goto resync_fail;
 
-  nl = c_parser_omp_variable_list (parser, clause_loc,
-  OMP_CLAUSE_DEPEND, list);
+  if (kind == OMP_CLAUSE_DEPEND_SINK)
+nl = c_parser_omp_clause_depend_sink (parser, clause_loc, list);
+  else
+{
+  nl = c_parser_omp_variable_list (parser, clause_loc,
+  OMP_CLAUSE_DEPEND, list);
 
-  for (c = nl; c != list; c = OMP_CLAUSE_CHAIN (c))
-OMP_CLAUSE_DEPEND_KIND (c) = kind;
+  for (c = nl; c != list; c = OMP_CLAUSE_CHAIN (c))
+   OMP_CLAUSE_DEPEND_KIND (c) = kind;
+}
 
   c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, expected %)%);
   return nl;
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index 469cd88..0b332e8 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool declare_simd)
  == OMP_CLAUSE_DEPEND_SOURCE);
  break;
}
+ if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
+   {
+ gcc_assert (TREE_CODE (t) ==

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-11 Thread Aldy Hernandez 



+  c-iter_vars.safe_push(0);
+  c-iter_vars.pop();


Whoops.  Consider this removed.  This was left over from some tests I 
was doing with the vector.


Aldy

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-10 Thread Aldy Hernandez 

On 07/09/2015 11:53 AM, Jakub Jelinek wrote:

Hi!

On Thu, Jul 09, 2015 at 11:24:44AM -0700, Aldy Hernandez wrote:

Thanks for working on it.


+ wide_int offset = wi::neg (addend, overflow);
+ addend = wide_int_to_tree (TREE_TYPE (addend), offset);
+ if (overflow)
+   warning_at (c_parser_peek_token (parser)-location,
+   OPT_Woverflow,
+   possible overflow in %depend(sink)% offset);


possible overflow looks weird.  Shouldn't it complain the same
as it does if you do:
int c = - (-2147483648);


Done.


?


--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool declare_simd)
  == OMP_CLAUSE_DEPEND_SOURCE);
  break;
}
+ if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
+   {
+ gcc_assert (TREE_CODE (t) == TREE_LIST);
+ break;
+   }
  if (TREE_CODE (t) == TREE_LIST)
{
  if (handle_omp_array_sections (c))


Won't this ICE if somebody uses depend(sink:) ? or depend(sink:.::) or
similar garbage?  Make sure you don't create OMP_CLAUSE_DEPEND in that
case.


I've fixed the parser to avoid creating such clause.




diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
index f0e2c67..ba79977 100644
--- a/gcc/gimple-walk.c
+++ b/gcc/gimple-walk.c
@@ -327,6 +327,10 @@ walk_gimple_op (gimple stmt, walk_tree_fn callback_op,
}
break;

+case GIMPLE_OMP_ORDERED:
+  /* Ignore clauses.  */
+  break;
+


I'm not convinced you don't want to walk the clauses.


Ok, I've done so.

Note that the OMP_CLAUSE_DECL will contain a TREE_LIST whose 
TREE_PURPOSE had the variable.  I noticed that walking TREE_LIST's just 
walks the TREE_VALUE, not the TREE_PURPOSE:


case TREE_LIST:
  WALK_SUBTREE (TREE_VALUE (*tp));
  WALK_SUBTREE_TAIL (TREE_CHAIN (*tp));
  break;


So, I changed the layout of the OMP_CLAUSE_DECL TREE_LIST to have the 
variable in the TREE_VALUE.  The TREE_PURPOSE will contain the lone 
integer, which shouldn't need to be walked.  However, if later (C++ 
iterators??) we have a TREE_PURPOSE that needs to be walked we will have 
to change the walker or the layout.





diff --git a/gcc/gimple.h b/gcc/gimple.h
index 6057ea0..e33fe1e 100644
--- a/gcc/gimple.h
+++ b/gcc/gimple.h
@@ -527,6 +527,17 @@ struct GTY((tag(GSS_OMP_CRITICAL)))
tree name;
  };

+/* GIMPLE_OMP_ORDERED */
+
+struct GTY((tag(GSS_OMP_ORDERED)))
+  gomp_ordered : public gimple_statement_omp
+{
+  /* [ WORD 1-7 ] : base class */
+
+  /* [ WORD 8 ]  */
+  tree clauses;
+};


I would have expected to use
struct GTY((tag(GSS_OMP_SINGLE_LAYOUT)))
   gomp_ordered : public gimple_statement_omp_single_layout
{
 /* No extra fields; adds invariant:
  stmt-code == GIMPLE_OMP_ORDERED.  */
};
instead (like gomp_single, gomp_teams, ...).


Oh, neat.  I missed that.  Fixed.




@@ -149,6 +149,9 @@ struct gimplify_omp_ctx
struct gimplify_omp_ctx *outer_context;
splay_tree variables;
hash_settree *privatized_types;
+  /* Iteration variables in an OMP_FOR.  */
+  tree *iter_vars;
+  int niter_vars;


Wonder if it wouldn't be better to use a vectree instead.
Then the size would be there as vec_length.


Done.




@@ -8169,6 +8185,19 @@ gimplify_transaction (tree *expr_p, gimple_seq *pre_p)
return GS_ALL_DONE;
  }

+/* Verify the validity of the depend(sink:...) variable VAR.
+   Return TRUE if everything is OK, otherwise return FALSE.  */
+
+static bool
+verify_sink_var (location_t loc, tree var)
+{
+  for (int i = 0; i  gimplify_omp_ctxp-niter_vars; ++i)
+if (var == gimplify_omp_ctxp-iter_vars[i])
+  return true;
+  error_at (loc, variable %qE is not an iteration variable, var);
+  return false;


I believe what we want to verify is that ith variable in the OMP_CLAUSE_DECL
vector is iter_vars[i], so not just some random permutation etc.


Fixed.




@@ -3216,7 +3218,51 @@ check_omp_nesting_restrictions (gimple stmt, omp_context 
*ctx)
break;
  }
break;
+case GIMPLE_OMP_TASK:
+  for (c = gimple_omp_task_clauses (stmt); c; c = OMP_CLAUSE_CHAIN (c))
+   if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
+(OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
+   || OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK))
+ {
+   error_at (OMP_CLAUSE_LOCATION (c),
+ depend(%s) is only available in 'omp ordered',


Please avoid using ' in diagnostics, it should be %omp ordered% instead.


Fixed.




+ OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
+ ? source : sink);
+   return false;
+ }
+  break;


This will eventually be needed also for GIMPLE_OMP_TARGET and
GIMPLE_OMP_ENTER/EXIT_DATA.  But as that isn't really supported right now,
can wait.


I

Re: [gomp4.1] depend(sink) and depend(source) parsing for C

2015-07-09 Thread Jakub Jelinek 
Hi!

On Thu, Jul 09, 2015 at 11:24:44AM -0700, Aldy Hernandez wrote:

Thanks for working on it.

 +   wide_int offset = wi::neg (addend, overflow);
 +   addend = wide_int_to_tree (TREE_TYPE (addend), offset);
 +   if (overflow)
 + warning_at (c_parser_peek_token (parser)-location,
 + OPT_Woverflow,
 + possible overflow in %depend(sink)% offset);

possible overflow looks weird.  Shouldn't it complain the same
as it does if you do:
int c = - (-2147483648);
?

 --- a/gcc/c/c-typeck.c
 +++ b/gcc/c/c-typeck.c
 @@ -12489,6 +12489,11 @@ c_finish_omp_clauses (tree clauses, bool 
 declare_simd)
 == OMP_CLAUSE_DEPEND_SOURCE);
 break;
   }
 +   if (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK)
 + {
 +   gcc_assert (TREE_CODE (t) == TREE_LIST);
 +   break;
 + }
 if (TREE_CODE (t) == TREE_LIST)
   {
 if (handle_omp_array_sections (c))

Won't this ICE if somebody uses depend(sink:) ? or depend(sink:.::) or
similar garbage?  Make sure you don't create OMP_CLAUSE_DEPEND in that
case.

 diff --git a/gcc/gimple-walk.c b/gcc/gimple-walk.c
 index f0e2c67..ba79977 100644
 --- a/gcc/gimple-walk.c
 +++ b/gcc/gimple-walk.c
 @@ -327,6 +327,10 @@ walk_gimple_op (gimple stmt, walk_tree_fn callback_op,
}
break;
  
 +case GIMPLE_OMP_ORDERED:
 +  /* Ignore clauses.  */
 +  break;
 +

I'm not convinced you don't want to walk the clauses.

 diff --git a/gcc/gimple.h b/gcc/gimple.h
 index 6057ea0..e33fe1e 100644
 --- a/gcc/gimple.h
 +++ b/gcc/gimple.h
 @@ -527,6 +527,17 @@ struct GTY((tag(GSS_OMP_CRITICAL)))
tree name;
  };
  
 +/* GIMPLE_OMP_ORDERED */
 +
 +struct GTY((tag(GSS_OMP_ORDERED)))
 +  gomp_ordered : public gimple_statement_omp
 +{
 +  /* [ WORD 1-7 ] : base class */
 +
 +  /* [ WORD 8 ]  */
 +  tree clauses;
 +};

I would have expected to use 
struct GTY((tag(GSS_OMP_SINGLE_LAYOUT)))
  gomp_ordered : public gimple_statement_omp_single_layout
{
/* No extra fields; adds invariant:
 stmt-code == GIMPLE_OMP_ORDERED.  */
};
instead (like gomp_single, gomp_teams, ...).

 @@ -149,6 +149,9 @@ struct gimplify_omp_ctx
struct gimplify_omp_ctx *outer_context;
splay_tree variables;
hash_settree *privatized_types;
 +  /* Iteration variables in an OMP_FOR.  */
 +  tree *iter_vars;
 +  int niter_vars;

Wonder if it wouldn't be better to use a vectree instead.
Then the size would be there as vec_length.

 @@ -8169,6 +8185,19 @@ gimplify_transaction (tree *expr_p, gimple_seq *pre_p)
return GS_ALL_DONE;
  }
  
 +/* Verify the validity of the depend(sink:...) variable VAR.
 +   Return TRUE if everything is OK, otherwise return FALSE.  */
 +
 +static bool
 +verify_sink_var (location_t loc, tree var)
 +{
 +  for (int i = 0; i  gimplify_omp_ctxp-niter_vars; ++i)
 +if (var == gimplify_omp_ctxp-iter_vars[i])
 +  return true;
 +  error_at (loc, variable %qE is not an iteration variable, var);
 +  return false;

I believe what we want to verify is that ith variable in the OMP_CLAUSE_DECL
vector is iter_vars[i], so not just some random permutation etc.

 @@ -3216,7 +3218,51 @@ check_omp_nesting_restrictions (gimple stmt, 
 omp_context *ctx)
   break;
 }
break;
 +case GIMPLE_OMP_TASK:
 +  for (c = gimple_omp_task_clauses (stmt); c; c = OMP_CLAUSE_CHAIN (c))
 + if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_DEPEND
 +  (OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
 + || OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SINK))
 +   {
 + error_at (OMP_CLAUSE_LOCATION (c),
 +   depend(%s) is only available in 'omp ordered',

Please avoid using ' in diagnostics, it should be %omp ordered% instead.

 +   OMP_CLAUSE_DEPEND_KIND (c) == OMP_CLAUSE_DEPEND_SOURCE
 +   ? source : sink);
 + return false;
 +   }
 +  break;

This will eventually be needed also for GIMPLE_OMP_TARGET and
GIMPLE_OMP_ENTER/EXIT_DATA.  But as that isn't really supported right now,
can wait.

  case GIMPLE_OMP_ORDERED:
 +  for (c = gimple_omp_ordered_clauses (as_a gomp_ordered * (stmt));
 +c; c = OMP_CLAUSE_CHAIN (c))
 + {
 +   enum omp_clause_depend_kind kind = OMP_CLAUSE_DEPEND_KIND (c);
 +   if (kind == OMP_CLAUSE_DEPEND_SOURCE
 +   || kind == OMP_CLAUSE_DEPEND_SINK)
 + {
 +   bool have_ordered = false;
 +   /* Look for containing ordered(N) loop.  */
 +   for (omp_context *ctx_ = ctx; ctx_; ctx_ = ctx_-outer)

Please use octx or something similar, I don't like the trailing _ ;)

 +   if (!have_ordered)
 + {
 +   error_at (OMP_CLAUSE_LOCATION (c),
 + depend clause is not within an ordered loop);

Not within is not the right OpenMP term, the requirement is that it must
be