This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


Hi!

On Fri, Sep 20, 2013 at 12:00:41PM -0400, Jason Merrill wrote:

Thanks for the review, I'll try to get to most of that next week.

> On 09/12/2013 04:55 AM, Jakub Jelinek wrote:
> >-      if (t1 != t2)
> >+      if (t1 != t2 && !DECL_OMP_DECLARE_REDUCTION_P (newdecl))
> >  	return 0;
> 
> What's the theory here?  Why should decls_match return true for
> reductions with mismatching templates?

In templates the UDRs are always FUNCTION_DECLs in classes or
at function block scope, the above one liner was I believe for the latter,
where without it duplicate_decls was returning incorrectly 0; the UDRs
from mismatching templates would actually never be seen by duplicate_decls,
but t1 was different from t2.  That was before the changes to
add the mangled names to the UDR DECL_NAMEs though, I can try to remove it
and see if the whole testsuite still passes.

> >+	  && ! (DECL_OMP_DECLARE_REDUCTION_P (newdecl)
> >+		&& DECL_CONTEXT (newdecl) == NULL_TREE
> >+		&& DECL_CONTEXT (olddecl) == current_function_decl))
> 
> And this looks like you need to set DECL_CONTEXT sooner on reductions.

I know it is ugly, but setting FUNCTION_DECL DECL_CONTEXT too early resulted
in all kinds of problems, when the C++ frontend doesn't support nested
functions.  So the patch doesn't set DECL_CONTEXT until it is pushdecled
into the block scope.  The combiner/initializer expressions can't use vars
other than omp_{out,in,priv,orig}, so the UDRs aren't really nested in the
C nested function sense, and after all, after parsing the FUNCTION_DECL
for UDRs is just a container holding the expressions and artificial
VAR_DECLs, and is a FUNCTION_DECL so that normal C++ lookups can be
performed on it.

But like with the above, perhaps it isn't needed anymore, as with the
mangled type names in DECL_NAMEs we shouldn't really use overloads for UDRs.

> >+	    if (TREE_CODE (argtype) == REFERENCE_TYPE)
> >+	      error_at (DECL_SOURCE_LOCATION (t),
> >+			"function, array or reference type in "
> >+			"%<#pragma omp declare reduction%>");
> 
> Let's just say "reference type", since we know that's what it is.

That is true, but I wanted to match the same error message elsewhere,
otherwise the error will be different (more specific) for instantiation
vs. in non-template code.  Though, I could of course in that second spot
just special case REFERENCE_TYPE with a separate error message
and just have one about function or array type.
> 
> >+			 && DECL_CONTEXT (pattern_decl)
> >+			 && TREE_CODE (DECL_CONTEXT (pattern_decl))
> >+			    == FUNCTION_DECL)
> 
> This is DECL_FUNCTION_SCOPE_P.
>
> >+  return TREE_CODE (TREE_TYPE (decl)) == REFERENCE_TYPE
> >+	 || is_invisiref_parm (decl);
> 
> Needs parens to protect indentation.
> 
> >+/* Instantiate the special body of the artificial DECL_OMP_DECLARE_REDUCTION
> >+   function.  */
> 
> We could use documentation of what this special body looks like,
> either here, in cp_check_omp_declare_reduction, or elsewhere.

3xOk.
> 
> >+      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;
> 
> This should check for ambiguity rather than returning the first match.

I believe I need to discuss that on omp-lang, what exactly is the intended
behavior (and get the standard clarified).  Returning the first base class
is an option (and, depth-first vs. breadth-first?), or erroring out if more
than one base class has an UDR is another.

> >+   Also append INIT_EXPR for DECL_INITIAL of omp_priv after its
> >+   DECL_EXPR.  */
> 
> Why not let the DECL_EXPR handle initialization?

I can try that.

> Let's break out the finish_omp_clauses reduction code into a
> separate function, as it's rather large.

Will try that.
> 
> >+		    if (DECL_TEMPLATE_INFO (id))
> >+		      id = instantiate_decl (id, /*defer_ok*/0, true);
> 
> Let's use mark_used instead.

Will that always instantiate it?

	Jakub


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]