Re: [PATCH] Fix PR78027

[Alexander Monakov <amonakov at ispras dot ru>]

On Fri, 9 Dec 2016, Cesar Philippidis wrote:
> >>  if (cgraph_node::get_create (decl)->offloadable
> >>      && !lookup_attribute ("omp declare target",
> >>                           DECL_ATTRIBUTES (current_function_decl)))
> >>    {
> >>      const char *target_attr = (is_gimple_omp_offloaded (ctx->stmt)
> >>                                 ? "omp target entrypoint"
> >>                                 : "omp declare target");
> >>      DECL_ATTRIBUTES (decl)
> >>        = tree_cons (get_identifier (target_attr),
> >>                     NULL_TREE, DECL_ATTRIBUTES (decl));
> >>    }
> > 
> > So, like I said -- offloaded functions have either "omp target entrypoint" or
> > "omp declare function".  Either of those are caught by the _existing_ "omp "
> > prefix check in IPA ICF.
> 
> How did you interpret this from that code?

Bah, sorry, I was missing the distinction between 'decl' and
'current_function_decl' and was always interpreting this code like 'add an
attribute, unless "omp declare function" is already present on decl'.

Perhaps the original intention was to suppress creating target entrypoints
inside of offloaded code.  But in that case, the code should have checked either
"omp declare target" or "omp target entrypoint" on the containing function.  I
think this is quite confusing.  I've changed this code in OpenMP/NVPTX work, but
not too much.

Jakub -- do you think I'm deducing the original intent here correctly?  Can we
a) fix that code to properly follow that intention, and
b) start enforcing the invariant that all offloaded code has either "omp target
entrypoint" or "omp declare function"?

> > on them, because even if they have just "omp declare target" it's still subject
> > to that ICF blacklist.
> > 
> > My objection still stands -- if you have "oacc fnattr" functions that have
> > neither of those attributes, that doesn't seem right.
> 
> I'm confused. Are you implying that create_omp_child_function is
> correct, or are you arguing for omp function attributes to always be
> present on all offloaded functions, or both?

The 2nd option -- I thought offloaded functions were intended to have one of
"omp *" attributes, but it appears I might have been mistaken.


Still, "offloaded functions must have one of two omp attributes" sounds like a
simpler invariant than "<the above> or the oacc attribute [and all code checking
the former must start checking the oacc attribute too]".

Thanks for bearing with me.
Alexander