public inbox for gcc-patches@gcc.gnu.org
 help / color / mirror / Atom feed
From: Jakub Jelinek <jakub@redhat.com>
To: Chung-Lin Tang <cltang@codesourcery.com>
Cc: Tobias Burnus <tobias@codesourcery.com>,
	gcc-patches <gcc-patches@gcc.gnu.org>,
	Hafiz Abid Qadeer <abid_qadeer@mentor.com>,
	Andrew Stubbs <ams@codesourcery.com>
Subject: Re: [PATCH, OpenMP, v4] Implement uses_allocators clause for target regions
Date: Mon, 13 Jun 2022 16:04:58 +0200	[thread overview]
Message-ID: <YqdECrcUEUke/8Ou@tucnak> (raw)
In-Reply-To: <075e0483-6d7b-3bbc-a718-b0872a870f85@codesourcery.com>

On Mon, Jun 13, 2022 at 09:29:34PM +0800, Chung-Lin Tang wrote:
> > I was hoping you'd drop all this.
> > Seehttps://gcc.gnu.org/r13-1002
> > for implementation (both C and C++ FE) of something very similar,
> > the only difference there is that in the case of linear clause, it is
> > looking for
> > val
> > ref
> > uval
> > step ( whatever )
> > followed by , or )
> > (anod ref and uval not in C FE),
> > while you are looking for
> > memspace ( whatever )
> > traits ( whatever )
> > followed by : or by , (in case of , repeat).
> > But in both cases you can actually use the same parser APIs
> > for raw token pre-parsing to just compute if it is the modifier
> > syntax or not, set bool has_modifiers based on that (when you
> > come over probably valid syntax followed by CPP_COLON).
> 
> The linear clause doesn't have the legacy 'allocator1(t1), allocator2(t2), ...' requirement,
> and c_parser_omp_variable_list doesn't seem to support this pattern.

True, but I don't see why it is relevant.

> Also, the way c_parser_omp_clause_linear is implemented doesn't support the requirement
> you mentioned earlier of allowing the use of "memspace", "traits" as the allocator name when
> it's actually not a modifier.

No, it is exactly the same thing.
As you can see e.g. in the testsuite coverage I've committed in the linear
patch, in the linear clause case after : either one uses a modifier syntax,
or everything after the : is the step expression (assignment-expression in
C/C++).  There is parsing ambiguity and the spec says that it is resolved
to the modifier syntax in that case.
Say if I have:
constexpr int step (int x) { return x; }
and use
linear (a, b, c : step (1))
then it is the modifier syntax (incompatible change from 5.1), while
linear (a, b, c : step (1) + 0)
linear (a, b, c : (step (1)))
linear (a, b, c : 0 + step (1))
etc. have step expressions.  The spec wording is such that it doesn't even
have to be discovered by strictly tentative parsing (like in GCC the C++ and
Fortran FEs do but C FE doesn't), modifier syntax wins if one sees the
modifiers with balanced () after it if it is complex, followed by , or ) as
a terminator of a single modifier.
The first raw token walk in the patch is just a fast "tentative" parsing
check whether it is modifier syntax or not, if it is, then we just parse it
as modifiers, otherwise parse it as expression.

The uses_allocator is similar, although in that case it actually isn't
a parsing ambiguity, just that we need arbitrary number of tokens look-ahead
to decide.  We need to check if the tokens after uses_allocators (
look like one or more complex modifiers (with the 2 modifier names and just
a balanced ()s after them - in the uses_allocators case currently all
supported modifiers are complex), if yes and it is followed by : token,
it is the modifiers syntax, otherwise it is not.
So say:
#include <omp.h>
void foo (void)
{
  omp_alloc_handle_t traits, x;
  const omp_alloctrait_t my_traits[] = { ... };
  #pragma omp target uses_allocators (traits (my_traits) : x)
  ;
  #pragma omp target uses_allocators (traits (my_traits), x (my_traits))
  ;
  #pragma omp target uses_allocators (traits (my_traits), omp_high_mem_bw_mem_alloc)
  ;
  #pragma omp target uses_allocators (traits (my_traits))
  ;
}
All the clauses above start with the same tokens, but depending on what
follows we need to either parse it as the modifier syntax (the first
directive) or as the compatibility syntax (the rest).

Which is why I was suggesting to do this quick raw token parsing check
if it is the modifier syntax or not.
If it is, parse modifiers and : and then you know to expect a single
allocator without ()s after it (e.g. you could use
c_parser_omp_variable_list etc. and just verify afterwards the list
has a single entry in it).
If it is not, it might still be old or new syntax, the latter only if
the list contains a single var and not followed by ()s and sure, you need
to write a parsing loop for that.  It isn't the same thing as the modifier
loop though, modifiers start with a keyword, the allocator list with
a identifier for the variable.

For uses_allocators, we can then even simplify when we almost finish
OpenMP 6.0 support, if the old style syntax uses_allocators are gone
by then, we could decide if it is a modifier syntax or not just by
looking at first 2 tokens, whether the first token is allowed modifier
keyword and whether it is followed by (, then we could commit to
modifier parsing right away.  And the loop to do the ()s parsing
can go too...

	Jakub


  reply	other threads:[~2022-06-13 14:05 UTC|newest]

Thread overview: 20+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-06 13:20 [PATCH, OpenMP] " Chung-Lin Tang
2022-05-06 16:40 ` Tobias Burnus
2022-05-10 11:29   ` [PATCH, OpenMP, v2] " Chung-Lin Tang
2022-05-19 16:00     ` Jakub Jelinek
2022-05-19 17:02       ` Andrew Stubbs
2022-05-19 17:55         ` Jakub Jelinek
2022-05-20  6:59       ` Tobias Burnus
2022-05-19 17:46     ` Jakub Jelinek
2022-05-30 14:43       ` Chung-Lin Tang
2022-05-30 17:23         ` Jakub Jelinek
2022-05-31 10:02           ` Jakub Jelinek
2022-06-06 13:19             ` Chung-Lin Tang
2022-06-06 13:22               ` Jakub Jelinek
2022-06-06 13:38                 ` Chung-Lin Tang
2022-06-06 13:42                   ` Jakub Jelinek
2022-06-09  6:21             ` [PATCH, OpenMP, v4] " Chung-Lin Tang
2022-06-09 12:22               ` Jakub Jelinek
2022-06-13 13:29                 ` Chung-Lin Tang
2022-06-13 14:04                   ` Jakub Jelinek [this message]
2023-02-09 11:26 ` [og12] '{c-c++-common,gfortran.dg}/gomp/uses_allocators-*' -> 'libgomp.{c-c++-common,fortran}/uses_allocators-*' (was: [PATCH, OpenMP] Implement uses_allocators clause for target regions) Thomas Schwinge

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YqdECrcUEUke/8Ou@tucnak \
    --to=jakub@redhat.com \
    --cc=abid_qadeer@mentor.com \
    --cc=ams@codesourcery.com \
    --cc=cltang@codesourcery.com \
    --cc=gcc-patches@gcc.gnu.org \
    --cc=tobias@codesourcery.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).