Re: [gomp4.1] map clause parsing improvements

[Thomas Schwinge <thomas@codesourcery.com>]

[-- Attachment #1: Type: text/plain, Size: 3334 bytes --]

Hi!

On Mon, 19 Oct 2015 12:34:08 +0200, Jakub Jelinek <jakub@redhat.com> wrote:
> On Mon, Oct 19, 2015 at 12:20:23PM +0200, Thomas Schwinge wrote:
> > > +    /* Decrement usage count and deallocate if zero.  */
> > > +    GOMP_MAP_RELEASE =			(GOMP_MAP_FLAG_ALWAYS
> > > +					 | GOMP_MAP_FORCE_DEALLOC)
> > >    };
> > 
> > I have not yet read the OpenMP 4.1/4.5 standard, but it's not obvious to
> > me here how the GOMP_MAP_FLAG_ALWAYS flag relates to the OpenMP release
> > clause (GOMP_MAP_RELEASE here)?  Shouldn't GOMP_MAP_RELEASE be
> > "(GOMP_MAP_FLAG_SPECIAL_1 | 3)" or similar?
> 
> It isn't related to always, but always really is something that affects
> solely the data movement (i.e. to, from, tofrom), and while it can be
> specified elsewhere, it makes no difference.  Wasting one bit just for that
> is something we don't have the luxury for, which is why I've started using
> that bit for other OpenMP stuff (it acts there like GOMP_MAP_FLAG_SPECIAL_2
> to some extent).  It is not just release, but also the struct mapping etc.
> I'll still need to make further changes, because the rules for mapping
> structure element pointer/reference based array sections and structure
> element references have changed again.

Hmm, I do think we should allow the luxury to use its own bit for
GOMP_MAP_FLAG_ALWAYS -- we can extend the interface later, should we
really find uses for the other two remaining bits -- or if not using a
separate bit, at least make sure that GOMP_MAP_FLAG_ALWAYS is not used as
a flag.  See, for example, the following occasions where
GOMP_MAP_FLAG_ALWAYS is used as a flag: these conditionals will also be
matched for GOMP_MAP_STRUCT, GOMP_MAP_DELETE_ZERO_LEN_ARRAY_SECTION, and
GOMP_MAP_RELEASE.  I have not analyzed whether that is erroneous or not,
but it surely is confusing?

    $ < gcc/gimplify.c grep -C3 GOMP_MAP_FLAG_ALWAYS
                          struct_map_to_clause->put (decl, *list_p);
                          list_p = &OMP_CLAUSE_CHAIN (*list_p);
                          flags = GOVD_MAP | GOVD_EXPLICIT;
                          if (OMP_CLAUSE_MAP_KIND (c) & GOMP_MAP_FLAG_ALWAYS)
                            flags |= GOVD_SEEN;
                          goto do_add_decl;
                        }
    --
                          tree *sc = NULL, *pt = NULL;
                          if (!ptr && TREE_CODE (*osc) == TREE_LIST)
                            osc = &TREE_PURPOSE (*osc);
                          if (OMP_CLAUSE_MAP_KIND (c) & GOMP_MAP_FLAG_ALWAYS)
                            n->value |= GOVD_SEEN;
                          offset_int o1, o2;
                          if (offset)
    --
              n = splay_tree_lookup (ctx->variables, (splay_tree_key) decl);
              if ((ctx->region_type & ORT_TARGET) != 0
                  && !(n->value & GOVD_SEEN)
                  && ((OMP_CLAUSE_MAP_KIND (c) & GOMP_MAP_FLAG_ALWAYS) == 0
                      || OMP_CLAUSE_MAP_KIND (c) == GOMP_MAP_STRUCT))
                {
                  remove = true;

I'd suggest turning GOMP_MAP_FLAG_ALWAYS into GOMP_MAP_FLAG_SPECIAL_2,
and then provide a GOMP_MAP_ALWAYS_P that evaluates to true just for the
three "always,to", "always,from", and "always,tofrom" cases.


Grüße,
 Thomas

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 472 bytes --]