[Bug libgomp/94251] New: [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[Bug libgomp/94251] New: [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94251

            Bug ID: 94251
           Summary: [OpenMP] 'target link' fails at run time /
                    libgomp.c/target-link-1.c fails on GCN
           Product: gcc
           Version: 10.0
            Status: UNCONFIRMED
          Keywords: openmp, wrong-code
          Severity: normal
          Priority: P3
         Component: libgomp
          Assignee: unassigned at gcc dot gnu.org
          Reporter: burnus at gcc dot gnu.org
                CC: jakub at gcc dot gnu.org
  Target Milestone: ---

Follow-up to PR 94233.

The test case libgomp.c/target-link-1.c fails with AMDGCN with:
  "libgomp: Cannot map target variables (size mismatch)"

------------------------

That's in libgomp/target.c's gomp_load_image_to_device:

  /* Most significant bit of the size in host and target tables marks
     "omp declare target link" variables.  */
  const uintptr_t link_bit = 1ULL << (sizeof (uintptr_t) * __CHAR_BIT__ - 1);
  const uintptr_t size_mask = ~link_bit;

      struct addr_pair *target_var = &target_table[num_funcs + i];
      uintptr_t target_size = target_var->end - target_var->start;

      if ((uintptr_t) host_var_table[i * 2 + 1] != target_size)
        gomp_fatal ("Cannot map target variables (size mismatch)");

-----------------------

The code clearly assumes that the link_bit is also present in target_size –
which is not only visible in the quoted condition but also for:

      k->refcount = target_size & link_bit ? REFCOUNT_LINK : REFCOUNT_INFINITY;

While for the host data, that's not assumed as one masks the data:

      k->host_end
        = k->host_start + (size_mask & (uintptr_t) host_var_table[i * 2 + 1]);

I think it should be:
* no link mask in target_var->end / target_size
* Use 'host_var_table[i*2+1] & link_bit' in the refcount expression
* Use '& link_bit' for the size comparison. 

------------------------

The passed arguments are fine:
(gdb) p host_var_table[1]
$15 = (void *) 0x8000000000000008
(gdb) p host_var_table[3]
$16 = (void *) 0x80000000000000d8
(gdb) p host_var_table[5]
$17 = (void *) 0x4
(gdb) p host_var_table[7]
$18 = (void *) 0x8000000000000004

Namely: 'd' = 2*int = 8, 'c' = 27*double = 216 = 0xD8, 'a' and 'b' being
int=4.,
'b' is 'to', the rest is 'link'.

However, at least with AMDGCN, target_size == 8 for all variables. At a glance,
the code in GOMP_OFFLOAD_load_image looks fine, but:

image_desc->global_variables[i]->name is "d_linkptr" and not "d".


That variable is generated by lto/lto.c's offload_handle_link_vars as

        tree type = build_pointer_type (TREE_TYPE (var->decl));

where var->decl is "d" in this case.

Thus, it is not surprising that the size is always 8.

 * * *

Looking at the other plugins, they seems to work mostly likewise [get bit size
correctly]:

* hsa: global variables not processed in load_image, i.e.
  target_table not updated for the global variables!
* nvptx: cuModuleGetGlobal called, look fine.

[Bug libgomp/94251] [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94251

--- Comment #1 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Created attachment 48077
  --> https://gcc.gnu.org/bugzilla/attachment.cgi?id=48077&action=edit
(Partial patch): Patch for gomp_load_image_to_device to handle link_flag
correctly

[Cross ref: nvptx disables the libgomp.c/target-link-1.c because of PR81689]

Attached fixes the size check and the link vs not link for refcount.

However, the big question is what be the target variable?
 (a) the variable itself such as 'c' with 'int c[27]'
 (b) a pointer to a variable such as 'c_linkptr'

If (b), then the size check does not work and the the tgt side is a pointer.
If (a), then the passed name does not fit.

Jakub: How is this supposed to be implemented?

[Bug libgomp/94251] [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94251

--- Comment #2 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tobias Burnus <burnus@gcc.gnu.org>:

https://gcc.gnu.org/g:4897bb0045dd68474176be9aafb4d77bac4b363a

commit r10-7333-g4897bb0045dd68474176be9aafb4d77bac4b363a
Author: Tobias Burnus <tobias@codesourcery.com>
Date:   Mon Mar 23 15:29:05 2020 +0100

    libgomp â fix declare target link handling (PR94251)

            PR libgomp/94251
            * target.c (gomp_load_image_to_device): Fix link
            variable handling.

[Bug libgomp/94251] [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94251

--- Comment #3 from Tobias Burnus <burnus at gcc dot gnu.org> ---
FIXED for AMDGCN – see PR 81689 for Nvptx, which still fails.

[Bug libgomp/94251] [OpenMP] 'target link' fails at run time / libgomp.c/target-link-1.c fails on GCN

[burnus at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94251

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |FIXED

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> ---
Really close as FIXED