[PATCH] D84192: [OpenMP5.0] map item can be non-contiguous for target update

[Chi Chun Chen via Phabricator via cfe-commits]

cchen added a comment.

In D84192#2167649 <https://reviews.llvm.org/D84192#2167649>, @ABataev wrote:

> In D84192#2167636 <https://reviews.llvm.org/D84192#2167636>, @cchen wrote:
>
> > In D84192#2165522 <https://reviews.llvm.org/D84192#2165522>, @ABataev wrote:
> >
> > > In D84192#2165517 <https://reviews.llvm.org/D84192#2165517>, @cchen wrote:
> > >
> > > > In D84192#2165438 <https://reviews.llvm.org/D84192#2165438>, @ABataev wrote:
> > > >
> > > > > In D84192#2165396 <https://reviews.llvm.org/D84192#2165396>, @cchen wrote:
> > > > >
> > > > > > In D84192#2165249 <https://reviews.llvm.org/D84192#2165249>, @ABataev wrote:
> > > > > >
> > > > > > > In D84192#2165235 <https://reviews.llvm.org/D84192#2165235>, @cchen wrote:
> > > > > > >
> > > > > > > > In D84192#2164885 <https://reviews.llvm.org/D84192#2164885>, @ABataev wrote:
> > > > > > > >
> > > > > > > > > Also, what about compatibility with declare mapper? Can you add tests for it?
> > > > > > > >
> > > > > > > >
> > > > > > > > There's a restriction for map clause that non-contiguous is not allowed and I guess it also applies to declare mapper.
> > > > > > >
> > > > > > >
> > > > > > > I see that to/from clauses allow to use mappers too:
> > > > > > >
> > > > > > >   to([mapper(mapper-identifier):]locator-list) from([mapper(mapper-identifier):]locator-list
> > > > > > >   
> > > > > >
> > > > > >
> > > > > > I'm confused of how to use to([mapper(mapper-identifier):]locator-list) with array section.
> > > > > >  For this case:
> > > > > >
> > > > > >   class C {
> > > > > >   public:
> > > > > >     int a;
> > > > > >     double b[3][4][5];
> > > > > >   };
> > > > > >  
> > > > > >   #pragma omp declare mapper(id: C s) map(s.a, s.b[0:3][0:4][0:5])
> > > > > >  
> > > > > >   #pragma omp target update from(mapper(id): c)
> > > > > >
> > > > > >
> > > > > > Clang already has a semantic check in from clause when mapper is used: "mapper type must be of struct, union or class type".
> > > > > >  So the only item I can put in from clause in the above example is `c` and I cannot put c.b[0:2][1:2][0:2] or any even c.b[0:2].
> > > > >
> > > > >
> > > > > Does clang compile your example? If not, shall it be allowed for to/from clauses or not?
> > > >
> > > >
> > > > Clang can compile my example but the problem is that Clang do not accept something like `#pragma omp target update from(mapper(id): c.b[0:2][1:2][2:2])` (non-contiguous) or even `#pragma omp target update from(mapper(id): c.b[2][1][0:2])` (contiguous).
> > > >  Actually, Clang now only accepts `c` as struct/union/class type in `from(mapper(id): c)`. And the reason for the restriction is from declare mapper directive - "The type must be of struct, union or class type in C and C++".
> > >
> > >
> > > And it is fine. How does it work in declare mapper, the main question? Does it support extended array sections format mapoers with maps, to and from? Shall it be supported in declare mapper at all?
> >
> >
> > After discussing with @dreachem, my understanding is that it is not incorrect to not allowing extended/non-contiguous array section to appear in declare mapper.
> >  For the declare mapper directive, since spec only allows map clause, extended array section (with stride) or non-contiguous array section are both not allowed.
> >  For using the mapper in map/to/from clause, if mapping or updating an array section of type T, where there is a mapper declared for T, then the mapper needs to apply as if to each element of the array or array section. Spec is intentionally not sufficiently clear on this for target update so the semantic check in Clang is not incorrect. Which lead to the fact that I might not need to support extended/non-contiguous array section for declare mapper.
>
>
> What exactly is incorrect in clang sema?


I mean "not" incorrect for not allowing extended/non-contiguous array section in `to/from([mapper(mapper-identifier):] location-list).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D84192/new/

https://reviews.llvm.org/D84192