diff mbox series

OpenMP: Fix tmp-var handling with tree-nested.c [PR93553]

Message ID b9aa7398-26fe-b048-afe7-5c9749f6c0c9@codesourcery.com
State New
Headers show
Series OpenMP: Fix tmp-var handling with tree-nested.c [PR93553] | expand

Commit Message

Tobias Burnus July 20, 2020, 6:35 p.m. UTC 
This is about a PARAM_DECL of a procedure whose
internal/nested procedure uses this inside an
omp parallel. This leads to the code:

D.3940 = x;
(*D.3940)[D.3924] = …;

And the temporary variable "D.3940" introduced for
the nesting was not recorded as DECL for OpenMP,
leading to the ICE in scan_omp_1_op as shown in
the ICE.

This patch adds those temporary variables as PRIVATE
to the clause – fixing the ICE.

OK for the trunk?

Tobias

PS: For other other variables, that's done with
gimplify.c which adds it to the splay_tree, hence,
this issue only occurs if the variable is added later
– as here for tree-nested.c.
The new code is also used for some C OpenMP/OpenACC testcases
in the testsuite, but seemingly it works either way.
Interestingly, I do not see any 'private' in the dump for
the new testcase – seemingly, it gets optimized away.

PPS: Thanks for Jakub for some suggestions!

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter

Comments

Jakub Jelinek July 20, 2020, 7:12 p.m. UTC | #1 
On Mon, Jul 20, 2020 at 08:35:56PM +0200, Tobias Burnus wrote:
> gcc/ChangeLog:
> 
> 	PR fortran/93553
> 	* tree-nested.c (omp_new_clauses): New global var.
> 	(convert_nonlocal_reference_op): Add init_tmp_var/init_tmp_var
> 	vars to it.
> 	(convert_nonlocal_omp_clauses, convert_local_omp_clauses): Add
> 	those as 'private' to the OpenMP clause.

> --- a/gcc/tree-nested.c
> +++ b/gcc/tree-nested.c
> @@ -111,6 +111,7 @@ struct nesting_info
>    char static_chain_added;
>  };
>  
> +static tree omp_new_clauses;

I don't like this global variable.
Can you please instead stick it into struct nesting_info and make sure it is
cleared where it is allocated?

	Jakub
Tobias Burnus July 20, 2020, 9:20 p.m. UTC | #2 
On 7/20/20 9:12 PM, Jakub Jelinek wrote:
> I don't like this global variable.
> Can you please instead stick it into struct nesting_info and make sure it is
> cleared where it is allocated?

Done. The existing code uses
    struct nesting_info *info = XCNEW (struct nesting_info);
in create_nesting_tree; hence, the clearing is already done.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Tobias Burnus Aug. 3, 2020, 4:01 p.m. UTC | #3 
On 7/20/20 11:20 PM, Tobias Burnus wrote:
> On 7/20/20 9:12 PM, Jakub Jelinek wrote:
>> I don't like this global variable.
>> Can you please instead stick it into struct nesting_info and make
>> sure it is
>> cleared where it is allocated?
>
> Done. The existing code uses
>    struct nesting_info *info = XCNEW (struct nesting_info);
> in create_nesting_tree; hence, the clearing is already done.
>
> Tobias
>
-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter
Jakub Jelinek Aug. 3, 2020, 5:44 p.m. UTC | #4 
On Mon, Aug 03, 2020 at 06:01:40PM +0200, Tobias Burnus wrote:
> On 7/20/20 11:20 PM, Tobias Burnus wrote:
> > On 7/20/20 9:12 PM, Jakub Jelinek wrote:
> > > I don't like this global variable.
> > > Can you please instead stick it into struct nesting_info and make
> > > sure it is
> > > cleared where it is allocated?
> > 
> > Done. The existing code uses
> >    struct nesting_info *info = XCNEW (struct nesting_info);
> > in create_nesting_tree; hence, the clearing is already done.

Sorry for the delay, wanted to look at it in more detail and didn't get to
it until now.
I think the patch as posted isn't the best thing to do, one thing is that
it will create the clauses even when OpenMP isn't enabled or the current
location isn't nested in an OpenMP region, or even when it is, but it isn't
inside of the clauses that contain gimple sequences.
I don't understand why convert_local_* has been changed at all.
Now, I guess one could get around that by having the
convert_nonlocal_*clauses* function set the pointer to address of a local
variable when it will deal with this and keep NULL otherwise and only add
clauses in those cases.
But it seems better to me to follow what we do in all the other cases.
E.g. if the outer function's PARM_DECL is referenced inside of OMP_PARALLEL
body, then it works fine because OMP_PARALLEL handling does:
      save_local_var_chain = info->new_local_var_chain;
      info->new_local_var_chain = NULL;
before the body walk and
      if (info->new_local_var_chain)
        declare_vars (info->new_local_var_chain,
                      gimple_seq_first_stmt (gimple_omp_body (stmt)),
                      false);
      info->new_local_var_chain = save_local_var_chain;
afterwards, which means we don't really need any extra clauses, because the
temporaries will be declared inside of the parallel body rather than at some
outer scope.
I think we want to follow the suit with all the walks for clauses that
contain gimple sequences, so around the
OMP_CLAUSE_REDUCTION_GIMPLE_INIT
walk, around the
OMP_CLAUSE_REDUCTION_GIMPLE_MERGE
walk, around
OMP_CLAUSE_LASTPRIVATE_GIMPLE_SEQ
and
OMP_CLAUSE_LINEAR_GIMPLE_SEQ.
And probably both in the convert_nonlocal* case and in the convert_local*
case too.

Can you try that?  Or do you want me to try?

	Jakub
diff mbox series

Patch

OpenMP: Fix tmp-var handling with tree-nested.c [PR93553]

gcc/ChangeLog:

	PR fortran/93553
	* tree-nested.c (omp_new_clauses): New global var.
	(convert_nonlocal_reference_op): Add init_tmp_var/init_tmp_var
	vars to it.
	(convert_nonlocal_omp_clauses, convert_local_omp_clauses): Add
	those as 'private' to the OpenMP clause.

libgomp/ChangeLog:

	PR fortran/93553
	* testsuite/libgomp.fortran/pr93553.f90: New test.

 gcc/tree-nested.c                             | 37 +++++++++++++++++++++++++--
 libgomp/testsuite/libgomp.fortran/pr93553.f90 | 21 +++++++++++++++
 2 files changed, 56 insertions(+), 2 deletions(-)

diff --git a/gcc/tree-nested.c b/gcc/tree-nested.c
index 4dc5533be84..8c4dadc9f21 100644
--- a/gcc/tree-nested.c
+++ b/gcc/tree-nested.c
@@ -111,6 +111,7 @@  struct nesting_info
   char static_chain_added;
 };
 
+static tree omp_new_clauses;
 
 /* Iterate over the nesting tree, starting with ROOT, depth first.  */
 
@@ -1068,6 +1069,11 @@  convert_nonlocal_reference_op (tree *tp, int *walk_subtrees, void *data)
 	    if (use_pointer_in_frame (t))
 	      {
 		x = init_tmp_var (info, x, &wi->gsi);
+		tree c = build_omp_clause (DECL_SOURCE_LOCATION (x),
+					   OMP_CLAUSE_PRIVATE);
+		OMP_CLAUSE_DECL (c) = x;
+		OMP_CLAUSE_CHAIN (c) = omp_new_clauses;
+		omp_new_clauses = c;
 		x = build_simple_mem_ref_notrap (x);
 	      }
 	  }
@@ -1078,6 +1084,11 @@  convert_nonlocal_reference_op (tree *tp, int *walk_subtrees, void *data)
 	      x = save_tmp_var (info, x, &wi->gsi);
 	    else
 	      x = init_tmp_var (info, x, &wi->gsi);
+	    tree c = build_omp_clause (DECL_SOURCE_LOCATION (x),
+				       OMP_CLAUSE_PRIVATE);
+	    OMP_CLAUSE_DECL (c) = x;
+	    OMP_CLAUSE_CHAIN (c) = omp_new_clauses;
+	    omp_new_clauses = c;
 	  }
 
 	*tp = x;
@@ -1186,15 +1197,18 @@  convert_nonlocal_omp_clauses (tree *pclauses, struct walk_stmt_info *wi)
 {
   struct nesting_info *const info = (struct nesting_info *) wi->info;
   bool need_chain = false, need_stmts = false;
-  tree clause, decl, *pdecl;
+  tree clause, last_clause, decl, *pdecl;
   int dummy;
   bitmap new_suppress;
 
+  tree saved_omp_new_clauses = omp_new_clauses;
+  omp_new_clauses = NULL_TREE;
   new_suppress = BITMAP_GGC_ALLOC ();
   bitmap_copy (new_suppress, info->suppress_expansion);
 
   for (clause = *pclauses; clause ; clause = OMP_CLAUSE_CHAIN (clause))
     {
+      last_clause = clause;
       pdecl = NULL;
       switch (OMP_CLAUSE_CODE (clause))
 	{
@@ -1450,6 +1464,14 @@  convert_nonlocal_omp_clauses (tree *pclauses, struct walk_stmt_info *wi)
 	  break;
 	}
 
+
+  if (omp_new_clauses)
+    {
+      gcc_assert (*pclauses);
+      OMP_CLAUSE_CHAIN (last_clause) = omp_new_clauses;
+    }
+  omp_new_clauses = saved_omp_new_clauses;
+
   return need_chain;
 }
 
@@ -1919,15 +1941,19 @@  convert_local_omp_clauses (tree *pclauses, struct walk_stmt_info *wi)
 {
   struct nesting_info *const info = (struct nesting_info *) wi->info;
   bool need_frame = false, need_stmts = false;
-  tree clause, decl, *pdecl;
+  tree clause, last_clause, decl, *pdecl;
   int dummy;
   bitmap new_suppress;
 
+  tree saved_omp_new_clauses = omp_new_clauses;
+  omp_new_clauses = NULL_TREE;
+
   new_suppress = BITMAP_GGC_ALLOC ();
   bitmap_copy (new_suppress, info->suppress_expansion);
 
   for (clause = *pclauses; clause ; clause = OMP_CLAUSE_CHAIN (clause))
     {
+      last_clause = clause;
       pdecl = NULL;
       switch (OMP_CLAUSE_CODE (clause))
 	{
@@ -2193,6 +2219,13 @@  convert_local_omp_clauses (tree *pclauses, struct walk_stmt_info *wi)
 	  break;
 	}
 
+  if (omp_new_clauses)
+    {
+      gcc_assert (*pclauses);
+      OMP_CLAUSE_CHAIN (last_clause) = omp_new_clauses;
+    }
+  omp_new_clauses = saved_omp_new_clauses;
+
   return need_frame;
 }
 
diff --git a/libgomp/testsuite/libgomp.fortran/pr93553.f90 b/libgomp/testsuite/libgomp.fortran/pr93553.f90
new file mode 100644
index 00000000000..5d6f10febed
--- /dev/null
+++ b/libgomp/testsuite/libgomp.fortran/pr93553.f90
@@ -0,0 +1,21 @@ 
+program p
+   implicit none
+   integer :: x(8) = 0
+   call sub(x)
+end
+subroutine sub(x)
+   implicit none
+   integer i
+   integer :: x(8)
+   integer :: c(8) = [(11*i, i=1,8)]
+   call s
+   if (any (x /= c)) stop 1
+contains
+   subroutine s
+      integer :: i
+      !$omp parallel do reduction(+:x)
+      do i = 1, 8
+         x(i) = c(i)
+      end do
+   end
+end