diff mbox series

[v3,04/11] OpenMP/OpenACC: mapping group list-handling improvements

Message ID f2f84c39600751588e8cf4a7809f5644055fa727.1663101299.git.julian@codesourcery.com
State New
Headers show
Series OpenMP 5.0: Struct & mapping clause expansion rework | expand

Commit Message

Julian Brown Sept. 13, 2022, 9:01 p.m. UTC
This patch adjusts OpenMP/OpenACC clause list handling in a couple of
places, in preparation for the following mapping-clause expansion rework
patch. Firstly mapping groups are removed as a whole in the C and C++
front-ends when an error is detected, which avoids leaving "badly-formed"
mapping clause groups in the list.

Secondly, reindexing of the omp_mapping_group hashmap (during
omp_build_struct_sibling_lists) has been reimplemented, fixing some
tricky corner-cases where mapping groups are removed from a list at the
same time as it is being reordered.

Thirdly, code to check if a different clause on the same directive maps
the whole of a struct that we have a component mapping for (for example)
has been outlined, removing a bit of code duplication.

(These changes could be split into different patches if necessary.)

2022-09-13  Julian Brown  <julian@codesourcery.com>

gcc/
	* gimplify.cc (omp_group_last): Allow GOMP_MAP_ATTACH_DETACH after
	GOMP_MAP_STRUCT (for reindexing).
	(omp_gather_mapping_groups): Reimplement using...
	(omp_gather_mapping_groups_1): This new function.  Stop processing at
	GATHER_SENTINEL.
	(omp_group_base): Allow GOMP_MAP_TO_PSET without any following node.
	(omp_index_mapping_groups): Reimplement using...
	(omp_index_mapping_groups_1): This new function.  Handle
	REINDEX_SENTINEL.
	(omp_reindex_mapping_groups, omp_mapped_by_containing_struct): New
	functions.
	(omp_tsort_mapping_groups_1): Adjust handling of base group being the
	same as current group.  Use omp_mapped_by_containing_struct.
	(omp_build_struct_sibling_lists): Use omp_mapped_by_containing_struct
	and omp_reindex_mapping_groups.  Robustify group deletion for reordered
	lists.
	(gimplify_scan_omp_clauses): Update calls to
	omp_build_struct_sibling_lists.

gcc/c/
	* c-typeck.cc (c_finish_omp_clauses): Remove whole mapping node group
	on error.

gcc/cp/
	* semantics.cc (finish_omp_clauses): Likewise.
---
 gcc/c/c-typeck.cc   |  24 ++++-
 gcc/cp/semantics.cc |  26 ++++-
 gcc/gimplify.cc     | 227 +++++++++++++++++++++++++++++++-------------
 3 files changed, 209 insertions(+), 68 deletions(-)

Comments

Jakub Jelinek Sept. 14, 2022, 11:30 a.m. UTC | #1
On Tue, Sep 13, 2022 at 02:01:45PM -0700, Julian Brown wrote:
> @@ -9443,6 +9499,41 @@ omp_containing_struct (tree expr)
>    return expr;
>  }
>  

Missing function comment here.

> +static bool
> +omp_mapped_by_containing_struct (hash_map<tree_operand_hash,
> +					  omp_mapping_group *> *grpmap,
> +				 tree decl,
> +				 omp_mapping_group **mapped_by_group)

Otherwise LGTM.

	Jakub
diff mbox series

Patch

diff --git a/gcc/c/c-typeck.cc b/gcc/c/c-typeck.cc
index ee891ee33c2..7da8d70b3bd 100644
--- a/gcc/c/c-typeck.cc
+++ b/gcc/c/c-typeck.cc
@@ -14229,12 +14229,19 @@  c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	  break;
 	}
 
+  tree *grp_start_p = NULL, grp_sentinel = NULL_TREE;
+
   for (pc = &clauses, c = clauses; c ; c = *pc)
     {
       bool remove = false;
       bool need_complete = false;
       bool need_implicitly_determined = false;
 
+      /* We've reached the end of a list of expanded nodes.  Reset the group
+	 start pointer.  */
+      if (c == grp_sentinel)
+	grp_start_p = NULL;
+
       switch (OMP_CLAUSE_CODE (c))
 	{
 	case OMP_CLAUSE_SHARED:
@@ -14995,6 +15002,9 @@  c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	  t = OMP_CLAUSE_DECL (c);
 	  if (TREE_CODE (t) == TREE_LIST)
 	    {
+	      grp_start_p = pc;
+	      grp_sentinel = OMP_CLAUSE_CHAIN (c);
+
 	      if (handle_omp_array_sections (c, ort))
 		remove = true;
 	      else
@@ -15638,7 +15648,19 @@  c_finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	}
 
       if (remove)
-	*pc = OMP_CLAUSE_CHAIN (c);
+	{
+	  if (grp_start_p)
+	    {
+	      /* If we found a clause to remove, we want to remove the whole
+		 expanded group, otherwise gimplify
+		 (omp_resolve_clause_dependencies) can get confused.  */
+	      *grp_start_p = grp_sentinel;
+	      pc = grp_start_p;
+	      grp_start_p = NULL;
+	    }
+	  else
+	    *pc = OMP_CLAUSE_CHAIN (c);
+	}
       else
 	pc = &OMP_CLAUSE_CHAIN (c);
     }
diff --git a/gcc/cp/semantics.cc b/gcc/cp/semantics.cc
index ae7c8ea7b1f..7302d21fc54 100644
--- a/gcc/cp/semantics.cc
+++ b/gcc/cp/semantics.cc
@@ -6755,11 +6755,18 @@  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	  break;
 	}
 
+  tree *grp_start_p = NULL, grp_sentinel = NULL_TREE;
+
   for (pc = &clauses, c = clauses; c ; c = *pc)
     {
       bool remove = false;
       bool field_ok = false;
 
+      /* We've reached the end of a list of expanded nodes.  Reset the group
+	 start pointer.  */
+      if (c == grp_sentinel)
+	grp_start_p = NULL;
+
       switch (OMP_CLAUSE_CODE (c))
 	{
 	case OMP_CLAUSE_SHARED:
@@ -7985,6 +7992,9 @@  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	  t = OMP_CLAUSE_DECL (c);
 	  if (TREE_CODE (t) == TREE_LIST)
 	    {
+	      grp_start_p = pc;
+	      grp_sentinel = OMP_CLAUSE_CHAIN (c);
+
 	      if (handle_omp_array_sections (c, ort))
 		remove = true;
 	      else
@@ -8356,6 +8366,9 @@  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 		       && (OMP_CLAUSE_MAP_KIND (c)
 			   != GOMP_MAP_ATTACH_DETACH))
 		{
+		  grp_start_p = pc;
+		  grp_sentinel = OMP_CLAUSE_CHAIN (c);
+
 		  tree c2 = build_omp_clause (OMP_CLAUSE_LOCATION (c),
 					      OMP_CLAUSE_MAP);
 		  if (TREE_CODE (t) == COMPONENT_REF)
@@ -8766,7 +8779,18 @@  finish_omp_clauses (tree clauses, enum c_omp_region_type ort)
 	}
 
       if (remove)
-	*pc = OMP_CLAUSE_CHAIN (c);
+	{
+	  if (grp_start_p)
+	    {
+	      /* If we found a clause to remove, we want to remove the whole
+		 expanded group, otherwise gimplify can get confused.  */
+	      *grp_start_p = grp_sentinel;
+	      pc = grp_start_p;
+	      grp_start_p = NULL;
+	    }
+	  else
+	    *pc = OMP_CLAUSE_CHAIN (c);
+	}
       else
 	pc = &OMP_CLAUSE_CHAIN (c);
     }
diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index 7037fb4eb6c..c7998c2ccbd 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -9160,7 +9160,8 @@  omp_group_last (tree *start_p)
 	unsigned HOST_WIDE_INT num_mappings
 	  = tree_to_uhwi (OMP_CLAUSE_SIZE (c));
 	if (OMP_CLAUSE_MAP_KIND (nc) == GOMP_MAP_FIRSTPRIVATE_POINTER
-	    || OMP_CLAUSE_MAP_KIND (nc) == GOMP_MAP_FIRSTPRIVATE_REFERENCE)
+	    || OMP_CLAUSE_MAP_KIND (nc) == GOMP_MAP_FIRSTPRIVATE_REFERENCE
+	    || OMP_CLAUSE_MAP_KIND (nc) == GOMP_MAP_ATTACH_DETACH)
 	  grp_last_p = &OMP_CLAUSE_CHAIN (*grp_last_p);
 	for (unsigned i = 0; i < num_mappings; i++)
 	  grp_last_p = &OMP_CLAUSE_CHAIN (*grp_last_p);
@@ -9176,12 +9177,13 @@  omp_group_last (tree *start_p)
    associated GOMP_MAP_POINTER mappings).  Return a vector of omp_mapping_group
    if we have more than one such group, else return NULL.  */
 
-static vec<omp_mapping_group> *
-omp_gather_mapping_groups (tree *list_p)
+static void
+omp_gather_mapping_groups_1 (tree *list_p, vec<omp_mapping_group> *groups,
+			     tree gather_sentinel)
 {
-  vec<omp_mapping_group> *groups = new vec<omp_mapping_group> ();
-
-  for (tree *cp = list_p; *cp; cp = &OMP_CLAUSE_CHAIN (*cp))
+  for (tree *cp = list_p;
+       *cp && *cp != gather_sentinel;
+       cp = &OMP_CLAUSE_CHAIN (*cp))
     {
       if (OMP_CLAUSE_CODE (*cp) != OMP_CLAUSE_MAP)
 	continue;
@@ -9199,6 +9201,14 @@  omp_gather_mapping_groups (tree *list_p)
 
       cp = grp_last_p;
     }
+}
+
+static vec<omp_mapping_group> *
+omp_gather_mapping_groups (tree *list_p)
+{
+  vec<omp_mapping_group> *groups = new vec<omp_mapping_group> ();
+
+  omp_gather_mapping_groups_1 (list_p, groups, NULL_TREE);
 
   if (groups->length () > 0)
     return groups;
@@ -9247,7 +9257,8 @@  omp_group_base (omp_mapping_group *grp, unsigned int *chained,
       node = OMP_CLAUSE_CHAIN (node);
       if (node && OMP_CLAUSE_MAP_KIND (node) == GOMP_MAP_TO_PSET)
 	{
-	  gcc_assert (node != grp->grp_end);
+	  if (node == grp->grp_end)
+	    return *grp->grp_start;
 	  node = OMP_CLAUSE_CHAIN (node);
 	}
       if (node)
@@ -9345,18 +9356,22 @@  omp_group_base (omp_mapping_group *grp, unsigned int *chained,
 /* Given a vector of omp_mapping_groups, build a hash table so we can look up
    nodes by tree_operand_hash.  */
 
-static hash_map<tree_operand_hash, omp_mapping_group *> *
-omp_index_mapping_groups (vec<omp_mapping_group> *groups)
+static void
+omp_index_mapping_groups_1 (hash_map<tree_operand_hash,
+				     omp_mapping_group *> *grpmap,
+			    vec<omp_mapping_group> *groups,
+			    tree reindex_sentinel)
 {
-  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap
-    = new hash_map<tree_operand_hash, omp_mapping_group *>;
-
   omp_mapping_group *grp;
   unsigned int i;
+  bool reindexing = reindex_sentinel != NULL_TREE, above_hwm = false;
 
   FOR_EACH_VEC_ELT (*groups, i, grp)
     {
-      if (grp->deleted)
+      if (reindexing && *grp->grp_start == reindex_sentinel)
+	above_hwm = true;
+
+      if (reindexing && !above_hwm)
 	continue;
 
       tree fpp;
@@ -9378,8 +9393,7 @@  omp_index_mapping_groups (vec<omp_mapping_group> *groups)
 	     source instead.  FIXME.  */
 	  if (TREE_CODE (decl) == MEM_REF
 	      && integer_zerop (TREE_OPERAND (decl, 1)))
-	    decl = build1 (INDIRECT_REF, TREE_TYPE (decl),
-			   TREE_OPERAND (decl, 0));
+	    decl = build_fold_indirect_ref (TREE_OPERAND (decl, 0));
 
 	  omp_mapping_group **prev = grpmap->get (decl);
 
@@ -9408,7 +9422,7 @@  omp_index_mapping_groups (vec<omp_mapping_group> *groups)
 	continue;
 
       omp_mapping_group **prev = grpmap->get (fpp);
-      if (prev)
+      if (prev && *prev != grp)
 	{
 	  grp->sibling = (*prev)->sibling;
 	  (*prev)->sibling = grp;
@@ -9416,6 +9430,48 @@  omp_index_mapping_groups (vec<omp_mapping_group> *groups)
       else
 	grpmap->put (fpp, grp);
     }
+}
+
+static hash_map<tree_operand_hash, omp_mapping_group *> *
+omp_index_mapping_groups (vec<omp_mapping_group> *groups)
+{
+  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap
+    = new hash_map<tree_operand_hash, omp_mapping_group *>;
+
+  omp_index_mapping_groups_1 (grpmap, groups, NULL_TREE);
+
+  return grpmap;
+}
+
+/* Rebuild group map from partially-processed clause list (during
+   omp_build_struct_sibling_lists).  We have already processed nodes up until
+   a high-water mark (HWM).  This is a bit tricky because the list is being
+   reordered as it is scanned, but we know:
+
+   1. The list after HWM has not been touched yet, so we can reindex it safely.
+
+   2. The list before and including HWM has been altered, but remains
+      well-formed throughout the sibling-list building operation.
+
+   so, we can do the reindex operation in two parts, on the processed and
+   then the unprocessed halves of the list.  */
+
+static hash_map<tree_operand_hash, omp_mapping_group *> *
+omp_reindex_mapping_groups (tree *list_p,
+			    vec<omp_mapping_group> *groups,
+			    vec<omp_mapping_group> *processed_groups,
+			    tree sentinel)
+{
+  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap
+    = new hash_map<tree_operand_hash, omp_mapping_group *>;
+
+  processed_groups->truncate (0);
+
+  omp_gather_mapping_groups_1 (list_p, processed_groups, sentinel);
+  omp_index_mapping_groups_1 (grpmap, processed_groups, NULL_TREE);
+  if (sentinel)
+    omp_index_mapping_groups_1 (grpmap, groups, sentinel);
+
   return grpmap;
 }
 
@@ -9443,6 +9499,41 @@  omp_containing_struct (tree expr)
   return expr;
 }
 
+static bool
+omp_mapped_by_containing_struct (hash_map<tree_operand_hash,
+					  omp_mapping_group *> *grpmap,
+				 tree decl,
+				 omp_mapping_group **mapped_by_group)
+{
+  tree wsdecl = NULL_TREE;
+
+  *mapped_by_group = NULL;
+
+  while (true)
+    {
+      wsdecl = omp_containing_struct (decl);
+      if (wsdecl == decl)
+	break;
+      omp_mapping_group **wholestruct = grpmap->get (wsdecl);
+      if (!wholestruct
+	  && TREE_CODE (wsdecl) == MEM_REF
+	  && integer_zerop (TREE_OPERAND (wsdecl, 1)))
+	{
+	  tree deref = TREE_OPERAND (wsdecl, 0);
+	  deref = build_fold_indirect_ref (deref);
+	  wholestruct = grpmap->get (deref);
+	}
+      if (wholestruct)
+	{
+	  *mapped_by_group = *wholestruct;
+	  return true;
+	}
+      decl = wsdecl;
+    }
+
+  return false;
+}
+
 /* Helper function for omp_tsort_mapping_groups.  Returns TRUE on success, or
    FALSE on error.  */
 
@@ -9470,9 +9561,8 @@  omp_tsort_mapping_groups_1 (omp_mapping_group ***outlist,
     {
       omp_mapping_group **basep = grpmap->get (attaches_to);
 
-      if (basep)
+      if (basep && *basep != grp)
 	{
-	  gcc_assert (*basep != grp);
 	  for (omp_mapping_group *w = *basep; w; w = w->sibling)
 	    if (!omp_tsort_mapping_groups_1 (outlist, groups, grpmap, w))
 	      return false;
@@ -9489,25 +9579,16 @@  omp_tsort_mapping_groups_1 (omp_mapping_group ***outlist,
 	break;
 
       omp_mapping_group **innerp = grpmap->get (base);
+      omp_mapping_group *wholestruct;
 
       /* We should treat whole-structure mappings as if all (pointer, in this
 	 case) members are mapped as individual list items.  Check if we have
 	 such a whole-structure mapping, if we don't have an explicit reference
 	 to the pointer member itself.  */
-      if (!innerp && TREE_CODE (base) == COMPONENT_REF)
-	{
-	  base = omp_containing_struct (base);
-	  innerp = grpmap->get (base);
-
-	  if (!innerp
-	      && TREE_CODE (base) == MEM_REF
-	      && integer_zerop (TREE_OPERAND (base, 1)))
-	    {
-	      tree ind = TREE_OPERAND (base, 0);
-	      ind = build1 (INDIRECT_REF, TREE_TYPE (base), ind);
-	      innerp = grpmap->get (ind);
-	    }
-	}
+      if (!innerp
+	  && TREE_CODE (base) == COMPONENT_REF
+	  && omp_mapped_by_containing_struct (grpmap, base, &wholestruct))
+	innerp = &wholestruct;
 
       if (innerp && *innerp != grp)
 	{
@@ -10296,7 +10377,8 @@  omp_build_struct_sibling_lists (enum tree_code code,
 				enum omp_region_type region_type,
 				vec<omp_mapping_group> *groups,
 				hash_map<tree_operand_hash, omp_mapping_group *>
-				  **grpmap)
+				  **grpmap,
+				tree *list_p)
 {
   unsigned i;
   omp_mapping_group *grp;
@@ -10304,16 +10386,22 @@  omp_build_struct_sibling_lists (enum tree_code code,
   bool success = true;
   tree *new_next = NULL;
   tree *tail = &OMP_CLAUSE_CHAIN ((*groups)[groups->length () - 1].grp_end);
+  auto_vec<omp_mapping_group> pre_hwm_groups;
 
   FOR_EACH_VEC_ELT (*groups, i, grp)
     {
       tree c = grp->grp_end;
       tree decl = OMP_CLAUSE_DECL (c);
-      tree *grp_start_p = new_next ? new_next : grp->grp_start;
       tree grp_end = grp->grp_end;
+      tree sentinel = OMP_CLAUSE_CHAIN (grp_end);
+
+      if (new_next)
+	grp->grp_start = new_next;
 
       new_next = NULL;
 
+      tree *grp_start_p = grp->grp_start;
+
       if (DECL_P (decl))
 	continue;
 
@@ -10353,36 +10441,16 @@  omp_build_struct_sibling_lists (enum tree_code code,
       if (TREE_CODE (decl) != COMPONENT_REF)
 	continue;
 
-      omp_mapping_group **wholestruct = NULL;
-      tree wsdecl = omp_containing_struct (OMP_CLAUSE_DECL (c));
-
-      if (!(region_type & ORT_ACC) && wsdecl != OMP_CLAUSE_DECL (c))
-	{
-	  wholestruct = (*grpmap)->get (wsdecl);
-	  if (!wholestruct
-	      && TREE_CODE (wsdecl) == MEM_REF
-	      && integer_zerop (TREE_OPERAND (wsdecl, 1)))
-	    {
-	      tree deref = TREE_OPERAND (wsdecl, 0);
-	      deref = build1 (INDIRECT_REF, TREE_TYPE (wsdecl), deref);
-	      wholestruct = (*grpmap)->get (deref);
-	    }
-	}
-
-      if (wholestruct)
+      /* If we're mapping the whole struct in another node, skip creation of
+	 sibling lists.  */
+      omp_mapping_group *wholestruct;
+      if (!(region_type & ORT_ACC)
+	  && omp_mapped_by_containing_struct (*grpmap, OMP_CLAUSE_DECL (c),
+					      &wholestruct))
 	{
 	  if (*grp_start_p == grp_end)
-	    {
-	      /* Remove the whole of this mapping -- redundant.  */
-	      if (i + 1 < groups->length ())
-		{
-		  omp_mapping_group *nextgrp = &(*groups)[i + 1];
-		  nextgrp->grp_start = grp_start_p;
-		}
-	      grp->deleted = true;
-	      new_next = grp_start_p;
-	      *grp_start_p = OMP_CLAUSE_CHAIN (grp_end);
-	    }
+	    /* Remove the whole of this mapping -- redundant.  */
+	    grp->deleted = true;
 
 	  continue;
 	}
@@ -10427,7 +10495,6 @@  omp_build_struct_sibling_lists (enum tree_code code,
 		*tail = inner;
 
 	      OMP_CLAUSE_CHAIN (inner) = NULL_TREE;
-
 	      omp_mapping_group newgrp;
 	      newgrp.grp_start = new_next ? new_next : tail;
 	      newgrp.grp_end = inner;
@@ -10441,13 +10508,39 @@  omp_build_struct_sibling_lists (enum tree_code code,
 		 map.  Rebuild it here.  This is a bit inefficient, but
 		 shouldn't happen very often.  */
 	      delete (*grpmap);
-	      *grpmap = omp_index_mapping_groups (groups);
+	      *grpmap
+		= omp_reindex_mapping_groups (list_p, groups, &pre_hwm_groups,
+					      sentinel);
 
 	      tail = &OMP_CLAUSE_CHAIN (inner);
 	    }
 	}
     }
 
+  /* Delete groups marked for deletion above.  At this point the order of the
+     groups may no longer correspond to the order of the underlying list,
+     which complicates this a little.  First clear out OMP_CLAUSE_DECL for
+     deleted nodes...  */
+
+  FOR_EACH_VEC_ELT (*groups, i, grp)
+    if (grp->deleted)
+      for (tree d = *grp->grp_start;
+	   d != OMP_CLAUSE_CHAIN (grp->grp_end);
+	   d = OMP_CLAUSE_CHAIN (d))
+	OMP_CLAUSE_DECL (d) = NULL_TREE;
+
+  /* ...then sweep through the list removing the now-empty nodes.  */
+
+  tail = list_p;
+  while (*tail)
+    {
+      if (OMP_CLAUSE_CODE (*tail) == OMP_CLAUSE_MAP
+	  && OMP_CLAUSE_DECL (*tail) == NULL_TREE)
+	*tail = OMP_CLAUSE_CHAIN (*tail);
+      else
+	tail = &OMP_CLAUSE_CHAIN (*tail);
+    }
+
 error_out:
   if (struct_map_to_clause)
     delete struct_map_to_clause;
@@ -10508,7 +10601,8 @@  gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
 	  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap;
 	  grpmap = omp_index_mapping_groups (groups);
 
-	  omp_build_struct_sibling_lists (code, region_type, groups, &grpmap);
+	  omp_build_struct_sibling_lists (code, region_type, groups, &grpmap,
+					  list_p);
 
 	  omp_mapping_group *outlist = NULL;
 
@@ -10543,7 +10637,8 @@  gimplify_scan_omp_clauses (tree *list_p, gimple_seq *pre_p,
 	  hash_map<tree_operand_hash, omp_mapping_group *> *grpmap;
 	  grpmap = omp_index_mapping_groups (groups);
 
-	  omp_build_struct_sibling_lists (code, region_type, groups, &grpmap);
+	  omp_build_struct_sibling_lists (code, region_type, groups, &grpmap,
+					  list_p);
 
 	  delete groups;
 	  delete grpmap;