Re: [gomp3] Task sharing fixes

[Jakub Jelinek <jakub at redhat dot com>]

On Tue, Mar 11, 2008 at 04:41:41PM -0400, Jakub Jelinek wrote:
> This patch fixes two problems with tasks (as soon as they aren't just
> stubbed - forcefully if (0)ed in libgomp).

Here is an updated patch on top of the changes I did earlier today in
4.4/4.3.  I've added two testcases as well.  Committed to gomp-3_0-branch.

2008-03-12  Jakub Jelinek  <jakub@redhat.com>

	* tree.h (OMP_TASK_EXPLICIT_START): Define.
	* omp-low.c (task_shared_vars): New variable.
	(use_pointer_for_field): In OMP_TASK disallow copy-in/out
	sharing.
	(lower_send_shared_vars): Don't copy-out if TREE_READONLY,
	only copy-in.
	(lower_rec_input_clauses): Set OMP_TASK_EXPLICIT_START
	if firstprivate or allocatable private needs to copy data from
	outer task.  Emit GOMP_task_wait call if so.
	(expand_task_call): Change last GOMP_task argument to bitmask.
	* builtin-types.def (BT_FN_VOID_OMPFN_PTR_BOOL_BOOL): Remove.
	(BT_FN_VOID_OMPFN_PTR_BOOL_UINT): New.
	* omp-builtins.def (BUILT_IN_GOMP_TASK_START): New.
	(BUILT_IN_GOMP_TASK): Change type of last argument.

	* types.def (BT_FN_VOID_OMPFN_PTR_BOOL_BOOL): Remove.
	(BT_FN_VOID_OMPFN_PTR_BOOL_UINT): New.

	* libgomp_g.h (GOMP_task_flag_untied,
	GOMP_task_flag_explicit_start): Define.
	* libgomp.map (GOMP_task_start): Export @@GOMP_2.0.
	* task.c (GOMP_task): Change last argument.
	(GOMP_task_start): New function.
	* testsuite/libgomp.c/task-3.c: New test.
	* testsuite/libgomp.c++/task-2.C: New test.

--- libgomp/libgomp_g.h	(revision 133137)
+++ libgomp/libgomp_g.h	(working copy)
@@ -1,4 +1,4 @@
-/* Copyright (C) 2005 Free Software Foundation, Inc.
+/* Copyright (C) 2005, 2007, 2008 Free Software Foundation, Inc.
    Contributed by Richard Henderson <rth@redhat.com>.
 
    This file is part of the GNU OpenMP Library (libgomp).
@@ -95,7 +95,10 @@ extern void GOMP_parallel_end (void);
 
 /* team.c */
 
-extern void GOMP_task (void (*) (void *), void *, bool, bool);
+#define GOMP_task_flag_untied 	      1 /* UNTIED clause present.  */
+#define GOMP_task_flag_explicit_start 2 /* Explicit GOMP_task_start needed.  */
+extern void GOMP_task (void (*) (void *), void *, bool, unsigned);
+extern void GOMP_task_start (void);
 extern void GOMP_taskwait (void);
 
 /* sections.c */
--- libgomp/libgomp.map	(revision 133137)
+++ libgomp/libgomp.map	(working copy)
@@ -154,5 +154,6 @@ GOMP_1.0 {
 GOMP_2.0 {
   global:
 	GOMP_task;
+	GOMP_task_start;
 	GOMP_taskwait;
 } GOMP_1.0;
--- libgomp/task.c	(revision 133137)
+++ libgomp/task.c	(working copy)
@@ -1,4 +1,4 @@
-/* Copyright (C) 2007 Free Software Foundation, Inc.
+/* Copyright (C) 2007, 2008 Free Software Foundation, Inc.
    Contributed by Richard Henderson <rth@redhat.com>.
 
    This file is part of the GNU OpenMP Library (libgomp).
@@ -63,7 +63,7 @@ gomp_end_task (void)
 void
 GOMP_task (void (*fn) (void *), void *data,
 	   bool if_clause __attribute__((unused)),
-	   bool untied __attribute__((unused)))
+	   unsigned flags __attribute__((unused)))
 {
   struct gomp_thread *thr = gomp_thread ();
   thr->task = gomp_new_task (thr->task, gomp_icv ());
@@ -75,6 +75,17 @@ GOMP_task (void (*fn) (void *), void *da
   gomp_end_task ();
 }
 
+/* Called after a task has been initialized.  Only should be called if
+   GOMP_task was called with GOMP_task_flag_explicit_start bit set,
+   after all firstprivate etc. copying is done.  The copying will
+   happen immediately, in the thread that created the task, afterwards
+   it can be suspended and/or moved to another thread, even if not untied.  */
+
+void
+GOMP_task_start (void)
+{
+}
+
 /* Called when encountering a taskwait directive.  */
 
 void
--- libgomp/testsuite/libgomp.c++/task-2.C	(revision 0)
+++ libgomp/testsuite/libgomp.c++/task-2.C	(revision 0)
@@ -0,0 +1,70 @@
+// { dg-do run }
+
+#include <omp.h>
+extern "C" void abort ();
+
+int l = 5;
+
+int
+foo (int i)
+{
+  int j = 7;
+  const int k = 8;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp taskwait
+  return (i != 8 * omp_get_thread_num () + 4
+	  || j != 4 * i - 3
+	  || k != 8);
+}
+
+int
+main (void)
+{
+  int r = 0;
+  #pragma omp parallel num_threads (4) reduction(+:r)
+    if (omp_get_num_threads () != 4)
+      {
+	#pragma omp master
+	  l = 133;
+      }
+    else if (foo (8 * omp_get_thread_num ()))
+      r++;
+  if (r || l != 133)
+    abort ();
+  return 0;
+}
--- libgomp/testsuite/libgomp.c/task-3.c	(revision 0)
+++ libgomp/testsuite/libgomp.c/task-3.c	(revision 0)
@@ -0,0 +1,70 @@
+/* { dg-do run } */
+
+#include <omp.h>
+extern void abort ();
+
+int l = 5;
+
+int
+foo (int i)
+{
+  int j = 7;
+  const int k = 8;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp task firstprivate (i) shared (j, l)
+  {
+    #pragma omp critical
+      {
+	j += i;
+	l += k;
+      }
+  }
+  i++;
+  #pragma omp taskwait
+  return (i != 8 * omp_get_thread_num () + 4
+	  || j != 4 * i - 3
+	  || k != 8);
+}
+
+int
+main (void)
+{
+  int r = 0;
+  #pragma omp parallel num_threads (4) reduction(+:r)
+    if (omp_get_num_threads () != 4)
+      {
+	#pragma omp master
+	  l = 133;
+      }
+    else if (foo (8 * omp_get_thread_num ()))
+      r++;
+  if (r || l != 133)
+    abort ();
+  return 0;
+}
--- gcc/tree.h	(revision 133139)
+++ gcc/tree.h	(working copy)
@@ -501,6 +501,8 @@ struct gimple_stmt GTY(())
 	   OMP_SECTION
        OMP_PARALLEL_COMBINED in
 	   OMP_PARALLEL
+       OMP_TASK_EXPLICIT_START in
+	   OMP_TASK
        OMP_CLAUSE_PRIVATE_OUTER_REF in
 	   OMP_CLAUSE_PRIVATE
 
@@ -1803,6 +1805,11 @@ struct tree_constructor GTY(())
 #define OMP_PARALLEL_COMBINED(NODE) \
   TREE_PRIVATE (OMP_PARALLEL_CHECK (NODE))
 
+/* True on an OMP_TASK statement if explicit GOMP_task_start call
+   is needed after privatized variable initialization.  */
+#define OMP_TASK_EXPLICIT_START(NODE) \
+  TREE_PRIVATE (OMP_TASK_CHECK (NODE))
+
 /* True on a PRIVATE clause if its decl is kept around for debugging
    information only and its DECL_VALUE_EXPR is supposed to point
    to what it has been remapped to.  */
--- gcc/omp-low.c	(revision 133139)
+++ gcc/omp-low.c	(working copy)
@@ -118,6 +118,7 @@ struct omp_for_data
 static splay_tree all_contexts;
 static int taskreg_nesting_level;
 struct omp_region *root_omp_region;
+static bitmap task_shared_vars;
 
 static void scan_omp (tree *, omp_context *);
 static void lower_omp (tree *, omp_context *);
@@ -627,11 +628,11 @@ use_pointer_for_field (const_tree decl, 
 	    if (maybe_lookup_decl (decl, up))
 	      break;
 
-	  if (up && is_parallel_ctx (up))
+	  if (up && is_taskreg_ctx (up))
 	    {
 	      tree c;
 
-	      for (c = OMP_PARALLEL_CLAUSES (up->stmt);
+	      for (c = OMP_TASKREG_CLAUSES (up->stmt);
 		   c; c = OMP_CLAUSE_CHAIN (c))
 		if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_SHARED
 		    && OMP_CLAUSE_DECL (c) == decl)
@@ -641,6 +642,24 @@ use_pointer_for_field (const_tree decl, 
 		return true;
 	    }
 	}
+
+      /* For tasks copy-out is not possible, so force by_ref.  */
+      if (!TREE_READONLY (decl)
+	  && TREE_CODE (shared_ctx->stmt) == OMP_TASK)
+	{
+	  tree outer = maybe_lookup_decl_in_outer_ctx (decl, shared_ctx);
+	  if (is_gimple_reg (outer))
+	    {
+	      /* Taking address of OUTER in lower_send_shared_vars
+		 might need regimplification of everything that uses the
+		 variable.  */
+	      if (!task_shared_vars)
+		task_shared_vars = BITMAP_ALLOC (NULL);
+	      bitmap_set_bit (task_shared_vars, DECL_UID (outer));
+	      TREE_ADDRESSABLE (outer) = 1;
+	    }
+	  return true;
+	}
     }
 
   return false;
@@ -1099,11 +1118,11 @@ scan_sharing_clauses (tree clauses, omp_
 	  gcc_assert (is_taskreg_ctx (ctx));
 	  decl = OMP_CLAUSE_DECL (c);
 	  gcc_assert (!is_variable_sized (decl));
-	  by_ref = use_pointer_for_field (decl, ctx);
 	  /* 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;
+	  by_ref = use_pointer_for_field (decl, ctx);
 	  if (! TREE_READONLY (decl)
 	      || TREE_ADDRESSABLE (decl)
 	      || by_ref
@@ -1987,7 +2006,12 @@ lower_rec_input_clauses (tree clauses, t
 	    case OMP_CLAUSE_PRIVATE:
 	      if (OMP_CLAUSE_CODE (c) != OMP_CLAUSE_PRIVATE
 		  || OMP_CLAUSE_PRIVATE_OUTER_REF (c))
-		x = build_outer_var_ref (var, ctx);
+		{
+		  x = build_outer_var_ref (var, ctx);
+		  if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_PRIVATE
+		      && TREE_CODE (ctx->stmt) == OMP_TASK)
+		    OMP_TASK_EXPLICIT_START (ctx->stmt) = 1;
+		}
 	      else
 		x = NULL;
 	      x = lang_hooks.decls.omp_clause_default_ctor (c, new_var, x);
@@ -2009,6 +2033,14 @@ lower_rec_input_clauses (tree clauses, t
 	      x = build_outer_var_ref (var, ctx);
 	      x = lang_hooks.decls.omp_clause_copy_ctor (c, new_var, x);
 	      gimplify_and_add (x, ilist);
+	      if (TREE_CODE (ctx->stmt) == OMP_TASK)
+		{
+		  if (is_global_var (maybe_lookup_decl_in_outer_ctx (var,
+								     ctx))
+		      || is_variable_sized (var)
+		      || use_pointer_for_field (var, NULL))
+		    OMP_TASK_EXPLICIT_START (ctx->stmt) = 1;
+		}
 	      goto do_dtor;
 	      break;
 
@@ -2068,6 +2100,14 @@ lower_rec_input_clauses (tree clauses, t
      happens after firstprivate copying in all threads.  */
   if (copyin_by_ref || lastprivate_firstprivate)
     gimplify_and_add (build_omp_barrier (), ilist);
+
+  if (TREE_CODE (ctx->stmt) == OMP_TASK
+      && OMP_TASK_EXPLICIT_START (ctx->stmt))
+    {
+      x = built_in_decls[BUILT_IN_GOMP_TASK_START];
+      x = build_call_expr (x, 0);
+      gimplify_and_add (x, ilist);
+    }
 }
 
 
@@ -2401,9 +2441,12 @@ lower_send_shared_vars (tree *ilist, tre
 	  x = build_gimple_modify_stmt (x, var);
 	  gimplify_and_add (x, ilist);
 
-	  x = build_sender_ref (ovar, ctx);
-	  x = build_gimple_modify_stmt (var, x);
-	  gimplify_and_add (x, olist);
+	  if (!TREE_READONLY (var))
+	    {
+	      x = build_sender_ref (ovar, ctx);
+	      x = build_gimple_modify_stmt (var, x);
+	      gimplify_and_add (x, olist);
+	    }
 	}
     }
 }
@@ -2583,7 +2626,7 @@ expand_parallel_call (struct omp_region 
 static void
 expand_task_call (basic_block bb, tree entry_stmt)
 {
-  tree t, t1, t2, untied, cond, c, clauses;
+  tree t, t1, t2, flags, cond, c, clauses;
   block_stmt_iterator si;
 
   clauses = OMP_TASK_CLAUSES (entry_stmt);
@@ -2595,7 +2638,9 @@ expand_task_call (basic_block bb, tree e
     cond = boolean_true_node;
 
   c = find_omp_clause (clauses, OMP_CLAUSE_UNTIED);
-  untied = c ? boolean_true_node : boolean_false_node;
+  flags = build_int_cst (unsigned_type_node,
+			 (c ? 1 : 0)
+			 | (OMP_TASK_EXPLICIT_START (entry_stmt) ? 2 : 0));
 
   si = bsi_last (bb);
   t = OMP_TASK_DATA_ARG (entry_stmt);
@@ -2606,7 +2651,7 @@ expand_task_call (basic_block bb, tree e
   t2 = build_fold_addr_expr (OMP_TASK_FN (entry_stmt));
 
   t = build_call_expr (built_in_decls[BUILT_IN_GOMP_TASK], 4, t2, t1,
-		       cond, untied);
+		       cond, flags);
 
   force_gimple_operand_bsi (&si, t, true, NULL_TREE,
 			    false, BSI_CONTINUE_LINKING);
@@ -5491,7 +5536,9 @@ lower_omp_1 (tree *tp, int *walk_subtree
       break;
 
     case VAR_DECL:
-      if (ctx && DECL_HAS_VALUE_EXPR_P (t))
+      if ((ctx && DECL_HAS_VALUE_EXPR_P (t))
+	  || (task_shared_vars
+	      && bitmap_bit_p (task_shared_vars, DECL_UID (t))))
 	{
 	  lower_regimplify (&t, wi);
 	  if (wi->val_only)
@@ -5506,7 +5553,7 @@ lower_omp_1 (tree *tp, int *walk_subtree
       break;
 
     case ADDR_EXPR:
-      if (ctx)
+      if (ctx || task_shared_vars)
 	lower_regimplify (tp, wi);
       break;
 
@@ -5516,12 +5563,12 @@ lower_omp_1 (tree *tp, int *walk_subtree
     case IMAGPART_EXPR:
     case COMPONENT_REF:
     case VIEW_CONVERT_EXPR:
-      if (ctx)
+      if (ctx || task_shared_vars)
 	lower_regimplify (tp, wi);
       break;
 
     case INDIRECT_REF:
-      if (ctx)
+      if (ctx || task_shared_vars)
 	{
 	  wi->is_lhs = false;
 	  wi->val_only = true;
@@ -5564,13 +5611,20 @@ execute_lower_omp (void)
   gcc_assert (taskreg_nesting_level == 0);
 
   if (all_contexts->root)
-    lower_omp (&DECL_SAVED_TREE (current_function_decl), NULL);
+    {
+      if (task_shared_vars)
+	push_gimplify_context ();
+      lower_omp (&DECL_SAVED_TREE (current_function_decl), NULL);
+      if (task_shared_vars)
+	pop_gimplify_context (NULL);
+    }
 
   if (all_contexts)
     {
       splay_tree_delete (all_contexts);
       all_contexts = NULL;
     }
+  BITMAP_FREE (task_shared_vars);
   return 0;
 }
 
--- gcc/builtin-types.def	(revision 133137)
+++ gcc/builtin-types.def	(working copy)
@@ -1,4 +1,4 @@
-/* Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007
+/* Copyright (C) 2001, 2002, 2003, 2004, 2005, 2006, 2007, 2008
    Free Software Foundation, Inc.
 
 This file is part of GCC.
@@ -393,8 +393,8 @@ DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PT
 		     BT_VOID, BT_PTR_FN_VOID_PTR, BT_PTR, BT_UINT, BT_UINT)
 DEF_FUNCTION_TYPE_4 (BT_FN_VOID_PTR_WORD_WORD_PTR,
 		     BT_VOID, BT_PTR, BT_WORD, BT_WORD, BT_PTR)
-DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PTR_BOOL_BOOL, BT_VOID, BT_PTR_FN_VOID_PTR,
-		     BT_PTR, BT_BOOL, BT_BOOL)
+DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PTR_BOOL_UINT, BT_VOID, BT_PTR_FN_VOID_PTR,
+		     BT_PTR, BT_BOOL, BT_UINT)
 
 DEF_FUNCTION_TYPE_5 (BT_FN_INT_STRING_INT_SIZE_CONST_STRING_VALIST_ARG,
 		     BT_INT, BT_STRING, BT_INT, BT_SIZE, BT_CONST_STRING,
--- gcc/fortran/types.def	(revision 133137)
+++ gcc/fortran/types.def	(working copy)
@@ -117,8 +117,8 @@ DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PT
                      BT_VOID, BT_PTR_FN_VOID_PTR, BT_PTR, BT_UINT, BT_UINT)
 DEF_FUNCTION_TYPE_4 (BT_FN_VOID_PTR_WORD_WORD_PTR,
 		     BT_VOID, BT_PTR, BT_WORD, BT_WORD, BT_PTR)
-DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PTR_BOOL_BOOL, BT_VOID,
-		     BT_PTR_FN_VOID_PTR, BT_PTR, BT_BOOL, BT_BOOL)
+DEF_FUNCTION_TYPE_4 (BT_FN_VOID_OMPFN_PTR_BOOL_UINT, BT_VOID,
+		     BT_PTR_FN_VOID_PTR, BT_PTR, BT_BOOL, BT_UINT)
 
 DEF_FUNCTION_TYPE_5 (BT_FN_BOOL_LONG_LONG_LONG_LONGPTR_LONGPTR,
                      BT_BOOL, BT_LONG, BT_LONG, BT_LONG,
--- gcc/omp-builtins.def	(revision 133137)
+++ gcc/omp-builtins.def	(working copy)
@@ -1,6 +1,6 @@
 /* This file contains the definitions and documentation for the
    OpenMP builtins used in the GNU compiler.
-   Copyright (C) 2005, 2007 Free Software Foundation, Inc.
+   Copyright (C) 2005, 2007, 2008 Free Software Foundation, Inc.
 
 This file is part of GCC.
 
@@ -37,6 +37,8 @@ DEF_GOMP_BUILTIN (BUILT_IN_GOMP_BARRIER,
 		  BT_FN_VOID, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_TASKWAIT, "GOMP_taskwait",
 		  BT_FN_VOID, ATTR_NOTHROW_LIST)
+DEF_GOMP_BUILTIN (BUILT_IN_GOMP_TASK_START, "GOMP_task_start",
+		  BT_FN_VOID, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_CRITICAL_START, "GOMP_critical_start",
 		  BT_FN_VOID, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_CRITICAL_END, "GOMP_critical_end",
@@ -151,7 +153,7 @@ DEF_GOMP_BUILTIN (BUILT_IN_GOMP_PARALLEL
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_PARALLEL_END, "GOMP_parallel_end",
 		  BT_FN_VOID, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_TASK, "GOMP_task",
-		  BT_FN_VOID_OMPFN_PTR_BOOL_BOOL, ATTR_NOTHROW_LIST)
+		  BT_FN_VOID_OMPFN_PTR_BOOL_UINT, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_SECTIONS_START, "GOMP_sections_start",
 		  BT_FN_UINT_UINT, ATTR_NOTHROW_LIST)
 DEF_GOMP_BUILTIN (BUILT_IN_GOMP_SECTIONS_NEXT, "GOMP_sections_next",


	Jakub