Re: [PATCH v7 2/5] OpenMP/OpenACC: Rework clause expansion and nested struct handling

[Tobias Burnus <tobias@codesourcery.com>]

Hi Julian,

On 29.11.23 12:43, Julian Brown wrote:
> Here is a patch incorporating your initial review comments
> (hopefully!).

Thanks.

The patch LGTM - with the two remarks below addressed.

(i.e. fixing one testcase and filing two PRs (or common PR) about the features
missing and exposed by the two test cases, referencing also to those testcases
- and for the lvalues mentioning the OpenMP spec issue number.)

* * *

BTW: The 1/5 has been several times approved and is just reindenting - and
is obviously still OK.


(Review wise, 3/5, 4/5 and 5/5 still has to be done.

I think the patch can go in before - given the huge improvements, even though
it regresses for a few cases (xfail added for 2 Fortran testcases). 3/5 un-xfails
one and a half of the textcases, 5/5 un-xfails the remaining half and all of {3,4,5}/5
contain very useful improvements besides this. - But maybe waiting for at least 3/5
makes sense.

In either case, I try to review the remaining patches soon.)

* * *

Question regarding the following:
(a) The dg-xfail-run-if looks bogus as this an OpenMP test and not an OpenACC test
(b) If there is shared memory, using 'omp target' should be fine.

Namely, given that:

> --- /dev/null +++ b/libgomp/testsuite/libgomp.c++/target-49.C @@ -0,0
> +1,37 @@ +#include <cstring> +#include <cassert> + +struct s { + int
> (&a)[10]; + s(int (&a0)[10]) : a(a0) {} +}; + +int +main (int argc,
> char *argv[]) +{ + int la[10]; + s v_real(la); + s *v = &v_real; + +
> memset (la, 0, sizeof la); + + #pragma omp target enter data map(to:
> v) + + /* Copying the whole v[0] here DOES NOT WORK yet because the
> reference 'a' is + not copied "as if" it was mapped explicitly as a
> member. FIXME. */ + #pragma omp target enter data map(to: v[0]) + +
> //#pragma omp target + { + v->a[5]++; + } + + #pragma omp target exit
> data map(release: v[0]) + #pragma omp target exit data map(from: v) +
> + assert (v->a[5] == 1); + + return 0; +} + +// { dg-xfail-run-if
> "TODO" { *-*-* } { "-DACC_MEM_SHARED=0" } }
Shouldn't the XFAIL not be based on '{ target offload_device_nonshared_as }'
and the 'omp target' be uncommented?

And I wonder whether we need to file a PR about this issue - I guess it is not
addressed by any of the follow-up issues and might get forgotten unless there is PR.


* * *
> libgomp/testsuite/libgomp.c++/baseptrs-4.C ... // Needs map clause
> "lvalue"-parsing support. //#define REF2ARRAY_DECL_BASE

There is an open OpenMP issue to disallow some lvalues, namely:
OpenMP Issue 2618 ("Clarify behavior of mapping lvalues on target construct")
talks about code like the following

   map(*p = 10)
   map(x = 20)
   map(x ? y[0] : p[1])
   map(f(y))

is valid or not. The sentiment was to require that a 'map' clause list item
must have a base pointer or a base variable.


However, it looks as your examples would be valid in this regard. Can you file
a PR about this one? Referencing both to this testcase and to the OpenMP issue?

(I do note that Clang and GCC reject the lvalue examples from the OpenMP issue
but not your reference examples; those are accepted by clang++-14.)


Tobias

-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955