diff mbox

Fix OpenMP ICE due to fold_stmt (PR middle-end/66820)

Message ID 20150709212330.GG10247@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek July 9, 2015, 9:23 p.m. UTC
Hi!

As discussed on IRC, we have a problem because fold_stmt can call
force_gimple_operand_1, which in turn does
push_gimplify_context/pop_gimplify_context (NULL), even when inside
of some gimplify_ctxp and thus pushes the decls into function context
rather than the current gimplify_ctxp.

So far I've bootstrapped/regtested this version, which supposedly could be
useful for the 5 branch.  Other options are to avoid
push_gimplify_context/pop_gimplify_context in force_gimple_operand_1, if
gimplify_ctxp is already non-NULL when it is called (the problem with that
solution is that gimplify_ctxp is now private in gimplify.c and
force_gimple_operand_1 lives somewhere else, so we'd need some accessor
function for that or something).  Or avoid force_gimple_operand* from
fold_stmt.

2015-07-09  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/66820
	* gimplify.c (maybe_fold_stmt): Don't fold in ORT_PARALLEL
	or ORT_TASK contexts.
	* omp-low.c (lower_omp): Call fold_stmt even if taskreg_nesting_level
	is non-zero.

	* gcc.dg/gomp/pr66820.c: New test.


	Jakub

Comments

Richard Biener July 10, 2015, 7:28 a.m. UTC | #1
On Thu, 9 Jul 2015, Jakub Jelinek wrote:

> Hi!
> 
> As discussed on IRC, we have a problem because fold_stmt can call
> force_gimple_operand_1, which in turn does
> push_gimplify_context/pop_gimplify_context (NULL), even when inside
> of some gimplify_ctxp and thus pushes the decls into function context
> rather than the current gimplify_ctxp.
> 
> So far I've bootstrapped/regtested this version, which supposedly could be
> useful for the 5 branch.  Other options are to avoid
> push_gimplify_context/pop_gimplify_context in force_gimple_operand_1, if
> gimplify_ctxp is already non-NULL when it is called (the problem with that
> solution is that gimplify_ctxp is now private in gimplify.c and
> force_gimple_operand_1 lives somewhere else, so we'd need some accessor
> function for that or something).  Or avoid force_gimple_operand* from
> fold_stmt.

I think this is ok for now also on trunk.  Yes, I think we should
simply disallow pushing gimplifier contexts onto others (not sure
where that triggers...).  Thus definitely move away from
using force_gimple_operand in the middle-end (which is my plan
with match-and-simplify anyway).

Thanks,
Richard.

> 2015-07-09  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR middle-end/66820
> 	* gimplify.c (maybe_fold_stmt): Don't fold in ORT_PARALLEL
> 	or ORT_TASK contexts.
> 	* omp-low.c (lower_omp): Call fold_stmt even if taskreg_nesting_level
> 	is non-zero.
> 
> 	* gcc.dg/gomp/pr66820.c: New test.
> 
> --- gcc/gimplify.c.jj	2015-07-08 19:50:04.000000000 +0200
> +++ gcc/gimplify.c	2015-07-09 16:36:58.336609740 +0200
> @@ -2245,16 +2245,17 @@ gimplify_arg (tree *arg_p, gimple_seq *p
>    return gimplify_expr (arg_p, pre_p, NULL, test, fb);
>  }
>  
> -/* Don't fold inside offloading regions: it can break code by adding decl
> -   references that weren't in the source.  We'll do it during omplower pass
> -   instead.  */
> +/* Don't fold inside offloading or taskreg regions: it can break code by
> +   adding decl references that weren't in the source.  We'll do it during
> +   omplower pass instead.  */
>  
>  static bool
>  maybe_fold_stmt (gimple_stmt_iterator *gsi)
>  {
>    struct gimplify_omp_ctx *ctx;
>    for (ctx = gimplify_omp_ctxp; ctx; ctx = ctx->outer_context)
> -    if (ctx->region_type == ORT_TARGET)
> +    if (ctx->region_type == ORT_TARGET
> +	|| (ctx->region_type & (ORT_PARALLEL | ORT_TASK)) != 0)
>        return false;
>    return fold_stmt (gsi);
>  }
> --- gcc/omp-low.c.jj	2015-07-08 19:49:39.000000000 +0200
> +++ gcc/omp-low.c	2015-07-09 16:37:43.169942838 +0200
> @@ -11890,8 +11890,8 @@ lower_omp (gimple_seq *body, omp_context
>    for (gsi = gsi_start (*body); !gsi_end_p (gsi); gsi_next (&gsi))
>      lower_omp_1 (&gsi, ctx);
>    /* During gimplification, we haven't folded statments inside offloading
> -     regions (gimplify.c:maybe_fold_stmt); do that now.  */
> -  if (target_nesting_level)
> +     or taskreg regions (gimplify.c:maybe_fold_stmt); do that now.  */
> +  if (target_nesting_level || taskreg_nesting_level)
>      for (gsi = gsi_start (*body); !gsi_end_p (gsi); gsi_next (&gsi))
>        fold_stmt (&gsi);
>    input_location = saved_location;
> --- gcc/testsuite/gcc.dg/gomp/pr66820.c.jj	2015-04-23 09:27:31.812958582 +0200
> +++ gcc/testsuite/gcc.dg/gomp/pr66820.c	2015-07-09 17:16:34.762225577 +0200
> @@ -0,0 +1,18 @@
> +/* PR middle-end/66820 */
> +/* { dg-do compile } */
> +/* { dg-options "-fopenmp" } */
> +
> +void bar (char *);
> +
> +void
> +foo (char **x)
> +{
> +#pragma omp parallel for
> +  for (int i = 0; i < 16; i++)
> +    {
> +      char y[50];
> +      __builtin_strcpy (y, x[i]);
> +      __builtin_strcat (y, "foo");
> +      bar (y);
> +    }
> +}
> 
> 	Jakub
> 
>
diff mbox

Patch

--- gcc/gimplify.c.jj	2015-07-08 19:50:04.000000000 +0200
+++ gcc/gimplify.c	2015-07-09 16:36:58.336609740 +0200
@@ -2245,16 +2245,17 @@  gimplify_arg (tree *arg_p, gimple_seq *p
   return gimplify_expr (arg_p, pre_p, NULL, test, fb);
 }
 
-/* Don't fold inside offloading regions: it can break code by adding decl
-   references that weren't in the source.  We'll do it during omplower pass
-   instead.  */
+/* Don't fold inside offloading or taskreg regions: it can break code by
+   adding decl references that weren't in the source.  We'll do it during
+   omplower pass instead.  */
 
 static bool
 maybe_fold_stmt (gimple_stmt_iterator *gsi)
 {
   struct gimplify_omp_ctx *ctx;
   for (ctx = gimplify_omp_ctxp; ctx; ctx = ctx->outer_context)
-    if (ctx->region_type == ORT_TARGET)
+    if (ctx->region_type == ORT_TARGET
+	|| (ctx->region_type & (ORT_PARALLEL | ORT_TASK)) != 0)
       return false;
   return fold_stmt (gsi);
 }
--- gcc/omp-low.c.jj	2015-07-08 19:49:39.000000000 +0200
+++ gcc/omp-low.c	2015-07-09 16:37:43.169942838 +0200
@@ -11890,8 +11890,8 @@  lower_omp (gimple_seq *body, omp_context
   for (gsi = gsi_start (*body); !gsi_end_p (gsi); gsi_next (&gsi))
     lower_omp_1 (&gsi, ctx);
   /* During gimplification, we haven't folded statments inside offloading
-     regions (gimplify.c:maybe_fold_stmt); do that now.  */
-  if (target_nesting_level)
+     or taskreg regions (gimplify.c:maybe_fold_stmt); do that now.  */
+  if (target_nesting_level || taskreg_nesting_level)
     for (gsi = gsi_start (*body); !gsi_end_p (gsi); gsi_next (&gsi))
       fold_stmt (&gsi);
   input_location = saved_location;
--- gcc/testsuite/gcc.dg/gomp/pr66820.c.jj	2015-04-23 09:27:31.812958582 +0200
+++ gcc/testsuite/gcc.dg/gomp/pr66820.c	2015-07-09 17:16:34.762225577 +0200
@@ -0,0 +1,18 @@ 
+/* PR middle-end/66820 */
+/* { dg-do compile } */
+/* { dg-options "-fopenmp" } */
+
+void bar (char *);
+
+void
+foo (char **x)
+{
+#pragma omp parallel for
+  for (int i = 0; i < 16; i++)
+    {
+      char y[50];
+      __builtin_strcpy (y, x[i]);
+      __builtin_strcat (y, "foo");
+      bar (y);
+    }
+}