Re: [Patch] OpenMP: Handle cpp_implicit_alias in declare-target discovery (PR96390)

[Tobias Burnus <tobias@codesourcery.com>]

On 9/22/20 4:24 PM, Jakub Jelinek wrote:
> On Tue, Sep 22, 2020 at 04:11:19PM +0200, Tobias Burnus wrote:
>> +      while (node->alias_target)
>> +        {
>> +          node = node->ultimate_alias_target ();
> At least in theory, ultimate_alias_target can look through multiple aliases.

Granted. But we need to handle two things:
* target alias (such as for __attribute__(alias(...))
   for this one, I can walk 'node = node->alias_target'
   here, I need to mark all nodes on the way.
* C++ aliases
   for this one, I used node = node->ultimate_alias_target ()
   here, I only need to mark the last one as only
   that function decl is streamed out

> While it might not do that most of the time because this is executed quite
> early, I think we have no guarantees it will never do it.
> So I'd prefer what you had in the earlier patch, i.e. do the
> ultimate_alias_target call + loop to find the ultimate node, and then
> in another loop go from the original node (inclusive) up to the ultimate one
> (exclusive) and do what you do in this loop now.
> Does that make sense?

I am lost. What do I gain by running the loops twice? Initially
the idea was to return NULL_TREE but as you example showed we need
to mark all unmarked ones until to final node.

Thus, we have to mark all – even if the final one is already
'omp declare target'.

But in that case, why can't we do it in a single loop?



If we assume that there is no c++ aliasing, we could do:

           while (node->alias_target)
             {
               // see assumption above: // node = node->ultimate_alias_target ();
               if (!omp_declare_target_fn_p (node->decl)
                   && !lookup_attribute ("omp declare target host",
                                         DECL_ATTRIBUTES (node->decl)))
                 {
                   node->offloadable = 1;
                   DECL_ATTRIBUTES (node->decl)
                     = tree_cons (id, NULL_TREE, DECL_ATTRIBUTES (node->decl));
                 }
               node = symtab_node::get (node->alias_target);
             }
           // all node->alias_target resolved
           node = node->ultimate_alias_target ();

That would avoid the in-between calling of ultimate_alias_target() but still
calls it if there is no alias_target or for the final alias target.

Is this (really) better?

BTW: When the assumption about the ordering completely changes,
the current __attribute__(alias(…)) testcase will fail; this might
not catch all issues but at least if it completely changes.

Tobias

-----------------
Mentor Graphics (Deutschland) GmbH, Arnulfstraße 201, 80634 München / Germany
Registergericht München HRB 106955, Geschäftsführer: Thomas Heurung, Alexander Walter