diff mbox

[hsa] Make copy_gimple_seq_and_replace_locals copy seqs in omp clauses

Message ID 20151203182619.GH3155@virgil.suse.cz
State New
Headers show

Commit Message

Martin Jambor Dec. 3, 2015, 6:26 p.m. UTC 
Hi,

this is a fix to the last "last" ICE of the hsa branch.  THe problem
turned out not to be in the gridification itself but, depending your
point of view, in the gimple and tree walking infrastructure or in
function copy_gimple_seq_and_replace_locals from tree-inline.c on
which hsa gridification relies.

The issue is that in between gimplification and omplow pass, there can
be gimple sequences attached to OMP_CLAUSE trees that are attached to
omp statements and that are neither copied by gimple_seq_copy nor
walked by walk_gimple_seq.

While the correct solution would probably be to extend tree and gimple
walkers to handle them, that would be a big change.  I have talked
with Jakub about this yesterday on the IRC and he suggested that I
enhance the internal walkers of copy_gimple_seq_and_replace_locals
deal with this situation.  Even though that leaves gimple_seq_copy,
walk_gimple_seq and other to be technically incorrect, that is what I
have done in the patch below, which fixes my last ICEs and which I
have already committed to the branch.

Any feedback is of course very much appreciated,

Martin


2015-12-03  Martin Jambor  <mjambor@suse.cz>

	* tree-inline.c (duplicate_remap_omp_clause_seq): New function.
	(replace_locals_op): Duplicate gimple sequences in OMP clauses.

---
 gcc/tree-inline.c | 43 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 43 insertions(+)

Comments

Jakub Jelinek Dec. 3, 2015, 6:36 p.m. UTC | #1 
On Thu, Dec 03, 2015 at 07:26:20PM +0100, Martin Jambor wrote:
> this is a fix to the last "last" ICE of the hsa branch.  THe problem
> turned out not to be in the gridification itself but, depending your
> point of view, in the gimple and tree walking infrastructure or in
> function copy_gimple_seq_and_replace_locals from tree-inline.c on
> which hsa gridification relies.
> 
> The issue is that in between gimplification and omplow pass, there can
> be gimple sequences attached to OMP_CLAUSE trees that are attached to
> omp statements and that are neither copied by gimple_seq_copy nor
> walked by walk_gimple_seq.
> 
> While the correct solution would probably be to extend tree and gimple
> walkers to handle them, that would be a big change.  I have talked
> with Jakub about this yesterday on the IRC and he suggested that I
> enhance the internal walkers of copy_gimple_seq_and_replace_locals
> deal with this situation.  Even though that leaves gimple_seq_copy,
> walk_gimple_seq and other to be technically incorrect, that is what I
> have done in the patch below, which fixes my last ICEs and which I
> have already committed to the branch.

The point is that those gimple_seqs are there only from gimplification
till omplower, and I believe nothing else for now cares about those.
> @@ -5200,6 +5231,18 @@ replace_locals_stmt (gimple_stmt_iterator *gsip,
>    return NULL_TREE;
>  }
>  
> +/* Create a copy of SEQ and remap all decls in it.  */
> +
> +static gimple_seq
> +duplicate_remap_omp_clause_seq (gimple_seq seq, struct walk_stmt_info *wi)
> +{

I would have expected an early if (seq == NULL) return NULL; either here,
or in the callers (not doing anything in the common case when it is NULL).

	Jakub
diff mbox

Patch

diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
index ebab189..15141dc 100644
--- a/gcc/tree-inline.c
+++ b/gcc/tree-inline.c
@@ -5116,6 +5116,8 @@  mark_local_labels_stmt (gimple_stmt_iterator *gsip,
   return NULL_TREE;
 }
 
+static gimple_seq duplicate_remap_omp_clause_seq (gimple_seq seq,
+						  struct walk_stmt_info *wi);
 
 /* Called via walk_gimple_seq by copy_gimple_seq_and_replace_local.
    Using the splay_tree pointed to by ST (which is really a `splay_tree'),
@@ -5160,6 +5162,35 @@  replace_locals_op (tree *tp, int *walk_subtrees, void *data)
 	  TREE_OPERAND (expr, 3) = NULL_TREE;
 	}
     }
+  else if (TREE_CODE (expr) == OMP_CLAUSE)
+    {
+      /* Before the omplower pass completes, some OMP clauses can contain
+	 sequences that are neither copied by gimple_seq_copy nor walked by
+	 walk_gimple_seq.  To make copy_gimple_seq_and_replace_locals work even
+	 in those situations, we have to copy and process them explicitely.  */
+
+      if (OMP_CLAUSE_CODE (expr) == OMP_CLAUSE_LASTPRIVATE)
+	{
+	  gimple_seq seq = OMP_CLAUSE_LASTPRIVATE_GIMPLE_SEQ (expr);
+	  seq = duplicate_remap_omp_clause_seq (seq, wi);
+	  OMP_CLAUSE_LASTPRIVATE_GIMPLE_SEQ (expr) = seq;
+	}
+      else if (OMP_CLAUSE_CODE (expr) == OMP_CLAUSE_LINEAR)
+	{
+	  gimple_seq seq = OMP_CLAUSE_LINEAR_GIMPLE_SEQ (expr);
+	  seq = duplicate_remap_omp_clause_seq (seq, wi);
+	  OMP_CLAUSE_LINEAR_GIMPLE_SEQ (expr) = seq;
+	}
+      else if (OMP_CLAUSE_CODE (expr) == OMP_CLAUSE_REDUCTION)
+	{
+	  gimple_seq seq = OMP_CLAUSE_REDUCTION_GIMPLE_INIT (expr);
+	  seq = duplicate_remap_omp_clause_seq (seq, wi);
+	  OMP_CLAUSE_REDUCTION_GIMPLE_INIT (expr) = seq;
+	  seq = OMP_CLAUSE_REDUCTION_GIMPLE_MERGE (expr);
+	  seq = duplicate_remap_omp_clause_seq (seq, wi);
+	  OMP_CLAUSE_REDUCTION_GIMPLE_MERGE (expr) = seq;
+	}
+    }
 
   /* Keep iterating.  */
   return NULL_TREE;
@@ -5200,6 +5231,18 @@  replace_locals_stmt (gimple_stmt_iterator *gsip,
   return NULL_TREE;
 }
 
+/* Create a copy of SEQ and remap all decls in it.  */
+
+static gimple_seq
+duplicate_remap_omp_clause_seq (gimple_seq seq, struct walk_stmt_info *wi)
+{
+  /* If there are any labels in OMP sequences, they can be only referred to in
+     the sequence itself and therefore we can do both here.  */
+  walk_gimple_seq (seq, mark_local_labels_stmt, NULL, wi);
+  gimple_seq copy = gimple_seq_copy (seq);
+  walk_gimple_seq (copy, replace_locals_stmt, replace_locals_op, wi);
+  return copy;
+}
 
 /* Copies everything in SEQ and replaces variables and labels local to
    current_function_decl.  */