Re: [PATCH] Fix OpenMP/OpenACC/Cilk+ issues with forced/non-local labels (PR c/81687)

[Richard Biener <rguenther at suse dot de>]

On Fri, 4 Aug 2017, Jakub Jelinek wrote:

> Hi!
> 
> If there are forced or non-local labels in OpenMP parallel/task/target
> regions (or OpenACC or Cilk+), then we often fail to link as the following
> testcases show - the labels aren't emitted anywhere, just references to
> them.
> 
> While it is true that for forced/non-local labels we can't clone the basic
> blocks containing them, for OpenMP we actually aren't cloning it, but
> outlining the SESE region to a new function, i.e. moving the blocks.
> As such, we can move the label too.  It is UB if a goto to such a label
> (computed or non-local) is performed across boundaries of the region
> (from outside of the region into it or from inside of it to outside),
> but it should be ok to use it within the same region, and if just printing
> address or something similar we can even reference label in one function
> from another one (where one of them is outlined from the other or similar).
> 
> The following patch arranges for this - does not copy such labels during
> lowering, and ensures that their DECL_CONTEXT will be the function that
> contains the label (rather than just any reference to it).
> 
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

Ok.

Thanks,
Richard.

> 2017-08-04  Jakub Jelinek  <jakub@redhat.com>
> 
> 	PR c/81687
> 	* omp-low.c (omp_copy_decl): Don't remap FORCED_LABEL or DECL_NONLOCAL
> 	LABEL_DECLs.
> 	* tree-cfg.c (move_stmt_op): Don't adjust DECL_CONTEXT of FORCED_LABEL
> 	or DECL_NONLOCAL labels.
> 	(move_stmt_r) <case GIMPLE_LABEL>: Adjust DECL_CONTEXT of FORCED_LABEL
> 	or DECL_NONLOCAL labels here.
> 
> 	* testsuite/libgomp.c/pr81687-1.c: New test.
> 	* testsuite/libgomp.c/pr81687-2.c: New test.
> 
> --- gcc/omp-low.c.jj	2017-08-03 10:33:41.000000000 +0200
> +++ gcc/omp-low.c	2017-08-03 15:37:52.998873566 +0200
> @@ -796,6 +796,8 @@ omp_copy_decl (tree var, copy_body_data
>  
>    if (TREE_CODE (var) == LABEL_DECL)
>      {
> +      if (FORCED_LABEL (var) || DECL_NONLOCAL (var))
> +	return var;
>        new_var = create_artificial_label (DECL_SOURCE_LOCATION (var));
>        DECL_CONTEXT (new_var) = current_function_decl;
>        insert_decl_map (&ctx->cb, var, new_var);
> --- gcc/tree-cfg.c.jj	2017-07-29 09:48:43.000000000 +0200
> +++ gcc/tree-cfg.c	2017-08-03 15:37:40.395021370 +0200
> @@ -6718,7 +6718,15 @@ move_stmt_op (tree *tp, int *walk_subtre
>  		*tp = t = out->to;
>  	    }
>  
> -	  DECL_CONTEXT (t) = p->to_context;
> +	  /* For FORCED_LABELs we can end up with references from other
> +	     functions if some SESE regions are outlined.  It is UB to
> +	     jump in between them, but they could be used just for printing
> +	     addresses etc.  In that case, DECL_CONTEXT on the label should
> +	     be the function containing the glabel stmt with that LABEL_DECL,
> +	     rather than whatever function a reference to the label was seen
> +	     last time.  */
> +	  if (!FORCED_LABEL (t) && !DECL_NONLOCAL (t))
> +	    DECL_CONTEXT (t) = p->to_context;
>  	}
>        else if (p->remap_decls_p)
>  	{
> @@ -6836,6 +6844,21 @@ move_stmt_r (gimple_stmt_iterator *gsi_p
>      case GIMPLE_OMP_RETURN:
>      case GIMPLE_OMP_CONTINUE:
>        break;
> +
> +    case GIMPLE_LABEL:
> +      {
> +	/* For FORCED_LABEL, move_stmt_op doesn't adjust DECL_CONTEXT,
> +	   so that such labels can be referenced from other regions.
> +	   Make sure to update it when seeing a GIMPLE_LABEL though,
> +	   that is the owner of the label.  */
> +	walk_gimple_op (stmt, move_stmt_op, wi);
> +	*handled_ops_p = true;
> +	tree label = gimple_label_label (as_a <glabel *> (stmt));
> +	if (FORCED_LABEL (label) || DECL_NONLOCAL (label))
> +	  DECL_CONTEXT (label) = p->to_context;
> +      }
> +      break;
> +
>      default:
>        if (is_gimple_omp (stmt))
>  	{
> --- libgomp/testsuite/libgomp.c/pr81687-1.c.jj	2017-08-03 15:43:32.832888380 +0200
> +++ libgomp/testsuite/libgomp.c/pr81687-1.c	2017-08-03 15:43:06.000000000 +0200
> @@ -0,0 +1,23 @@
> +/* PR c/81687 */
> +/* { dg-do link } */
> +/* { dg-additional-options "-O2" } */
> +
> +extern int printf (const char *, ...);
> +
> +int
> +main ()
> +{
> +  #pragma omp parallel
> +  {
> +   lab1:
> +    printf ("lab1=%p\n", (void *)(&&lab1));
> +  }
> + lab2:
> +  #pragma omp parallel
> +  {
> +   lab3:
> +    printf ("lab2=%p\n", (void *)(&&lab2));
> +  }
> +  printf ("lab3=%p\n", (void *)(&&lab3));
> +  return 0;
> +}
> --- libgomp/testsuite/libgomp.c/pr81687-2.c.jj	2017-08-03 15:43:36.229848545 +0200
> +++ libgomp/testsuite/libgomp.c/pr81687-2.c	2017-08-03 15:43:15.000000000 +0200
> @@ -0,0 +1,27 @@
> +/* PR c/81687 */
> +/* { dg-do link } */
> +/* { dg-additional-options "-O2" } */
> +
> +int
> +main ()
> +{
> +  __label__ lab4, lab5, lab6;
> +  volatile int l = 0;
> +  int m = l;
> +  void foo (int x) { if (x == 1) goto lab4; }
> +  void bar (int x) { if (x == 2) goto lab5; }
> +  void baz (int x) { if (x == 3) goto lab6; }
> +  #pragma omp parallel
> +  {
> +    foo (m + 1);
> +   lab4:;
> +  }
> +  #pragma omp task
> +  {
> +    bar (m + 2);
> +   lab5:;
> +  }
> +  baz (m + 3);
> + lab6:;
> +  return 0;
> +}
> 
> 	Jakub
> 
> 

-- 
Richard Biener <rguenther@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)