[PATCH] Fix a race condition in lastprivate (PR middle-end/35196, P1 regression from 4.2)

[Jakub Jelinek <jakub at redhat dot com>]

Hi!

The http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=128223 change
introduced a race condition into lastprivate handling of omp for
with no-chunk static - if there is at least one thread which will have
no iterations assigned to it at all, then those zero-assigned-iterations
threads race with the thread that executes the last loop body who will
copy the lastprivate variables last.  If the one that executes the last
loop body doesn't come in last, then except for the iteration variable
it will copy uninitialized garbage.

Since r128223 gcc generates:
  i = 0;	<----- this is initialized in lower_omp_for_lastprivate
  D.1576 = __builtin_omp_get_num_threads ();
  D.1577 = __builtin_omp_get_thread_num ();
  D.1578 = 5 / D.1576;
  D.1579 = D.1578 * D.1576;
  D.1580 = D.1579 != 5;
  D.1581 = D.1580 + D.1578;
  D.1582 = D.1581 * D.1577;
  D.1583 = D.1582 + D.1581;
  D.1584 = MIN_EXPR <D.1583, 5>;
  i = D.1582;	<----- fd->v is initialized here, overwriting the above i = 0
  if (D.1582 >= D.1584)	<----- if D.1582 == D.1584 == 5 here, it jumps straight to bb5, with i == 5
    goto <bb 5>;
  else
    goto <bb 3>;
<bb 3>:
		<----- here we used to set i = D.1582 before and with the patch below too
		       The only difference is for the D.1582 >= D.1584 case.
<bb 4>:
  j = i;
  i = i + 1;
  if (i < D.1584)
    goto <bb 4>;
  else
    goto <bb 5>;
<bb 5>:
  if (i == 5)	<----- i == 5 implies last loop, performing lastprivate copying
    goto <bb 7>;
  else
    goto <bb 6>;
<bb 6>:
  return;
<bb 7>:
  .omp_data_i->i = i;
  .omp_data_i->j = j;
  goto <bb 6>;

The patch also removes a redundant second initialization of fd->v
in the non-static schedule expansion - fd->v is initialized during
lower_omp_for_lastprivate already, so if there is any lastprivate, there
is no point initializing it again, if there is no lastprivate, then
it is a useless store, as nothing will look at the fd->v value afterwards.

Regtested on x86_64-linux, ok for trunk?

2008-02-15  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/35196
	* omp-low.c (expand_omp_for_generic): Don't initialize fd->v
	in entry_bb.
	(expand_omp_for_static_nochunk): Initialize fd->v in seq_start_bb
	rather than in entry_bb.

	* testsuite/libgomp.c/pr35196.c: New test.

--- gcc/omp-low.c.jj	2008-02-11 14:48:12.000000000 +0100
+++ gcc/omp-low.c	2008-02-15 16:17:40.000000000 +0100
@@ -2782,22 +2782,6 @@ expand_omp_for_generic (struct omp_regio
   t = build3 (COND_EXPR, void_type_node, t, NULL_TREE, NULL_TREE);
   bsi_insert_after (&si, t, BSI_SAME_STMT);
 
-  /* V may be used outside of the loop (e.g., to handle lastprivate clause).
-     If this is the case, its value is undefined if the loop is not entered
-     at all.  To handle this case, set its initial value to N1.  */
-  if (gimple_in_ssa_p (cfun))
-    {
-      e = find_edge (entry_bb, l3_bb);
-      for (phi = phi_nodes (l3_bb); phi; phi = PHI_CHAIN (phi))
-	if (PHI_ARG_DEF_FROM_EDGE (phi, e) == fd->v)
-	  SET_USE (PHI_ARG_DEF_PTR_FROM_EDGE (phi, e), fd->n1);
-    }
-  else
-    {
-      t = build_gimple_modify_stmt (fd->v, fd->n1);
-      bsi_insert_before (&si, t, BSI_SAME_STMT);
-    }
-
   /* Remove the OMP_FOR statement.  */
   bsi_remove (&si, true);
 
@@ -2995,16 +2979,6 @@ expand_omp_for_static_nochunk (struct om
   t = fold_build2 (MIN_EXPR, type, t, n);
   e0 = force_gimple_operand_bsi (&si, t, true, NULL_TREE, true, BSI_SAME_STMT);
 
-  t = fold_convert (type, s0);
-  t = fold_build2 (MULT_EXPR, type, t, fd->step);
-  t = fold_build2 (PLUS_EXPR, type, t, fd->n1);
-  t = force_gimple_operand_bsi (&si, t, false, NULL_TREE,
-				true, BSI_SAME_STMT);
-  t = build_gimple_modify_stmt (fd->v, t);
-  bsi_insert_before (&si, t, BSI_SAME_STMT);
-  if (gimple_in_ssa_p (cfun))
-    SSA_NAME_DEF_STMT (fd->v) = t;
-
   t = build2 (GE_EXPR, boolean_type_node, s0, e0);
   t = build3 (COND_EXPR, void_type_node, t, NULL_TREE, NULL_TREE);
   bsi_insert_before (&si, t, BSI_SAME_STMT);
@@ -3015,6 +2989,16 @@ expand_omp_for_static_nochunk (struct om
   /* Setup code for sequential iteration goes in SEQ_START_BB.  */
   si = bsi_start (seq_start_bb);
 
+  t = fold_convert (type, s0);
+  t = fold_build2 (MULT_EXPR, type, t, fd->step);
+  t = fold_build2 (PLUS_EXPR, type, t, fd->n1);
+  t = force_gimple_operand_bsi (&si, t, false, NULL_TREE,
+				false, BSI_CONTINUE_LINKING);
+  t = build_gimple_modify_stmt (fd->v, t);
+  bsi_insert_after (&si, t, BSI_CONTINUE_LINKING);
+  if (gimple_in_ssa_p (cfun))
+    SSA_NAME_DEF_STMT (fd->v) = t;
+
   t = fold_convert (type, e0);
   t = fold_build2 (MULT_EXPR, type, t, fd->step);
   t = fold_build2 (PLUS_EXPR, type, t, fd->n1);
--- libgomp/testsuite/libgomp.c/pr35196.c.jj	2008-02-15 16:23:30.000000000 +0100
+++ libgomp/testsuite/libgomp.c/pr35196.c	2008-02-15 16:23:03.000000000 +0100
@@ -0,0 +1,43 @@
+/* PR middle-end/35196 */
+/* { dg-do run } */
+
+extern void abort (void);
+extern void omp_set_dynamic (int);
+
+int
+main (void)
+{
+  int i, j;
+  omp_set_dynamic (0);
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (static)
+  for (i = 0; i < 5; i++)
+    j = i;
+  if (i != 5 || j != 4)
+    abort ();
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (static, 2)
+  for (i = 0; i < 5; i++)
+    j = i;
+  if (i != 5 || j != 4)
+    abort ();
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (dynamic)
+  for (i = 0; i < 5; i++)
+    j = i;
+  if (i != 5 || j != 4)
+    abort ();
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (static)
+  for (i = -12; i < 21; i += 3)
+    j = i;
+  if (i != 21 || j != 18)
+    abort ();
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (static, 2)
+  for (i = -12; i < 21; i += 3)
+    j = i;
+  if (i != 21 || j != 18)
+    abort ();
+#pragma omp parallel for lastprivate (i, j) num_threads (8) schedule (dynamic, 3)
+  for (i = -12; i < 21; i += 3)
+    j = i;
+  if (i != 21 || j != 18)
+    abort ();
+  return 0;
+}

	Jakub