diff mbox

[gomp4] C++ OpenMP user defined reductions (take 2)

Message ID 20131009174311.GV30970@tucnak.zalov.cz
State New
Headers show

Commit Message

Jakub Jelinek Oct. 9, 2013, 5:43 p.m. UTC 
On Wed, Oct 09, 2013 at 10:47:22AM -0400, Jason Merrill wrote:
> On 10/07/2013 07:02 AM, Jakub Jelinek wrote:
> >duplicates anywhere, but during error diagnostics.  Without those two decl.c
> >hunks (either of them), pushdecl will sometimes return a different decl from
> >the original or error_mark_node, and the original fndecl passed to it has
> >ggc_free called on it, thus any further use of it ICEs or may ICE.
> 
> Right.
> 
> >Perhaps if pushdecl returns error_mark_node,
> >then I'd should expect that the error has been reported already and if
> >it returns some other FUNCTION_DECL, then I should report it myself,
> 
> Makes sense.
> 
> >but a problem with that is that there are multiple locations that call
> >pushdecl (two in parser.c, one in pt.c) and more importantly, that for
> >the diagnostics the new fndecl is ggc_freed and thus I can't use it
> >for the diagnostics anymore.
> 
> True, though probably input_location is enough.

It turned much easier to just diagnose and return error_mark_node
for UDRs always in duplicate_decls by just moving the if a few lines
earlier, then no changes in decls_match are needed.

> >>Normal C++ lookup behavior is to check for ambiguity, so I think
> >>that's the best bet for what the eventual defined semantics will be.
> >
> >No response from omp-lang yet, so I'm not changing this yet.
> 
> Please do change it.  The current behavior is just wrong, and we
> should set a good example for others to follow.  It's ok to fix this
> in a follow-up patch.

Ok, see the attached patch.
> 
> >Unfortunately it didn't work, again on the udr-3.C testcase.
> >mark_used was already called during instantiation of the decl, DECL_ODR_USED
> >got set on it, but it was actually deferred, then when mark_used is called
> >again on it, it is ignored.  I'd need to clear DECL_ODR_USED explicitly
> >and call mark_used, perhaps that would work.
> 
> If deferring it is a problem you can add UDRs to the group of things
> which are always instantiated immediately in mark_used:

Ok, forced it there.  Just to be sure moved the DECL_LANG_SPECIFIC check
first, because DECL_OMP_DECLARE_REDUCTION_P is in DECL_LANG_SPECIFIC.
Perhaps DECL_TEMPLATE_INFO check could also be less expensive to be done
before calling decl_maybe_constant_var_p or undeduced_auto_decl ?

> >+	error_at (loc, "predeclared arithmetic type in %qT"
> >+	error_at (loc, "reference type in %qT"
> 
> "%qT in"

Fixed.

Ok?

2013-10-09  Jakub Jelinek  <jakub@redhat.com>

	* decl.c (decls_match): Revert DECL_OMP_DECLARE_REDUCTION_P
	special cases.
	(duplicate_decls): Move DECL_OMP_DECLARE_REDUCTION_P case
	earlier.
	* parser.c (cp_parser_omp_declare_reduction): Fix spelling
	of some error messages.  Set DECL_CONTEXT (fndecl) to
	global_namespace first and set DECL_LOCAL_FUNCTION_P (fndecl).
	* decl2.c (mark_used): Force immediate instantiation of
	DECL_OMP_DECLARE_REDUCTION_P decls.
	* semantics.c (omp_reduction_lookup): Add baselinkp and
	ambiguousp arguments, diagnose ambiguities, perform access
	check only if non-ambiguous.
	(finish_omp_reduction_clause): Adjust omp_reduction_lookup
	caller, if it returned error_mark_node, just return true,
	use mark_used instead of instantiate_decl.
gcc/testsuite/
	* g++.dg/gomp/udr-6.C: New test.
libgomp/
	* testsuite/libgomp.c++/udr-6.C: Remove UDR + on type F.



	Jakub

Comments

Jason Merrill Oct. 10, 2013, 12:24 a.m. UTC | #1 
On 10/09/2013 01:43 PM, Jakub Jelinek wrote:
> Perhaps DECL_TEMPLATE_INFO check could also be less expensive to be done
> before calling decl_maybe_constant_var_p or undeduced_auto_decl ?

Sure.

> +	  DECL_LOCAL_FUNCTION_P (fndecl) = 1;

This should be set by pushdecl_maybe_friend_1, so I don't think you need 
it here.

Jason
diff mbox

Patch

--- gcc/cp/decl.c.jj	2013-10-07 14:06:58.000000000 +0200
+++ gcc/cp/decl.c	2013-10-09 17:41:58.264948392 +0200
@@ -978,15 +978,12 @@  decls_match (tree newdecl, tree olddecl)
       tree t2 = (DECL_USE_TEMPLATE (olddecl)
 		 ? DECL_TI_TEMPLATE (olddecl)
 		 : NULL_TREE);
-      if (t1 != t2 && !DECL_OMP_DECLARE_REDUCTION_P (newdecl))
+      if (t1 != t2)
 	return 0;
 
       if (CP_DECL_CONTEXT (newdecl) != CP_DECL_CONTEXT (olddecl)
 	  && ! (DECL_EXTERN_C_P (newdecl)
-		&& DECL_EXTERN_C_P (olddecl))
-	  && ! (DECL_OMP_DECLARE_REDUCTION_P (newdecl)
-		&& DECL_CONTEXT (newdecl) == NULL_TREE
-		&& DECL_CONTEXT (olddecl) == current_function_decl))
+		&& DECL_EXTERN_C_P (olddecl)))
 	return 0;
 
       /* A new declaration doesn't match a built-in one unless it
@@ -1344,6 +1341,15 @@  duplicate_decls (tree newdecl, tree oldd
 	    }
 	  return NULL_TREE;
 	}
+      else if (DECL_OMP_DECLARE_REDUCTION_P (olddecl))
+	{
+	  gcc_assert (DECL_OMP_DECLARE_REDUCTION_P (newdecl));
+	  error_at (DECL_SOURCE_LOCATION (newdecl),
+		    "redeclaration of %<pragma omp declare reduction%>");
+	  error_at (DECL_SOURCE_LOCATION (olddecl),
+		    "previous %<pragma omp declare reduction%> declaration");
+	  return error_mark_node;
+	}
       else if (!types_match)
 	{
 	  /* Avoid warnings redeclaring built-ins which have not been
@@ -1422,15 +1428,6 @@  duplicate_decls (tree newdecl, tree oldd
 	  type = cp_build_type_attribute_variant (type, attribs);
 	  TREE_TYPE (newdecl) = TREE_TYPE (olddecl) = type;
 	}
-      else if (DECL_OMP_DECLARE_REDUCTION_P (olddecl))
-	{
-	  gcc_assert (DECL_OMP_DECLARE_REDUCTION_P (newdecl));
-	  error_at (DECL_SOURCE_LOCATION (newdecl),
-		    "redeclaration of %<pragma omp declare reduction%>");
-	  error_at (DECL_SOURCE_LOCATION (olddecl),
-		    "previous %<pragma omp declare reduction%> declaration");
-	  return error_mark_node;
-	}
 
       /* If a function is explicitly declared "throw ()", propagate that to
 	 the corresponding builtin.  */
--- gcc/cp/parser.c.jj	2013-10-07 23:29:44.000000000 +0200
+++ gcc/cp/parser.c	2013-10-09 19:23:01.656235024 +0200
@@ -30131,7 +30131,7 @@  cp_parser_omp_declare_reduction (cp_pars
 				   "min") == 0
 			   || strcmp (IDENTIFIER_POINTER (orig_reduc_id),
 				      "max") == 0))))
-	error_at (loc, "predeclared arithmetic type in %qT"
+	error_at (loc, "predeclared arithmetic type %qT in "
 		       "%<#pragma omp declare reduction%>", type);
       else if (TREE_CODE (type) == FUNCTION_TYPE
 	       || TREE_CODE (type) == METHOD_TYPE
@@ -30139,7 +30139,7 @@  cp_parser_omp_declare_reduction (cp_pars
 	error_at (loc, "function or array type %qT in "
 		       "%<#pragma omp declare reduction%>", type);
       else if (TREE_CODE (type) == REFERENCE_TYPE)
-	error_at (loc, "reference type in %qT"
+	error_at (loc, "reference type %qT in "
 		       "%<#pragma omp declare reduction%>", type);
       else if (TYPE_QUALS_NO_ADDR_SPACE (type))
 	error_at (loc, "const, volatile or __restrict qualified type %qT in "
@@ -30197,6 +30197,8 @@  cp_parser_omp_declare_reduction (cp_pars
       if (current_function_decl)
 	{
 	  block_scope = true;
+	  DECL_CONTEXT (fndecl) = global_namespace;
+	  DECL_LOCAL_FUNCTION_P (fndecl) = 1;
 	  if (!processing_template_decl)
 	    pushdecl (fndecl);
 	}
--- gcc/cp/decl2.c.jj	2013-10-07 14:07:00.000000000 +0200
+++ gcc/cp/decl2.c	2013-10-09 18:14:25.958073742 +0200
@@ -4686,12 +4686,15 @@  mark_used (tree decl, tsubst_flags_t com
      or a constexpr function, we need it right now because a reference to
      such a data member or a call to such function is not value-dependent.
      For a function that uses auto in the return type, we need to instantiate
-     it to find out its type.  */
-  if ((decl_maybe_constant_var_p (decl)
-       || (TREE_CODE (decl) == FUNCTION_DECL
-	   && DECL_DECLARED_CONSTEXPR_P (decl))
-       || undeduced_auto_decl (decl))
-      && DECL_LANG_SPECIFIC (decl)
+     it to find out its type.  For OpenMP user defined reductions, we need
+     them instantiated for reduction clauses which inline them by hand
+     directly.  */
+  if (DECL_LANG_SPECIFIC (decl)
+      && (decl_maybe_constant_var_p (decl)
+	  || (TREE_CODE (decl) == FUNCTION_DECL
+	      && (DECL_DECLARED_CONSTEXPR_P (decl)
+		  || DECL_OMP_DECLARE_REDUCTION_P (decl)))
+	  || undeduced_auto_decl (decl))
       && DECL_TEMPLATE_INFO (decl)
       && !uses_template_parms (DECL_TI_ARGS (decl)))
     {
--- gcc/cp/semantics.c.jj	2013-10-08 08:44:15.000000000 +0200
+++ gcc/cp/semantics.c	2013-10-09 19:10:23.469074632 +0200
@@ -4601,9 +4601,11 @@  omp_reduction_id (enum tree_code reducti
    FUNCTION_DECL or NULL_TREE if not found.  */
 
 static tree
-omp_reduction_lookup (location_t loc, tree id, tree type)
+omp_reduction_lookup (location_t loc, tree id, tree type, tree *baselinkp,
+		      vec<tree> *ambiguousp)
 {
   tree orig_id = id;
+  tree baselink = NULL_TREE;
   if (identifier_p (id))
     {
       cp_id_kind idk;
@@ -4643,20 +4645,61 @@  omp_reduction_lookup (location_t loc, tr
 	    break;
 	}
     }
+  if (id && BASELINK_P (fns))
+    {
+      if (baselinkp)
+	*baselinkp = fns;
+      else
+	baselink = fns;
+    }
   if (id == NULL_TREE && CLASS_TYPE_P (type) && TYPE_BINFO (type))
     {
-      tree binfo = TYPE_BINFO (type), base_binfo;
+      vec<tree> ambiguous = vNULL;
+      tree binfo = TYPE_BINFO (type), base_binfo, ret = NULL_TREE;
       unsigned int ix;
+      if (ambiguousp == NULL)
+	ambiguousp = &ambiguous;
       for (ix = 0; BINFO_BASE_ITERATE (binfo, ix, base_binfo); ix++)
 	{
-	  id = omp_reduction_lookup (loc, orig_id, BINFO_TYPE (base_binfo));
-	  if (id != NULL_TREE)
-	    return id;
+	  id = omp_reduction_lookup (loc, orig_id, BINFO_TYPE (base_binfo),
+				     baselinkp ? baselinkp : &baselink,
+				     ambiguousp);
+	  if (id == NULL_TREE)
+	    continue;
+	  if (!ambiguousp->is_empty ())
+	    ambiguousp->safe_push (id);
+	  else if (ret != NULL_TREE)
+	    {
+	      ambiguousp->safe_push (ret);
+	      ambiguousp->safe_push (id);
+	      ret = NULL_TREE;
+	    }
+	  else
+	    ret = id;
 	}
+      if (ambiguousp != &ambiguous)
+	return ret;
+      if (!ambiguous.is_empty ())
+	{
+	  const char *str = _("candidates are:");
+	  unsigned int idx;
+	  tree udr;
+	  error_at (loc, "user defined reduction lookup is ambiguous");
+	  FOR_EACH_VEC_ELT (ambiguous, idx, udr)
+	    {
+	      inform (DECL_SOURCE_LOCATION (udr), "%s %#D", str, udr);
+	      if (idx == 0)
+		str = get_spaces (str);
+	    }
+	  ambiguous.release ();
+	  ret = error_mark_node;
+	  baselink = NULL_TREE;
+	}
+      id = ret;
     }
-  if (id && BASELINK_P (fns))
-    perform_or_defer_access_check (BASELINK_BINFO (fns), id, id,
-				   tf_warning_or_error);
+  if (id && baselink)
+    perform_or_defer_access_check (BASELINK_BINFO (baselink),
+				   id, id, tf_warning_or_error);
   return id;
 }
 
@@ -4949,12 +4992,13 @@  finish_omp_reduction_clause (tree c, boo
   if (id == NULL_TREE)
     id = omp_reduction_id (OMP_CLAUSE_REDUCTION_CODE (c),
 			   NULL_TREE, NULL_TREE);
-  id = omp_reduction_lookup (OMP_CLAUSE_LOCATION (c), id, type);
+  id = omp_reduction_lookup (OMP_CLAUSE_LOCATION (c), id, type, NULL, NULL);
   if (id)
     {
+      if (id == error_mark_node)
+	return true;
       id = OVL_CURRENT (id);
-      if (DECL_TEMPLATE_INFO (id))
-	id = instantiate_decl (id, /*defer_ok*/0, true);
+      mark_used (id);
       tree body = DECL_SAVED_TREE (id);
       if (TREE_CODE (body) == STATEMENT_LIST)
 	{
--- gcc/testsuite/g++.dg/gomp/udr-6.C.jj	2013-10-09 19:20:45.284925551 +0200
+++ gcc/testsuite/g++.dg/gomp/udr-6.C	2013-10-09 19:20:39.082957547 +0200
@@ -0,0 +1,59 @@ 
+// { dg-do compile }
+
+struct A { int a; A () : a (0) {} };
+struct B { int b; B () : b (0) {} };
+struct C : public A, B { int c; C () : c (0) {} };
+struct D { int d; D () : d (0) {} };
+struct E { int e; E () : e (0) {} };
+struct F : public D, E { int f; F () : f (0) {} };
+struct G : public C, F { int g; G () : g (0) {} };
+
+#pragma omp declare reduction (+: A : omp_out.a += omp_in.a) // { dg-message "operator" }
+#pragma omp declare reduction (+: B : omp_out.b += omp_in.b) // { dg-message "operator" }
+#pragma omp declare reduction (+: D : omp_out.d += omp_in.d)
+#pragma omp declare reduction (+: E : omp_out.e += omp_in.e)
+#pragma omp declare reduction (+: F : omp_out.f += omp_in.f) // { dg-message "operator" }
+
+void
+f1 ()
+{
+  G g;
+  #pragma omp parallel reduction (+: g)	// { dg-error "user defined reduction lookup is ambiguous" }
+  {
+    g.g++;
+  }
+}
+
+#pragma omp declare reduction (*: A : omp_out.a += omp_in.a)
+#pragma omp declare reduction (*: B : omp_out.b += omp_in.b)
+#pragma omp declare reduction (*: D : omp_out.d += omp_in.d)
+#pragma omp declare reduction (*: E : omp_out.e += omp_in.e)
+#pragma omp declare reduction (*: F : omp_out.f += omp_in.f)
+#pragma omp declare reduction (*: G : omp_out.g += omp_in.g)
+
+void
+f2 ()
+{
+  G g;
+  #pragma omp parallel reduction (*: g)
+  {
+    g.g++;
+  }
+}
+
+#pragma omp declare reduction (|: A : omp_out.a += omp_in.a)
+#pragma omp declare reduction (|: B : omp_out.b += omp_in.b)
+#pragma omp declare reduction (|: C : omp_out.c += omp_in.c) // { dg-message "operator" }
+#pragma omp declare reduction (|: D : omp_out.d += omp_in.d)
+#pragma omp declare reduction (|: E : omp_out.e += omp_in.e)
+#pragma omp declare reduction (|: F : omp_out.f += omp_in.f) // { dg-message "operator" }
+
+void
+f3 ()
+{
+  G g;
+  #pragma omp parallel reduction (|: g)	// { dg-error "user defined reduction lookup is ambiguous" }
+  {
+    g.g++;
+  }
+}
--- libgomp/testsuite/libgomp.c++/udr-6.C.jj	2013-09-18 12:43:23.000000000 +0200
+++ libgomp/testsuite/libgomp.c++/udr-6.C	2013-10-09 19:25:04.450612965 +0200
@@ -11,8 +11,6 @@  struct F : C, D {};
 struct G : E, F {};
 void foo (B &);
 void foo (F &);
-#pragma omp declare reduction (+:F:omp_out.c += omp_in.c) \
-		    initializer(foo (omp_priv))
 #pragma omp declare reduction (+:B:omp_out.b += omp_in.b) \
 		    initializer(foo (omp_priv))