[PATCH] Fix lower_omp regimplification (PR middle-end/35185)

[PATCH] Fix lower_omp regimplification (PR middle-end/35185)

[Jakub Jelinek]

Hi!

This patch rewrites lower_omp regimplification.  Previously, walk_stmts
was used and all ADDR_EXPRs, INDIRECT_REFs, etc. and VAR_DECLs with
DECL_HAS_VALUE_EXPR_P were regimplified using lower_regimplify.
The problem with this is that it has to guess the gimple predicate which
should be used for gimplify_expr, and often it guesses it wrong.
Either is more restrictive than necessary, creating unnecessary temporaries,
or it is too restrictive (as shown on the testcase below).

The new method implemented below is for each statement use walk_tree
to find if it needs to be regimplified, then regimplify whole statements.

Regtested on x86_64-linux, also regtested on gomp-3_0-branch.

Diego, does this look ok to you?

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

	PR middle-end/35185
	* omp-low.c (lower_regimplify, init_tmp_var, save_tmp_var): Removed.
	(lower_omp_2): New function.
	(lower_omp_1, lower_omp): Rewritten.

	* testsuite/libgomp.c++/pr35185.C: New test.

--- gcc/omp-low.c.jj	2008-03-12 10:10:26.000000000 +0100
+++ gcc/omp-low.c	2008-03-12 21:52:01.000000000 +0100
@@ -4880,184 +4880,177 @@ lower_omp_parallel (tree *stmt_p, omp_co
   pop_gimplify_context (NULL_TREE);
 }
 
-
-/* Pass *TP back through the gimplifier within the context determined by WI.
-   This handles replacement of DECL_VALUE_EXPR, as well as adjusting the 
-   flags on ADDR_EXPR.  */
-
-static void
-lower_regimplify (tree *tp, struct walk_stmt_info *wi)
-{
-  enum gimplify_status gs;
-  tree pre = NULL;
-
-  if (wi->is_lhs)
-    gs = gimplify_expr (tp, &pre, NULL, is_gimple_lvalue, fb_lvalue);
-  else if (wi->val_only)
-    gs = gimplify_expr (tp, &pre, NULL, is_gimple_val, fb_rvalue);
-  else
-    gs = gimplify_expr (tp, &pre, NULL, is_gimple_formal_tmp_var, fb_rvalue);
-  gcc_assert (gs == GS_ALL_DONE);
-
-  if (pre)
-    tsi_link_before (&wi->tsi, pre, TSI_SAME_STMT);
-}
-
-/* Copy EXP into a temporary.  Insert the initialization statement before TSI.  */
+/* Callback for lower_omp_1.  Return non-NULL if *tp needs to be
+   regimplified.  */
 
 static tree
-init_tmp_var (tree exp, tree_stmt_iterator *tsi)
+lower_omp_2 (tree *tp, int *walk_subtrees, void *data ATTRIBUTE_UNUSED)
 {
-  tree t, stmt;
+  tree t = *tp;
 
-  t = create_tmp_var (TREE_TYPE (exp), NULL);
-  DECL_GIMPLE_REG_P (t) = 1;
-  stmt = build_gimple_modify_stmt (t, exp);
-  SET_EXPR_LOCUS (stmt, EXPR_LOCUS (tsi_stmt (*tsi)));
-  tsi_link_before (tsi, stmt, TSI_SAME_STMT);
+  /* Any variable with DECL_VALUE_EXPR needs to be regimplified.  */
+  if (TREE_CODE (t) == VAR_DECL && DECL_HAS_VALUE_EXPR_P (t))
+    return t;
+
+  /* If a global variable has been privatized, TREE_CONSTANT on
+     ADDR_EXPR might be wrong.  */
+  if (TREE_CODE (t) == ADDR_EXPR)
+    recompute_tree_invariant_for_addr_expr (t);
 
-  return t;
+  *walk_subtrees = !TYPE_P (t) && !DECL_P (t);
+  return NULL_TREE;
 }
 
-/* Similarly, but copy from the temporary and insert the statement
-   after the iterator.  */
-
-static tree
-save_tmp_var (tree exp, tree_stmt_iterator *tsi)
+static void
+lower_omp_1 (tree *tp, omp_context *ctx, tree_stmt_iterator *tsi)
 {
-  tree t, stmt;
-
-  t = create_tmp_var (TREE_TYPE (exp), NULL);
-  DECL_GIMPLE_REG_P (t) = 1;
-  stmt = build_gimple_modify_stmt (exp, t);
-  SET_EXPR_LOCUS (stmt, EXPR_LOCUS (tsi_stmt (*tsi)));
-  tsi_link_after (tsi, stmt, TSI_SAME_STMT);
-
-  return t;
-}
+  tree t = *tp;
 
-/* Callback for walk_stmts.  Lower the OpenMP directive pointed by TP.  */
+  if (!t)
+    return;
 
-static tree
-lower_omp_1 (tree *tp, int *walk_subtrees, void *data)
-{
-  struct walk_stmt_info *wi = data;
-  omp_context *ctx = wi->info;
-  tree t = *tp;
+  if (EXPR_HAS_LOCATION (t))
+    input_location = EXPR_LOCATION (t);
 
   /* If we have issued syntax errors, avoid doing any heavy lifting.
      Just replace the OpenMP directives with a NOP to avoid
      confusing RTL expansion.  */
-  if (errorcount && OMP_DIRECTIVE_P (*tp))
+  if (errorcount && OMP_DIRECTIVE_P (t))
     {
       *tp = build_empty_stmt ();
-      return NULL_TREE;
+      return;
     }
 
-  *walk_subtrees = 0;
-  switch (TREE_CODE (*tp))
+  switch (TREE_CODE (t))
     {
+    case STATEMENT_LIST:
+      {
+	tree_stmt_iterator i;
+	for (i = tsi_start (t); !tsi_end_p (i); tsi_next (&i))
+	  lower_omp_1 (tsi_stmt_ptr (i), ctx, &i);
+      }
+      break;
+
+    case COND_EXPR:
+      lower_omp_1 (&COND_EXPR_THEN (t), ctx, NULL);
+      lower_omp_1 (&COND_EXPR_ELSE (t), ctx, NULL);
+      if (ctx
+	  && walk_tree (&COND_EXPR_COND (t), lower_omp_2, ctx, NULL))
+	{
+	  tree pre = NULL;
+	  gimplify_expr (&COND_EXPR_COND (t), &pre, NULL,
+			 is_gimple_condexpr, fb_rvalue);
+	  if (pre)
+	    {
+	      if (tsi)
+		tsi_link_before (tsi, pre, TSI_SAME_STMT);
+	      else
+		{
+		  append_to_statement_list (t, &pre);
+		  *tp = pre;
+		}
+	    }
+	}
+      break;
+    case CATCH_EXPR:
+      lower_omp_1 (&CATCH_BODY (t), ctx, NULL);
+      break;
+    case EH_FILTER_EXPR:
+      lower_omp_1 (&EH_FILTER_FAILURE (t), ctx, NULL);
+      break;
+    case TRY_CATCH_EXPR:
+    case TRY_FINALLY_EXPR:
+      lower_omp_1 (&TREE_OPERAND (t, 0), ctx, NULL);
+      lower_omp_1 (&TREE_OPERAND (t, 1), ctx, NULL);
+      break;
+    case BIND_EXPR:
+      lower_omp_1 (&BIND_EXPR_BODY (t), ctx, NULL);
+      break;
+    case RETURN_EXPR:
+      lower_omp_1 (&TREE_OPERAND (t, 0), ctx, NULL);
+      break;
+
     case OMP_PARALLEL:
       ctx = maybe_lookup_ctx (t);
       lower_omp_parallel (tp, ctx);
       break;
-
     case OMP_FOR:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_for (tp, ctx);
       break;
-
     case OMP_SECTIONS:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_sections (tp, ctx);
       break;
-
     case OMP_SINGLE:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_single (tp, ctx);
       break;
-
     case OMP_MASTER:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_master (tp, ctx);
       break;
-
     case OMP_ORDERED:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_ordered (tp, ctx);
       break;
-
     case OMP_CRITICAL:
       ctx = maybe_lookup_ctx (t);
       gcc_assert (ctx);
       lower_omp_critical (tp, ctx);
       break;
 
-    case VAR_DECL:
-      if (ctx && DECL_HAS_VALUE_EXPR_P (t))
+    default:
+      if (ctx && walk_tree (tp, lower_omp_2, ctx, NULL))
 	{
-	  lower_regimplify (&t, wi);
-	  if (wi->val_only)
+	  /* The gimplifier doesn't gimplify CALL_EXPR_STATIC_CHAIN.
+	     Handle that here.  */
+	  tree call = get_call_expr_in (t);
+	  if (call
+	      && CALL_EXPR_STATIC_CHAIN (call)
+	      && walk_tree (&CALL_EXPR_STATIC_CHAIN (call), lower_omp_2,
+			    ctx, NULL))
 	    {
-	      if (wi->is_lhs)
-		t = save_tmp_var (t, &wi->tsi);
-	      else
-		t = init_tmp_var (t, &wi->tsi);
+	      tree pre = NULL;
+	      gimplify_expr (&CALL_EXPR_STATIC_CHAIN (call), &pre, NULL,
+			     is_gimple_val, fb_rvalue);
+	      if (pre)
+		{
+		  if (tsi)
+		    tsi_link_before (tsi, pre, TSI_SAME_STMT);
+		  else
+		    {
+		      append_to_statement_list (t, &pre);
+		      lower_omp_1 (&pre, ctx, NULL);
+		      *tp = pre;
+		      return;
+		    }
+		}
 	    }
-	  *tp = t;
-	}
-      break;
-
-    case ADDR_EXPR:
-      if (ctx)
-	lower_regimplify (tp, wi);
-      break;
-
-    case ARRAY_REF:
-    case ARRAY_RANGE_REF:
-    case REALPART_EXPR:
-    case IMAGPART_EXPR:
-    case COMPONENT_REF:
-    case VIEW_CONVERT_EXPR:
-      if (ctx)
-	lower_regimplify (tp, wi);
-      break;
 
-    case INDIRECT_REF:
-      if (ctx)
-	{
-	  wi->is_lhs = false;
-	  wi->val_only = true;
-	  lower_regimplify (&TREE_OPERAND (t, 0), wi);
+	  if (tsi == NULL)
+	    gimplify_stmt (tp);
+	  else
+	    {
+	      tree pre = NULL;
+	      gimplify_expr (tp, &pre, NULL, is_gimple_stmt, fb_none);
+	      if (pre)
+		tsi_link_before (tsi, pre, TSI_SAME_STMT);
+	    }
 	}
       break;
-
-    default:
-      if (!TYPE_P (t) && !DECL_P (t))
-	*walk_subtrees = 1;
-      break;
     }
-
-  return NULL_TREE;
 }
 
 static void
 lower_omp (tree *stmt_p, omp_context *ctx)
 {
-  struct walk_stmt_info wi;
-
-  memset (&wi, 0, sizeof (wi));
-  wi.callback = lower_omp_1;
-  wi.info = ctx;
-  wi.val_only = true;
-  wi.want_locations = true;
-
-  walk_stmts (&wi, stmt_p);
+  lower_omp_1 (stmt_p, ctx, NULL);
 }
 \f
 /* Main entry point.  */
--- libgomp/testsuite/libgomp.c++/pr35185.C.jj	2008-03-12 21:19:04.000000000 +0100
+++ libgomp/testsuite/libgomp.c++/pr35185.C	2008-03-12 21:12:03.000000000 +0100
@@ -0,0 +1,33 @@
+// PR middle-end/35185
+// { dg-do run }
+
+extern "C" void abort ();
+
+struct S
+{
+  S () : s (6) {}
+  ~S () {}
+  int s;
+};
+
+__attribute__((noinline))
+bool
+bar (S s)
+{
+  return s.s != 6;
+}
+
+int
+main ()
+{
+  S s;
+  int err = 0;
+#pragma omp parallel shared (s)
+  {
+    if (bar (s))
+      #pragma omp atomic
+	err++;
+  }
+  if (err)
+    abort ();
+}

	Jakub

Re: [PATCH] Fix lower_omp regimplification (PR middle-end/35185)

[Diego Novillo]

On Wed, Mar 12, 2008 at 14:01, Jakub Jelinek <jakub@redhat.com> wrote:

>  Diego, does this look ok to you?

Yes, thanks for fixing this.


Diego.