Re: [PATCH] Fix up some "omp simd array" issues (PR middle-end/66429)

[Richard Biener <rguenther at suse dot de>]

On June 15, 2015 5:46:46 PM GMT+02:00, Jakub Jelinek <jakub@redhat.com> wrote:
>Hi!
>
>As Tom has reported, the for-2.c testcase ICEs at -O2 -fopenmp,
>because it has a noreturn function in the body and so while in omplower
>we decide to use "omp simd array" arrays, in ompexp there is no loop
>to attach the simd stuff to and I forgot to set the has_simduid_loops
>flag
>in that case (and propagate it to the outlined functions).
>
>But there is actually a bigger problem, the cleanup of the simduid
>internal
>calls and arrays has been done only in the vectorizer, but the
>vectorizer
>is part of the loop pass list that isn't run if there are no loops
>left,
>so e.g. in the testcase of noreturn loop where in the end there is no
>loop
>the cleanup wasn't performed.
>
>This patch (the omp-low.c part I can ack myself) thus clears the
>cfun->has_simduid_loops in the vectorizer after cleaning them up and
>adds a new pass gated on cfun->has_simduid_loops that performs just the
>cleanups.  That pass will be invoked only for these pathological cases
>(when the vectorizer pass has not been run because there were no loops
>to vectorize, but still with OpenMP / Cilk+ code which has the simd
>directives).
>
>Bootstrapped/regtested on x86_64-linux and i686-linux, ok for
>trunk/5.2?

OK.  Though I wonder whether this should be a property like the vector and complex lowered state properties we have.

Thanks
Richard.

>2015-06-15  Jakub Jelinek  <jakub@redhat.com>
>
>	PR middle-end/66429
>	* omp-low.c (expand_omp_taskreg): Use child_cfun instead of
>	DECL_STRUCT_FUNCTION (child_fn).  Or in has_simduid_loops
>	and has_force_vectorize_loops flags from cfun into
>	child_cfun.
>	(expand_omp_simd): For broken loop, set cfun->has_simduid_loops
>	if simduid is non-NULL.
>	* tree-pass.h (make_pass_simduid_cleanup): New prototype.
>	* passes.def (pass_simduid_cleanup): Add new pass after loop
>	passes.
>	* tree-vectorizer.c (adjust_simduid_builtins): Remove one unnecessary
>	indirection from htab argument's type.
>	(shrink_simd_arrays): New function.
>	(vectorize_loops): Use it.  Adjust adjust_simduid_builtins caller.
>	Don't call adjust_simduid_builtins if there are no loops.
>	(pass_data_simduid_cleanup, pass_simduid_cleanup): New variables.
>	(pass_simduid_cleanup::execute): New method.
>	(make_pass_simduid_cleanup): New function.
>
>	* c-c++-common/gomp/pr66429.c: New test.
>
>--- gcc/omp-low.c.jj	2015-06-10 11:06:13.000000000 +0200
>+++ gcc/omp-low.c	2015-06-15 13:36:45.644277964 +0200
>@@ -5589,7 +5589,9 @@ expand_omp_taskreg (struct omp_region *r
> 	vec_safe_truncate (child_cfun->local_decls, dstidx);
> 
>       /* Inform the callgraph about the new function.  */
>-      DECL_STRUCT_FUNCTION (child_fn)->curr_properties =
>cfun->curr_properties;
>+      child_cfun->curr_properties = cfun->curr_properties;
>+      child_cfun->has_simduid_loops |= cfun->has_simduid_loops;
>+      child_cfun->has_force_vectorize_loops |=
>cfun->has_force_vectorize_loops;
>       cgraph_node *node = cgraph_node::get_create (child_fn);
>       node->parallelized_function = 1;
>       cgraph_node::add_new_function (child_fn, true);
>@@ -7838,6 +7840,8 @@ expand_omp_simd (struct omp_region *regi
> 	  cfun->has_force_vectorize_loops = true;
> 	}
>     }
>+  else if (simduid)
>+    cfun->has_simduid_loops = true;
> }
> 
> 
>--- gcc/tree-pass.h.jj	2015-04-17 13:50:55.000000000 +0200
>+++ gcc/tree-pass.h	2015-06-15 14:18:50.299679523 +0200
>@@ -372,6 +372,7 @@ extern gimple_opt_pass *make_pass_graphi
> extern gimple_opt_pass *make_pass_if_conversion (gcc::context *ctxt);
>extern gimple_opt_pass *make_pass_loop_distribution (gcc::context
>*ctxt);
> extern gimple_opt_pass *make_pass_vectorize (gcc::context *ctxt);
>+extern gimple_opt_pass *make_pass_simduid_cleanup (gcc::context
>*ctxt);
> extern gimple_opt_pass *make_pass_slp_vectorize (gcc::context *ctxt);
>extern gimple_opt_pass *make_pass_complete_unroll (gcc::context *ctxt);
>extern gimple_opt_pass *make_pass_complete_unrolli (gcc::context
>*ctxt);
>--- gcc/passes.def.jj	2015-06-10 08:18:25.000000000 +0200
>+++ gcc/passes.def	2015-06-15 14:21:01.616671365 +0200
>@@ -270,6 +270,7 @@ along with GCC; see the file COPYING3.
>       PUSH_INSERT_PASSES_WITHIN (pass_tree_no_loop)
> 	  NEXT_PASS (pass_slp_vectorize);
>       POP_INSERT_PASSES ()
>+      NEXT_PASS (pass_simduid_cleanup);
>       NEXT_PASS (pass_lower_vector_ssa);
>       NEXT_PASS (pass_cse_reciprocals);
>       NEXT_PASS (pass_reassoc);
>--- gcc/tree-vectorizer.c.jj	2015-06-10 08:18:29.000000000 +0200
>+++ gcc/tree-vectorizer.c	2015-06-15 14:31:02.548482422 +0200
>@@ -171,7 +171,7 @@ simd_array_to_simduid::equal (const simd
>    into their corresponding constants.  */
> 
> static void
>-adjust_simduid_builtins (hash_table<simduid_to_vf> **htab)
>+adjust_simduid_builtins (hash_table<simduid_to_vf> *htab)
> {
>   basic_block bb;
> 
>@@ -203,10 +203,12 @@ adjust_simduid_builtins (hash_table<simd
> 	  gcc_assert (TREE_CODE (arg) == SSA_NAME);
> 	  simduid_to_vf *p = NULL, data;
> 	  data.simduid = DECL_UID (SSA_NAME_VAR (arg));
>-	  if (*htab)
>-	    p = (*htab)->find (&data);
>-	  if (p)
>-	    vf = p->vf;
>+	  if (htab)
>+	    {
>+	      p = htab->find (&data);
>+	      if (p)
>+		vf = p->vf;
>+	    }
> 	  switch (ifn)
> 	    {
> 	    case IFN_GOMP_SIMD_VF:
>@@ -309,6 +311,38 @@ note_simd_array_uses (hash_table<simd_ar
> 	    walk_gimple_op (use_stmt, note_simd_array_uses_cb, &wi);
>       }
> }
>+
>+/* Shrink arrays with "omp simd array" attribute to the corresponding
>+   vectorization factor.  */
>+
>+static void
>+shrink_simd_arrays
>+  (hash_table<simd_array_to_simduid> *simd_array_to_simduid_htab,
>+   hash_table<simduid_to_vf> *simduid_to_vf_htab)
>+{
>+  for (hash_table<simd_array_to_simduid>::iterator iter
>+	 = simd_array_to_simduid_htab->begin ();
>+       iter != simd_array_to_simduid_htab->end (); ++iter)
>+    if ((*iter)->simduid != -1U)
>+      {
>+	tree decl = (*iter)->decl;
>+	int vf = 1;
>+	if (simduid_to_vf_htab)
>+	  {
>+	    simduid_to_vf *p = NULL, data;
>+	    data.simduid = (*iter)->simduid;
>+	    p = simduid_to_vf_htab->find (&data);
>+	    if (p)
>+	      vf = p->vf;
>+	  }
>+	tree atype
>+	  = build_array_type_nelts (TREE_TYPE (TREE_TYPE (decl)), vf);
>+	TREE_TYPE (decl) = atype;
>+	relayout_decl (decl);
>+      }
>+
>+  delete simd_array_to_simduid_htab;
>+}
> >
> /* A helper function to free data refs.  */
> 
>@@ -445,11 +479,7 @@ vectorize_loops (void)
> 
>   /* Bail out if there are no loops.  */
>   if (vect_loops_num <= 1)
>-    {
>-      if (cfun->has_simduid_loops)
>-	adjust_simduid_builtins (&simduid_to_vf_htab);
>-      return 0;
>-    }
>+    return 0;
> 
>   if (cfun->has_simduid_loops)
>     note_simd_array_uses (&simd_array_to_simduid_htab);
>@@ -558,37 +588,14 @@ vectorize_loops (void)
> 
>   /* Fold IFN_GOMP_SIMD_{VF,LANE,LAST_LANE} builtins.  */
>   if (cfun->has_simduid_loops)
>-    adjust_simduid_builtins (&simduid_to_vf_htab);
>+    adjust_simduid_builtins (simduid_to_vf_htab);
> 
>   /* Shrink any "omp array simd" temporary arrays to the
>      actual vectorization factors.  */
>   if (simd_array_to_simduid_htab)
>-    {
>-      for (hash_table<simd_array_to_simduid>::iterator iter
>-	   = simd_array_to_simduid_htab->begin ();
>-	   iter != simd_array_to_simduid_htab->end (); ++iter)
>-	if ((*iter)->simduid != -1U)
>-	  {
>-	    tree decl = (*iter)->decl;
>-	    int vf = 1;
>-	    if (simduid_to_vf_htab)
>-	      {
>-		simduid_to_vf *p = NULL, data;
>-		data.simduid = (*iter)->simduid;
>-		p = simduid_to_vf_htab->find (&data);
>-		if (p)
>-		  vf = p->vf;
>-	      }
>-	    tree atype
>-	      = build_array_type_nelts (TREE_TYPE (TREE_TYPE (decl)), vf);
>-	    TREE_TYPE (decl) = atype;
>-	    relayout_decl (decl);
>-	  }
>-
>-      delete simd_array_to_simduid_htab;
>-    }
>-    delete simduid_to_vf_htab;
>-    simduid_to_vf_htab = NULL;
>+    shrink_simd_arrays (simd_array_to_simduid_htab,
>simduid_to_vf_htab);
>+  delete simduid_to_vf_htab;
>+  cfun->has_simduid_loops = false;
> 
>   if (num_vectorized_loops > 0)
>     {
>@@ -603,6 +610,64 @@ vectorize_loops (void)
> }
> 
> 
>+/* Entry point to the simduid cleanup pass.  */
>+
>+namespace {
>+
>+const pass_data pass_data_simduid_cleanup =
>+{
>+  GIMPLE_PASS, /* type */
>+  "simduid", /* name */
>+  OPTGROUP_NONE, /* optinfo_flags */
>+  TV_NONE, /* tv_id */
>+  ( PROP_ssa | PROP_cfg ), /* properties_required */
>+  0, /* properties_provided */
>+  0, /* properties_destroyed */
>+  0, /* todo_flags_start */
>+  0, /* todo_flags_finish */
>+};
>+
>+class pass_simduid_cleanup : public gimple_opt_pass
>+{
>+public:
>+  pass_simduid_cleanup (gcc::context *ctxt)
>+    : gimple_opt_pass (pass_data_simduid_cleanup, ctxt)
>+  {}
>+
>+  /* opt_pass methods: */
>+  opt_pass * clone () { return new pass_simduid_cleanup (m_ctxt); }
>+  virtual bool gate (function *fun) { return fun->has_simduid_loops; }
>+  virtual unsigned int execute (function *);
>+
>+}; // class pass_simduid_cleanup
>+
>+unsigned int
>+pass_simduid_cleanup::execute (function *fun)
>+{
>+  hash_table<simd_array_to_simduid> *simd_array_to_simduid_htab =
>NULL;
>+
>+  note_simd_array_uses (&simd_array_to_simduid_htab);
>+
>+  /* Fold IFN_GOMP_SIMD_{VF,LANE,LAST_LANE} builtins.  */
>+  adjust_simduid_builtins (NULL);
>+
>+  /* Shrink any "omp array simd" temporary arrays to the
>+     actual vectorization factors.  */
>+  if (simd_array_to_simduid_htab)
>+    shrink_simd_arrays (simd_array_to_simduid_htab, NULL);
>+  fun->has_simduid_loops = false;
>+  return 0;
>+}
>+
>+}  // anon namespace
>+
>+gimple_opt_pass *
>+make_pass_simduid_cleanup (gcc::context *ctxt)
>+{
>+  return new pass_simduid_cleanup (ctxt);
>+}
>+
>+
> /*  Entry point to basic block SLP phase.  */
> 
> namespace {
>--- gcc/testsuite/c-c++-common/gomp/pr66429.c.jj	2015-06-15
>14:36:28.193502945 +0200
>+++ gcc/testsuite/c-c++-common/gomp/pr66429.c	2015-06-15
>14:35:41.000000000 +0200
>@@ -0,0 +1,41 @@
>+/* PR middle-end/66429 */
>+/* { dg-do compile } */
>+/* { dg-options "-O2 -fopenmp" } */
>+
>+float b[10][15][10];
>+
>+__attribute__ ((noreturn)) void
>+noreturn (void)
>+{
>+  for (;;);
>+}
>+
>+__attribute__ ((noinline, noclone)) void
>+foo (int n)
>+{
>+  int i;
>+
>+#pragma omp parallel for simd schedule(static, 32) collapse(3)
>+  for (i = 0; i < 10; i++)
>+    for (int j = n; j < 8; j++)
>+      for (long k = -10; k < 10; k++)
>+	{
>+	  b[i][j][k] += 16;
>+	  noreturn ();
>+	  b[i][j][k] -= 32;
>+	}
>+}
>+
>+__attribute__ ((noinline, noclone)) void
>+bar (void)
>+{
>+  int i;
>+
>+#pragma omp parallel for simd schedule(static, 32)
>+  for (i = 0; i < 10; i++)
>+    {
>+      b[0][0][i] += 16;
>+      noreturn ();
>+      b[0][0][i] -= 32;
>+    }
>+}
>
>	Jakub