[PATCH, OpenMP 5.0] Implement structure element mapping changes in 5.0

[Jakub Jelinek]

On Sat, Oct 24, 2020 at 01:43:26AM +0800, Chung-Lin Tang wrote:
> On 2020/10/23 8:13 PM, Jakub Jelinek wrote:
> > > In general, upon encountering a construct, we can't statically determine and insert alloc/release maps
> > > for each element of a structure variable, since we don't really know which region of the structure is
> > > currently mapped or not, hence this probably can't be properly implemented in the compiler.
> > > 
> > > Instead this patch tries to do the equivalent in the runtime: I've modified the handling of the
> > > (GOMP_MAP_STRUCT, <field-map1>, <field-map2>, ...) sequence to:
> > > 
> > >    (1) Create just a single splay_tree_key to represent the entire structure's mapped-region
> > >        (all element target_var_desc's now reference this same key instead of creating their own), and
> > I'm not sure that is what we want.  If we create just a single
> > splay_tree_key spanning the whole structure mapped region, then we can't
> > diagnose various mapping errors.  E.g. if I have:
> > void bar (struct S *);
> > struct S { int a, b, c, d, e; };
> > void foo (struct S s)
> > {
> >    #pragma omp target data map(tofrom: s.b, s.d)
> >    #pragma omp target map (s.b, s.c)
> >    bar (&s);
> > }
> > then target data maps the &s.b to &s.d + 1 region of the struct, but s.c
> > wasn't mapped and so the target region's mapping should fail, even when it
> > is in the middle of the mapped region.
> 
> Are you really sure this is what we want? I don't quite see anything harmful
> about implicitly mapping "middle fields" like s.c, in fact the corresponding
> memory is actually "mapped" anyways.

Yes, it is a QoI and it is important not to regress about that.
Furthermore, the more we diverge from what the spec says, it will be harder
for us to implement, not just now, but in the future too.
What I wrote about the actual implementation is actually not accurate, we
need the master and slaves to be the struct splay_tree_key_s objects.
And that one already has the aux field that could be used for the slaves,
so we could e.g. use another magic value of refcount, e.g. REFCOUNT_SLAVE
~(uintptr_t) 2, and in that case aux would point to the master
splay_tree_key_s.

And the 
"If the corresponding list item’s reference count was not already incremented because of the
effect of a map clause on the construct then:
a) The corresponding list item’s reference count is incremented by one;"
and
"If the map-type is not delete and the corresponding list item’s reference count is finite and
was not already decremented because of the effect of a map clause on the construct then:
a) The corresponding list item’s reference count is decremented by one;"
rules we need to implement in any case, I don't see a way around that.
The same list item can now be mapped (or unmapped) multiple times on the same
construct.

	Jakub