Re: [GCC 9 RM attention] Re: [OpenACC] Update OpenACC data clause semantics to the 2.5 behavior

2019-04-30 Thread Thomas Schwinge
Hi Jakub!

On Mon, 29 Apr 2019 12:59:03 +0200, Jakub Jelinek  wrote:
> On Mon, Apr 29, 2019 at 12:18:15PM +0200, Thomas Schwinge wrote:
> > I know it's very late in the GCC 9 release process, but I'm still
> > catching up with OpenACC patch review, and just at the end of last week
> > I'd noticed that this commit (trunk r261813):
> 
> I'd like to see full patch before considering.  The bar at this point for
> anything to gcc-9-branch is very high.

Sure, understood.


I've spent some time thinking about this ABI change, back and forth, and
even produced the obvious patch (to use 'GOACC_FLAG_IF_PRESENT',
'GOACC_FLAG_FINALIZE'; attached for posterity), but in the end I'd like
to withdraw, and keep the current ABI.  (..., and I'll instead follow up,
at a later point in time, with some documentation updates and code
restructuring, to make this better understandable/maintainable.)


Grüße
 Thomas


From 46913aca5aa2dc24b04bd9012b8506bf767b442e Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Mon, 29 Apr 2019 21:25:41 +0200
Subject: [PATCH] [WIP] 'GOACC_FLAG_IF_PRESENT', 'GOACC_FLAG_FINALIZE'

TODO Adjust tree-scanning test cases.
---
 gcc/gimplify.c   | 47 
 gcc/omp-expand.c | 22 +++
 include/gomp-constants.h |  4 
 libgomp/oacc-parallel.c  | 25 ++---
 4 files changed, 29 insertions(+), 69 deletions(-)

diff --git a/gcc/gimplify.c b/gcc/gimplify.c
index e59f38261c36..44399b1af0d0 100644
--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -11811,53 +11811,6 @@ gimplify_omp_target_update (tree *expr_p, gimple_seq *pre_p)
 			 ort, TREE_CODE (expr));
   gimplify_adjust_omp_clauses (pre_p, NULL, &OMP_STANDALONE_CLAUSES (expr),
 			   TREE_CODE (expr));
-  if (TREE_CODE (expr) == OACC_UPDATE
-  && omp_find_clause (OMP_STANDALONE_CLAUSES (expr),
-			  OMP_CLAUSE_IF_PRESENT))
-{
-  /* The runtime uses GOMP_MAP_{TO,FROM} to denote the if_present
-	 clause.  */
-  for (tree c = OMP_STANDALONE_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c))
-	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP)
-	  switch (OMP_CLAUSE_MAP_KIND (c))
-	{
-	case GOMP_MAP_FORCE_TO:
-	  OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TO);
-	  break;
-	case GOMP_MAP_FORCE_FROM:
-	  OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_FROM);
-	  break;
-	default:
-	  break;
-	}
-}
-  else if (TREE_CODE (expr) == OACC_EXIT_DATA
-	   && omp_find_clause (OMP_STANDALONE_CLAUSES (expr),
-			   OMP_CLAUSE_FINALIZE))
-{
-  /* Use GOMP_MAP_DELETE/GOMP_MAP_FORCE_FROM to denote that "finalize"
-	 semantics apply to all mappings of this OpenACC directive.  */
-  bool finalize_marked = false;
-  for (tree c = OMP_STANDALONE_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN (c))
-	if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP)
-	  switch (OMP_CLAUSE_MAP_KIND (c))
-	{
-	case GOMP_MAP_FROM:
-	  OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_FORCE_FROM);
-	  finalize_marked = true;
-	  break;
-	case GOMP_MAP_RELEASE:
-	  OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_DELETE);
-	  finalize_marked = true;
-	  break;
-	default:
-	  /* Check consistency: libgomp relies on the very first data
-		 mapping clause being marked, so make sure we did that before
-		 any other mapping clauses.  */
-	  gcc_assert (finalize_marked);
-	  break;
-	}
-}
   stmt = gimple_build_omp_target (NULL, kind, OMP_STANDALONE_CLAUSES (expr));
 
   gimplify_seq_add_stmt (pre_p, stmt);
diff --git a/gcc/omp-expand.c b/gcc/omp-expand.c
index 89a3a3232671..53dbadea4c76 100644
--- a/gcc/omp-expand.c
+++ b/gcc/omp-expand.c
@@ -7542,14 +7542,28 @@ expand_omp_target (struct omp_region *region)
 
   clauses = gimple_omp_target_clauses (entry_stmt);
 
+  /* By default, no GOACC_FLAGs are set.  */
+  int goacc_flags_i = 0;
+
+  if (omp_find_clause (clauses, OMP_CLAUSE_IF_PRESENT))
+{
+  gcc_checking_assert (gimple_omp_target_kind (entry_stmt)
+			   == GF_OMP_TARGET_KIND_OACC_UPDATE);
+  goacc_flags_i |= GOACC_FLAG_IF_PRESENT;
+}
+
+  if (omp_find_clause (clauses, OMP_CLAUSE_FINALIZE))
+{
+  gcc_checking_assert (gimple_omp_target_kind (entry_stmt)
+			   == GF_OMP_TARGET_KIND_OACC_ENTER_EXIT_DATA);
+  goacc_flags_i |= GOACC_FLAG_FINALIZE;
+}
+
   tree device = NULL_TREE;
   location_t device_loc = UNKNOWN_LOCATION;
   tree goacc_flags = NULL_TREE;
   if (is_gimple_omp_oacc (entry_stmt))
-{
-  /* By default, no GOACC_FLAGs are set.  */
-  goacc_flags = integer_zero_node;
-}
+goacc_flags = build_int_cst (integer_type_node, goacc_flags_i);
   else
 {
   c = omp_find_clause (clauses, OMP_CLAUSE_DEVICE);
diff --git a/include/gomp-constants.h b/include/gomp-constants.h
index 8b93634f1b8b..8808c343034e 100644
--- a/include/gomp-constants.h
+++ b/include/gomp-constants.h
@@ -202,6 +202,10 @@ enum gomp_map_kind
 
 /* Force host fallback execution.  */
 #define GOACC_FLAG_HOST_FALLBACK	(1 <<

Re: [GCC 9 RM attention] Re: [OpenACC] Update OpenACC data clause semantics to the 2.5 behavior

2019-04-29 Thread Jakub Jelinek
On Mon, Apr 29, 2019 at 12:18:15PM +0200, Thomas Schwinge wrote:
> I know it's very late in the GCC 9 release process, but I'm still
> catching up with OpenACC patch review, and just at the end of last week
> I'd noticed that this commit (trunk r261813):

I'd like to see full patch before considering.  The bar at this point for
anything to gcc-9-branch is very high.

Jakub


[GCC 9 RM attention] Re: [OpenACC] Update OpenACC data clause semantics to the 2.5 behavior

2019-04-29 Thread Thomas Schwinge
Hi Jakub!

I know it's very late in the GCC 9 release process, but I'm still
catching up with OpenACC patch review, and just at the end of last week
I'd noticed that this commit (trunk r261813):

On Fri, 25 May 2018 13:01:58 -0700, Cesar Philippidis  
wrote:
> This patch updates GCC's to support OpenACC 2.5's data clause semantics
> [and other stuff...]

... should get rid off the following special-case handling for the new
OpenACC 'if_present' and 'finalize' clauses added here:

--- a/gcc/gimplify.c
+++ b/gcc/gimplify.c
@@ -10817,6 +10807,53 @@ gimplify_omp_target_update (tree *expr_p, 
gimple_seq *pre_p)
   ort, TREE_CODE (expr));
   gimplify_adjust_omp_clauses (pre_p, NULL, &OMP_STANDALONE_CLAUSES (expr),
 TREE_CODE (expr));
+  if (TREE_CODE (expr) == OACC_UPDATE
+  && omp_find_clause (OMP_STANDALONE_CLAUSES (expr),
+ OMP_CLAUSE_IF_PRESENT))
+{
+  /* The runtime uses GOMP_MAP_{TO,FROM} to denote the if_present
+clause.  */
+  for (tree c = OMP_STANDALONE_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN 
(c))
+   if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP)
+ switch (OMP_CLAUSE_MAP_KIND (c))
+   {
+   case GOMP_MAP_FORCE_TO:
+ OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_TO);
+ break;
+   case GOMP_MAP_FORCE_FROM:
+ OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_FROM);
+ break;
+   default:
+ break;
+   }
+}
+  else if (TREE_CODE (expr) == OACC_EXIT_DATA
+  && omp_find_clause (OMP_STANDALONE_CLAUSES (expr),
+  OMP_CLAUSE_FINALIZE))
+{
+  /* Use GOMP_MAP_DELETE/GOMP_MAP_FORCE_FROM to denote that "finalize"
+semantics apply to all mappings of this OpenACC directive.  */
+  bool finalize_marked = false;
+  for (tree c = OMP_STANDALONE_CLAUSES (expr); c; c = OMP_CLAUSE_CHAIN 
(c))
+   if (OMP_CLAUSE_CODE (c) == OMP_CLAUSE_MAP)
+ switch (OMP_CLAUSE_MAP_KIND (c))
+   {
+   case GOMP_MAP_FROM:
+ OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_FORCE_FROM);
+ finalize_marked = true;
+ break;
+   case GOMP_MAP_RELEASE:
+ OMP_CLAUSE_SET_MAP_KIND (c, GOMP_MAP_DELETE);
+ finalize_marked = true;
+ break;
+   default:
+ /* Check consistency: libgomp relies on the very first data
+mapping clause being marked, so make sure we did that before
+any other mapping clauses.  */
+ gcc_assert (finalize_marked);
+ break;
+   }
+}
   stmt = gimple_build_omp_target (NULL, kind, OMP_STANDALONE_CLAUSES 
(expr));

   gimplify_seq_add_stmt (pre_p, stmt);

..., plus the corresponding decoding process on the libgomp side.

This should instead use the interface that I added in trunk r267448 'For
libgomp OpenACC entry points, redefine the "device" argument to "flags"'
to communicate such flags from the compiler to libgomp.

I'll cook up a patch, very localized to OpenACC code, which will remove
more lines of code that it'll be adding, but as it's an ABI change, it
needs to go into gcc-9-branch before the 9.1 release is made (planned for
end of week, as I've read).


Grüße
 Thomas


signature.asc
Description: PGP signature