[gomp] Fix a couple of global var handling bugs, add firstprivate+lastprivate barrier (PR c++/26943) (take 2)

[Jakub Jelinek <jakub at redhat dot com>]

On Mon, May 01, 2006 at 12:47:03PM -0700, Richard Henderson wrote:
> On Mon, May 01, 2006 at 01:06:11PM -0400, Jakub Jelinek wrote:
> > +	  /* Global variables don't need to be copied,
> > +	     the receiver side will use them directly.
> > +	     For nested contexts we unfortunately don't know
> > +	     yet whether it will be a global variable or not.  */
> > +	  if (! ctx->is_nested && is_global_var (decl))
> > +	    break;
> >  	  if (! TREE_READONLY (decl)
> 
> What's the test case for this?  Just testing is_nested doesn't
> seem quite right.  I should think that we ought to be able to
> tell, at some point whether it is or isn't.  Perhaps a variant
> of lookup_decl_in_outer_ctx?

The following patch seems to work.
The reason why maybe_lookup_decl_in_outer_ctx can't do just
return ctx->is_nested ? lookup_decl_in_outer_ctx (decl, ctx) : decl;
are SHARED clauses in nested context, where the outer context
doesn't reference the var at all and defaults there to the SHARED
data sharing.  In that case the variable is not considered to be ever
touched in the outer context and thus lookup_decl_in_outer_ctx
ICEs (e.g. on testsuite/libgomp.c/pr26943-3.c).

For for trunk?

2006-05-02  Jakub Jelinek  <jakub@redhat.com>

	PR c++/26943
	* omp-low.c (maybe_lookup_decl_in_outer_ctx): New function.
	(build_outer_var_ref): Use maybe_lookup_decl_in_outer_ctx
	to find if var will be a global variable even in the nested context.
	(omp_copy_decl): Only check for global variable at the end, it might
	be overridden in outer contexts.
	(scan_sharing_clauses): For global variables don't create a field.
	(lower_rec_input_clauses): Do nothing for global shared variables.
	Emit a barrier at the end of ILIST if there were any decls in both
	firstprivate and lastprivate clauses.
	(lower_send_clauses): Do nothing for global variables except for
	COPYIN.

	* testsuite/libgomp.c/pr26943-1.c: New test.
	* testsuite/libgomp.c/pr26943-2.c: New test.
	* testsuite/libgomp.c/pr26943-3.c: New test.
	* testsuite/libgomp.c/pr26943-4.c: New test.
	* testsuite/libgomp.c++/pr27337.C: Remove barrier.
	* testsuite/libgomp.c++/pr26943.C: New test.

--- gcc/omp-low.c.jj	2006-04-28 13:29:49.000000000 +0200
+++ gcc/omp-low.c	2006-05-02 15:33:49.000000000 +0200
@@ -112,6 +112,8 @@ struct omp_region *root_omp_region;
 
 static void scan_omp (tree *, omp_context *);
 static void lower_omp (tree *, omp_context *);
+static tree lookup_decl_in_outer_ctx (tree, omp_context *);
+static tree maybe_lookup_decl_in_outer_ctx (tree, omp_context *);
 
 /* Find an OpenMP clause of type KIND within CLAUSES.  */
 
@@ -560,7 +562,7 @@ build_outer_var_ref (tree var, omp_conte
 {
   tree x;
 
-  if (is_global_var (var))
+  if (is_global_var (maybe_lookup_decl_in_outer_ctx (var, ctx)))
     x = var;
   else if (is_variable_sized (var))
     {
@@ -674,9 +676,6 @@ omp_copy_decl (tree var, copy_body_data 
   omp_context *ctx = (omp_context *) cb;
   tree new_var;
 
-  if (is_global_var (var) || decl_function_context (var) != ctx->cb.src_fn)
-    return var;
-
   if (TREE_CODE (var) == LABEL_DECL)
     {
       new_var = create_artificial_label ();
@@ -695,6 +694,9 @@ omp_copy_decl (tree var, copy_body_data 
 	return new_var;
     }
 
+  if (is_global_var (var) || decl_function_context (var) != ctx->cb.src_fn)
+    return var;
+
   return error_mark_node;
 }
 
@@ -937,6 +939,10 @@ scan_sharing_clauses (tree clauses, omp_
 	  decl = OMP_CLAUSE_DECL (c);
 	  gcc_assert (!is_variable_sized (decl));
 	  by_ref = use_pointer_for_field (decl, true);
+	  /* Global variables don't need to be copied,
+	     the receiver side will use them directly.  */
+	  if (is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx)))
+	    break;
 	  if (! TREE_READONLY (decl)
 	      || TREE_ADDRESSABLE (decl)
 	      || by_ref
@@ -963,7 +969,9 @@ scan_sharing_clauses (tree clauses, omp_
 	do_private:
 	  if (is_variable_sized (decl))
 	    break;
-	  else if (is_parallel_ctx (ctx))
+	  else if (is_parallel_ctx (ctx)
+		   && ! is_global_var (maybe_lookup_decl_in_outer_ctx (decl,
+								       ctx)))
 	    {
 	      by_ref = use_pointer_for_field (decl, false);
 	      install_var_field (decl, by_ref, ctx);
@@ -1029,7 +1037,8 @@ scan_sharing_clauses (tree clauses, omp_
 
 	case OMP_CLAUSE_SHARED:
 	  decl = OMP_CLAUSE_DECL (c);
-	  fixup_remapped_decl (decl, ctx, false);
+	  if (! is_global_var (maybe_lookup_decl_in_outer_ctx (decl, ctx)))
+	    fixup_remapped_decl (decl, ctx, false);
 	  break;
 
 	case OMP_CLAUSE_COPYPRIVATE:
@@ -1415,6 +1424,23 @@ lookup_decl_in_outer_ctx (tree decl, omp
 }
 
 
+/* Similar to lookup_decl_in_outer_ctx, but return DECL if not found
+   in outer contexts.  */
+
+static tree
+maybe_lookup_decl_in_outer_ctx (tree decl, omp_context *ctx)
+{
+  tree t = NULL;
+  omp_context *up;
+
+  if (ctx->is_nested)
+    for (up = ctx->outer, t = NULL; up && t == NULL; up = up->outer)
+      t = maybe_lookup_decl (decl, up);
+
+  return t ? t : decl;
+}
+
+
 /* Construct the initialization value for reduction CLAUSE.  */
 
 tree
@@ -1493,6 +1519,7 @@ lower_rec_input_clauses (tree clauses, t
   tree_stmt_iterator diter;
   tree c, dtor, copyin_seq, x, args, ptr;
   bool copyin_by_ref = false;
+  bool lastprivate_firstprivate = false;
   int pass;
 
   *dlist = alloc_stmt_list ();
@@ -1518,14 +1545,22 @@ lower_rec_input_clauses (tree clauses, t
 		continue;
 	      break;
 	    case OMP_CLAUSE_SHARED:
+	      if (maybe_lookup_decl (OMP_CLAUSE_DECL (c), ctx) == NULL)
+		{
+		  gcc_assert (is_global_var (OMP_CLAUSE_DECL (c)));
+		  continue;
+		}
 	    case OMP_CLAUSE_FIRSTPRIVATE:
 	    case OMP_CLAUSE_COPYIN:
 	    case OMP_CLAUSE_REDUCTION:
 	      break;
 	    case OMP_CLAUSE_LASTPRIVATE:
-	      if (pass != 0
-		  && OMP_CLAUSE_LASTPRIVATE_FIRSTPRIVATE (c))
-		continue;
+	      if (OMP_CLAUSE_LASTPRIVATE_FIRSTPRIVATE (c))
+		{
+		  lastprivate_firstprivate = true;
+		  if (pass != 0)
+		    continue;
+		}
 	      break;
 	    default:
 	      continue;
@@ -1611,6 +1646,9 @@ lower_rec_input_clauses (tree clauses, t
 	  switch (OMP_CLAUSE_CODE (c))
 	    {
 	    case OMP_CLAUSE_SHARED:
+	      /* Shared global vars are just accessed directly.  */
+	      if (is_global_var (new_var))
+		break;
 	      /* Set up the DECL_VALUE_EXPR for shared variables now.  This
 		 needs to be delayed until after fixup_child_record_type so
 		 that we get the correct type during the dereference.  */
@@ -1700,8 +1738,10 @@ lower_rec_input_clauses (tree clauses, t
 
   /* If any copyin variable is passed by reference, we must ensure the
      master thread doesn't modify it before it is copied over in all
-     threads.  */
-  if (copyin_by_ref)
+     threads.  Similarly for variables in both firstprivate and
+     lastprivate clauses we need to ensure the lastprivate copying
+     happens after firstprivate copying in all threads.  */
+  if (copyin_by_ref || lastprivate_firstprivate)
     build_omp_barrier (ilist);
 }
 
@@ -1919,6 +1959,9 @@ lower_send_clauses (tree clauses, tree *
       if (ctx->is_nested)
 	var = lookup_decl_in_outer_ctx (val, ctx);
 
+      if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_COPYIN
+	  && is_global_var (var))
+	continue;
       if (is_variable_sized (val))
 	continue;
       by_ref = use_pointer_for_field (val, false);
--- libgomp/testsuite/libgomp.c/pr26943-1.c.jj	2006-05-01 15:50:07.000000000 +0200
+++ libgomp/testsuite/libgomp.c/pr26943-1.c	2006-05-01 15:49:48.000000000 +0200
@@ -0,0 +1,24 @@
+/* PR c++/26943 */
+/* { dg-do run } */
+
+extern void abort (void);
+extern void omp_set_dynamic (int);
+int n = 6;
+
+int
+main (void)
+{
+  int i, x = 0;
+  omp_set_dynamic (0);
+#pragma omp parallel for num_threads (16) firstprivate (n) lastprivate (n) \
+			 schedule (static, 1) reduction (+: x)
+  for (i = 0; i < 16; i++)
+    {
+      if (n != 6)
+	++x;
+      n = i;
+    }
+  if (x || n != 15)
+    abort ();
+  return 0;
+}
--- libgomp/testsuite/libgomp.c/pr26943-2.c.jj	2006-05-01 16:41:34.000000000 +0200
+++ libgomp/testsuite/libgomp.c/pr26943-2.c	2006-05-01 16:42:01.000000000 +0200
@@ -0,0 +1,47 @@
+/* PR c++/26943 */
+/* { dg-do run } */
+
+extern int omp_set_dynamic (int);
+extern void abort (void);
+
+int a = 8, b = 12, c = 16, d = 20, j = 0;
+char e[10] = "a", f[10] = "b", g[10] = "c", h[10] = "d";
+
+int
+main (void)
+{
+  int i;
+  omp_set_dynamic (0);
+#pragma omp parallel for shared (a, e) firstprivate (b, f) \
+			 lastprivate (c, g) private (d, h) \
+			 schedule (static, 1) num_threads (4) \
+			 reduction (+:j)
+  for (i = 0; i < 4; i++)
+    {
+      if (a != 8 || b != 12 || e[0] != 'a' || f[0] != 'b')
+	j++;
+#pragma omp barrier
+#pragma omp atomic
+      a += i;
+      b += i;
+      c = i;
+      d = i;
+#pragma omp atomic
+      e[0] += i;
+      f[0] += i;
+      g[0] = 'g' + i;
+      h[0] = 'h' + i;
+#pragma omp barrier
+      if (a != 8 + 6 || b != 12 + i || c != i || d != i)
+	j += 8;
+      if (e[0] != 'a' + 6 || f[0] != 'b' + i || g[0] != 'g' + i)
+	j += 64;
+      if (h[0] != 'h' + i)
+	j += 512;
+    }
+  if (j || a != 8 + 6 || b != 12 || c != 3 || d != 20)
+    abort ();
+  if (e[0] != 'a' + 6 || f[0] != 'b' || g[0] != 'g' + 3 || h[0] != 'd')
+    abort ();
+  return 0;
+}
--- libgomp/testsuite/libgomp.c/pr26943-3.c.jj	2006-05-01 16:41:34.000000000 +0200
+++ libgomp/testsuite/libgomp.c/pr26943-3.c	2006-05-01 17:56:57.000000000 +0200
@@ -0,0 +1,56 @@
+/* PR c++/26943 */
+/* { dg-do run } */
+
+extern int omp_set_dynamic (int);
+extern int omp_get_thread_num (void);
+extern void abort (void);
+
+int a = 8, b = 12, c = 16, d = 20, j = 0, l = 0;
+char e[10] = "a", f[10] = "b", g[10] = "c", h[10] = "d";
+volatile int k;
+
+int
+main (void)
+{
+  int i;
+  omp_set_dynamic (0);
+  omp_set_nested (1);
+#pragma omp parallel num_threads (2) reduction (+:l)
+  if (k == omp_get_thread_num ())
+    {
+#pragma omp parallel for shared (a, e) firstprivate (b, f) \
+			 lastprivate (c, g) private (d, h) \
+			 schedule (static, 1) num_threads (4) \
+			 reduction (+:j)
+      for (i = 0; i < 4; i++)
+	{
+	  if (a != 8 || b != 12 || e[0] != 'a' || f[0] != 'b')
+	    j++;
+#pragma omp barrier
+#pragma omp atomic
+	  a += i;
+	  b += i;
+	  c = i;
+	  d = i;
+#pragma omp atomic
+	  e[0] += i;
+	  f[0] += i;
+	  g[0] = 'g' + i;
+	  h[0] = 'h' + i;
+#pragma omp barrier
+	  if (a != 8 + 6 || b != 12 + i || c != i || d != i)
+	    j += 8;
+	  if (e[0] != 'a' + 6 || f[0] != 'b' + i || g[0] != 'g' + i)
+	    j += 64;
+	  if (h[0] != 'h' + i)
+	    j += 512;
+	}
+      if (j || a != 8 + 6 || b != 12 || c != 3 || d != 20)
+	++l;
+      if (e[0] != 'a' + 6 || f[0] != 'b' || g[0] != 'g' + 3 || h[0] != 'd')
+	l += 8;
+    }
+  if (l)
+    abort ();
+  return 0;
+}
--- libgomp/testsuite/libgomp.c/pr26943-4.c.jj	2006-05-01 16:41:34.000000000 +0200
+++ libgomp/testsuite/libgomp.c/pr26943-4.c	2006-05-01 18:36:08.000000000 +0200
@@ -0,0 +1,61 @@
+/* PR c++/26943 */
+/* { dg-do run } */
+
+extern int omp_set_dynamic (int);
+extern int omp_get_thread_num (void);
+extern void abort (void);
+
+int a = 8, b = 12, c = 16, d = 20, j = 0, l = 0;
+char e[10] = "a", f[10] = "b", g[10] = "c", h[10] = "d";
+volatile int k;
+
+int
+main (void)
+{
+  int i;
+  omp_set_dynamic (0);
+  omp_set_nested (1);
+#pragma omp parallel num_threads (2) reduction (+:l) \
+		     firstprivate (a, b, c, d, e, f, g, h, j)
+  if (k == omp_get_thread_num ())
+    {
+#pragma omp parallel for shared (a, e) firstprivate (b, f) \
+			 lastprivate (c, g) private (d, h) \
+			 schedule (static, 1) num_threads (4) \
+			 reduction (+:j)
+      for (i = 0; i < 4; i++)
+	{
+	  if (a != 8 || b != 12 || e[0] != 'a' || f[0] != 'b')
+	    j++;
+#pragma omp barrier
+#pragma omp atomic
+	  a += i;
+	  b += i;
+	  c = i;
+	  d = i;
+#pragma omp atomic
+	  e[0] += i;
+	  f[0] += i;
+	  g[0] = 'g' + i;
+	  h[0] = 'h' + i;
+#pragma omp barrier
+	  if (a != 8 + 6 || b != 12 + i || c != i || d != i)
+	    j += 8;
+	  if (e[0] != 'a' + 6 || f[0] != 'b' + i || g[0] != 'g' + i)
+	    j += 64;
+	  if (h[0] != 'h' + i)
+	    j += 512;
+	}
+      if (j || a != 8 + 6 || b != 12 || c != 3 || d != 20)
+	++l;
+      if (e[0] != 'a' + 6 || f[0] != 'b' || g[0] != 'g' + 3 || h[0] != 'd')
+	l += 8;
+    }
+  if (l)
+    abort ();
+  if (a != 8 || b != 12 || c != 16 || d != 20)
+    abort ();
+  if (e[0] != 'a' || f[0] != 'b' || g[0] != 'c' || h[0] != 'd')
+    abort ();
+  return 0;
+}
--- libgomp/testsuite/libgomp.c++/pr27337.C.jj	2006-05-02 15:36:13.000000000 +0200
+++ libgomp/testsuite/libgomp.c++/pr27337.C	2006-05-01 16:22:36.000000000 +0200
@@ -48,11 +48,7 @@ foo ()
 #pragma omp parallel for firstprivate (ret) lastprivate (ret) \
 			 schedule (static, 1) num_threads (4)
   for (i = 0; i < 4; i++)
-    {
-      ret.i += omp_get_thread_num ();
-      // FIXME: The following barrier should be unnecessary.
-#pragma omp barrier
-    }
+    ret.i += omp_get_thread_num ();
 
   return ret;
 }
--- libgomp/testsuite/libgomp.c++/pr26943.C.jj	2006-05-01 15:42:17.000000000 +0200
+++ libgomp/testsuite/libgomp.c++/pr26943.C	2006-05-01 15:42:10.000000000 +0200
@@ -0,0 +1,62 @@
+// PR c++/26943
+// { dg-do run }
+
+#include <assert.h>
+#include <unistd.h>
+
+struct S
+{
+  public:
+    int x;
+    S () : x(-1) { }
+    S (const S &);
+    S& operator= (const S &);
+    void test ();
+};
+
+static volatile int hold;
+
+S::S (const S &s)
+{
+  #pragma omp master
+    sleep (1);
+
+  assert (s.x == -1);
+  x = 0;
+}
+
+S&
+S::operator= (const S& s)
+{
+  assert (s.x == 1);
+  x = 2;
+  return *this;
+}
+
+void
+S::test ()
+{
+  assert (x == 0);
+  x = 1;
+}
+
+static S x;
+
+void
+foo ()
+{
+  #pragma omp sections firstprivate(x) lastprivate(x)
+  {
+    x.test();
+  }
+}
+
+int
+main ()
+{
+  #pragma omp parallel num_threads(2)
+    foo();
+
+  assert (x.x == 2);
+  return 0;
+}


	Jakub