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
