Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Thomas Schwinge]

Hi!

Half a decade later...  ;-)

On 2015-11-05T18:10:49-0800, Cesar Philippidis  wrote:
> I've applied this patch to trunk.
> [...]

> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/goacc/template.C
> @@ -0,0 +1,141 @@
> +[...]
> +#pragma acc atomic capture
> +c = b++;
> +
> +#pragma atomic update
> +c++;
> +
> +#pragma acc atomic read
> +b = a;
> +
> +#pragma acc atomic write
> +b = a;
> +[...]

Pushed "[OpenACC] Fix '#pragma atomic update' typo in
'g++.dg/goacc/template.C'" to master branch in commit
6099b9cc8ce70d2ec7f2fc9f71da95fbb66d335f, see attached.


(Did I suggest to enable '-Wunknown-pragmas' for '-fopenacc'/'-fopenmp*',
or if that's not permissible, then at least do it in the relevant
testsuite '*.exp' files?)


Grüße
 Thomas


-
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
>From 6099b9cc8ce70d2ec7f2fc9f71da95fbb66d335f Mon Sep 17 00:00:00 2001
From: Thomas Schwinge 
Date: Wed, 21 Jul 2021 08:20:18 +0200
Subject: [PATCH] [OpenACC] Fix '#pragma atomic update' typo in
 'g++.dg/goacc/template.C'
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

[...]/g++.dg/goacc/template.C:58: warning: ignoring ‘#pragma atomic update’ [-Wunknown-pragmas]
   58 | #pragma atomic update
  |

Small fix-up for r229832 (commit 7a5e4956cc026cba54159d5c764486ac4151db85)
"[openacc] tile, independent, default, private and firstprivate support in
c/++".

	gcc/testsuite/
	* g++.dg/goacc/template.C: Fix '#pragma atomic update' typo.
---
 gcc/testsuite/g++.dg/goacc/template.C | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gcc/testsuite/g++.dg/goacc/template.C b/gcc/testsuite/g++.dg/goacc/template.C
index 8bcd2a1ce43..51a3f54e43f 100644
--- a/gcc/testsuite/g++.dg/goacc/template.C
+++ b/gcc/testsuite/g++.dg/goacc/template.C
@@ -55,7 +55,7 @@ oacc_parallel_copy (T a)
 #pragma acc atomic capture
   c = b++;
 
-#pragma atomic update
+#pragma acc atomic update
   c++;
 
 #pragma acc atomic read
-- 
2.30.2

[gomp4] Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Thomas Schwinge]

Hi!

On Thu, 5 Nov 2015 18:10:49 -0800, Cesar Philippidis  
wrote:
> I've applied this patch to trunk. It also includes the fortran and
> template changes. [...]

> Also, because of these reduction problems, I decided not to merge
> combined_loops.f90 with combined-directives.f90 yet because the latter
> relies on scanning which would fail with the errors detected during
> gimplfication. I'm planning on adding a couple of more test cases [...]

ACK.


Merging Cesar's trunk r229832 into gomp-4_0-branch, I tried to replicate
this part of his commit:

> --- a/gcc/fortran/trans-openmp.c
> +++ b/gcc/fortran/trans-openmp.c
> @@ -3449,16 +3478,33 @@ gfc_trans_oacc_combined_directive (gfc_code *code)
> [...]
> +  loop_clauses.lists[OMP_LIST_PRIVATE]
> + = construct_clauses.lists[OMP_LIST_PRIVATE];
> +  loop_clauses.lists[OMP_LIST_REDUCTION]
> + = construct_clauses.lists[OMP_LIST_REDUCTION];
> [...]
> +  construct_clauses.lists[OMP_LIST_PRIVATE] = NULL;
> [...]

... in the gcc/fortran/trans-openmp.c:gfc_filter_oacc_combined_clauses
function that we're using on gomp-4_0-branch, but ran into ICEs that
looked like "double free" (or similar; maybe related to the slightly
different structure of this code due to device_type support), so applied
the following hack and XFAILs as part of the merge, which needs to be
resolved later (on gomp-4_0-branch).  (A few related TODOs also remain to
be addressed in gcc/testsuite/c-c++-common/goacc/combined-directives.c.)

--- gcc/fortran/trans-openmp.c
+++ gcc/fortran/trans-openmp.c
@@ -3691,12 +3691,14 @@ gfc_filter_oacc_combined_clauses (gfc_omp_clauses 
**orig_clauses,
  (*orig_clauses)->tile = false;
  (*loop_clauses)->tile_list = (*orig_clauses)->tile_list;
  (*orig_clauses)->tile_list = NULL;
{+#if 0 /* TODO */+}
  (*loop_clauses)->lists[OMP_LIST_PRIVATE]
= (*orig_clauses)->lists[OMP_LIST_PRIVATE];
  (*orig_clauses)->lists[OMP_LIST_PRIVATE] = NULL;
  (*loop_clauses)->lists[OMP_LIST_REDUCTION]
= (*orig_clauses)->lists[OMP_LIST_REDUCTION];
  /* Don't reset (*orig_clauses)->lists[OMP_LIST_REDUCTION].  */
{+#endif+}

  (*loop_clauses)->device_types = (*orig_clauses)->device_types;

--- gcc/testsuite/gfortran.dg/goacc/combined-directives.f90
+++ gcc/testsuite/gfortran.dg/goacc/combined-directives.f90
@@ -4,6 +4,9 @@
! { dg-options "-fopenacc -fdump-tree-gimple" }

! TODO
{+! Fix OMP_LIST_PRIVATE and OMP_LIST_REDUCTION splitting in+}
{+! gcc/fortran/trans-openmp.c:gfc_filter_oacc_combined_clauses, and remove 
tree+}
{+! scanning XFAILs.+}
! Enable and update tree scanning for reduction clauses.
! Enable/add/update device_type clauses and tree scanning.

@@ -154,12 +157,12 @@ subroutine test
!  !$acc end kernels loop
end subroutine test

! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. 
collapse.2." 2 "gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. gang" 2 "gimple" {+{ 
xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. worker" 2 
"gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. vector" 2 
"gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. seq" 2 
"gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. auto" 2 
"gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. private.j. tile.2, 3" 
2 "gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "acc loop private.i. independent" 2 
"gimple" {+{ xfail *-*-* }+} } }
! { dg-final { scan-tree-dump-times "private.z" 2 "gimple" } }

With that, committed to gomp-4_0-branch in r230012:

commit 80ebc1e747e3422e0cc57c4c11387ec26bbf8814
Merge: 175e08b ef014f9
Author: tschwinge 
Date:   Mon Nov 9 11:10:12 2015 +

svn merge -r 229831:229832 svn+ssh://gcc.gnu.org/svn/gcc/trunk


git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/branches/gomp-4_0-branch@230012 
138bc75d-0d04-0410-961f-82ee72b054a4


Grüße
 Thomas


signature.asc
Description: PGP signature

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Nathan Sidwell]

On 11/06/15 01:50, Jakub Jelinek wrote:

On Thu, Nov 05, 2015 at 06:10:49PM -0800, Cesar Philippidis wrote:

I've applied this patch to trunk. It also includes the fortran and
template changes. Note that there is a new regression in
gfortran.dg/goacc/combined_loop.f90. Basically, the gimplifier is
complaining about reduction variables appearing in multiple clauses.
E.g. 'acc parallel reduction(+:var) copy(var)'. Nathan's upcoming
gimplifier changes should address that.


If you are relying on the OMP_CLAUSE_MAP_PRIVATE flag that I've added
on gomp-4_1-branch and then removed yesterday, feel free to re-add it,
but of course never set it for OpenMP, just OpenACC constructs
(so for OpenMP keep the gimplifier assertion, for OpenACC set it).



FWIW not noticed a problem with my firstprivate reworking, rebased ontop 
yesterday's  openmp merge


nathan

--
Nathan Sidwell

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Nathan Sidwell]

On 11/05/15 21:10, Cesar Philippidis wrote:

I've applied this patch to trunk. It also includes the fortran and
template changes. Note that there is a new regression in
gfortran.dg/goacc/combined_loop.f90. Basically, the gimplifier is
complaining about reduction variables appearing in multiple clauses.
E.g. 'acc parallel reduction(+:var) copy(var)'. Nathan's upcoming
gimplifier changes should address that.

Also, because of these reduction problems, I decided not to merge
combined_loops.f90 with combined-directives.f90 yet because the latter
relies on scanning which would fail with the errors detected during
gimplfication. I'm planning on adding a couple of more test cases once
acc reductions are working on trunk.


Reductions are already on trunk.  do you mean one or both of:
1) firstprivate
2) default handlinng (depends on #1)

I expect to post #1 today.  #2 (a smaller patch) may not make it today, as I 
have to rebase it on the reworking of #1 I did to remove the two enums I 
disucssed with Jakub.


nathan
--
Nathan Sidwell

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Thomas Schwinge]

Hi Cesar!

On Tue, 3 Nov 2015 14:16:59 -0800, Cesar Philippidis  
wrote:
> This patch does the following to the c and c++ front ends:

>  * updates c_oacc_split_loop_clauses to filter out the loop clauses
>from combined parallel/kernels loops

>   gcc/c-family/
>   * c-omp.c (c_oacc_split_loop_clauses): Make TILE, GANG, WORKER, VECTOR,
>   AUTO, SEQ and independent as loop clauses.  Associate REDUCTION
>   clauses with parallel and kernels.

> --- a/gcc/c-family/c-omp.c
> +++ b/gcc/c-family/c-omp.c
> @@ -709,12 +709,21 @@ c_oacc_split_loop_clauses (tree clauses, tree 
> *not_loop_clauses)
>  
>switch (OMP_CLAUSE_CODE (clauses))
>  {
> +   /* Loop clauses.  */
>   case OMP_CLAUSE_COLLAPSE:
> - case OMP_CLAUSE_REDUCTION:
> + case OMP_CLAUSE_TILE:
> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_WORKER:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_AUTO:
> + case OMP_CLAUSE_SEQ:
> + case OMP_CLAUSE_INDEPENDENT:
> OMP_CLAUSE_CHAIN (clauses) = loop_clauses;
> loop_clauses = clauses;
> break;
>  
> +   /* Parallel/kernels clauses.  */
> +
>   default:
> OMP_CLAUSE_CHAIN (clauses) = *not_loop_clauses;
> *not_loop_clauses = clauses;

Contrary to your ChangeLog entry, this is not duplicating but is moving
OMP_CLAUSE_REDUCTION handling.  Is that intentional?  (And, doesn't
anything break in the testsuite?)


Grüße
 Thomas


signature.asc
Description: PGP signature

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

On 11/05/2015 09:13 AM, Nathan Sidwell wrote:
> On 11/05/15 12:01, Thomas Schwinge wrote:
> 
>> On Thu, 5 Nov 2015 06:47:58 -0800, Cesar Philippidis
>>  wrote:
>>> On 11/05/2015 04:14 AM, Thomas Schwinge wrote:
> 
>>> Sorry, I must have mis-phrased it. The spec is unclear here. There are
>>> three possible ways to interpret 'acc parallel loop reduction':
>>>
>>>1. acc parallel reduction
>>>   acc loop
>>
>> This is what you propose in your patch, but I don't think that makes
>> sense, or does it?  I'm happy to learn otherwise, but in my current
>> understanding, a reduction clause needs to be attached (at least) to the
>> innermost construct where reductions are to be processed.  (Let's also
> 
> Correct, the  above interpretation must be wrong.
> 
>> consider multi-level gang/worker/vector loops/reductions.)  So, either:
>>
>>>2. acc parallel
>>>   acc loop reduction
>>
>> ... this, or even this:
>>
>>>3. acc parallel reduction
>>>   acc loop reduction
>>
>> ..., which I'm not sure what the execution model implementation requires.
>> (Nathan?)
> 
> interpretation #2 is sufficient, I think. However, both are lacking a
> 'copy (reduction_var)', clause as otherwise there's nothing changing the
> default data attribute of 'firstprivate' (working on that patch). 
> Perhaps 'reduction' on 'parallel'  is meant to imply that  (because
> that's what makes sense), but the std doesn't say it.
> 
> In summary it's probably safe to implement interpretation #3.  That way
> we can implement the hypothesis that reductions at the outer construct
> imply copy.

OK, #3 it is.

>> And while we're at it: the very same question also applies to the private
>> clause, which -- contrary to all other (as far as I remember) clauses --
>> also is applicable to both the parallel and loop constructs:
>>
>>  #pragma acc parallel loop private([...])
>>
>> ... is to be decomposed into which of the following:
>>
>>  #pragma acc parallel private([...])
>>  #pragma acc loop
>>
>>  #pragma acc parallel
>>  #pragma acc loop private([...])
>>
>>  #pragma acc parallel private([...])
>>  #pragma acc loop private([...])
>>
>> (There is no private clause allowed to be specified with the kernels
>> construct for what it's worth, but that doesn't mean we couldn't use it
>> internally, of course, if so required.)
> 
> I think interpretation #2 or #3 make sense, and I suspect result in the
> same emitted code.

I'll probably go #2 here to make life easier with kernels.

After I make these changes (and the c++ template updates), I'll apply
them to trunk and backport them to gomp4. Thank you Jakub, Thomas and
Nathan for reviewing these patches.

Cesar

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Thomas Schwinge]

Hi!

Nathan, question here about clause splitting for combined OpenACC
"parallel loop" and "kernels loop" constructs, in particular:

#pragma acc parallel loop reduction([...])

On Thu, 5 Nov 2015 06:47:58 -0800, Cesar Philippidis  
wrote:
> On 11/05/2015 04:14 AM, Thomas Schwinge wrote:
> 
> > On Tue, 3 Nov 2015 14:16:59 -0800, Cesar Philippidis 
> >  wrote:
> >> This patch does the following to the c and c++ front ends:
> > 
> >>  * updates c_oacc_split_loop_clauses to filter out the loop clauses
> >>from combined parallel/kernels loops
> > 
> >>gcc/c-family/
> >>* c-omp.c (c_oacc_split_loop_clauses): Make TILE, GANG, WORKER, VECTOR,
> >>AUTO, SEQ and independent as loop clauses.  Associate REDUCTION
> >>clauses with parallel and kernels.
> > 
> >> --- a/gcc/c-family/c-omp.c
> >> +++ b/gcc/c-family/c-omp.c
> >> @@ -709,12 +709,21 @@ c_oacc_split_loop_clauses (tree clauses, tree 
> >> *not_loop_clauses)
> >>  
> >>switch (OMP_CLAUSE_CODE (clauses))
> >>  {
> >> +/* Loop clauses.  */
> >>case OMP_CLAUSE_COLLAPSE:
> >> -  case OMP_CLAUSE_REDUCTION:
> >> +  case OMP_CLAUSE_TILE:
> >> +  case OMP_CLAUSE_GANG:
> >> +  case OMP_CLAUSE_WORKER:
> >> +  case OMP_CLAUSE_VECTOR:
> >> +  case OMP_CLAUSE_AUTO:
> >> +  case OMP_CLAUSE_SEQ:
> >> +  case OMP_CLAUSE_INDEPENDENT:
> >>  OMP_CLAUSE_CHAIN (clauses) = loop_clauses;
> >>  loop_clauses = clauses;
> >>  break;
> >>  
> >> +/* Parallel/kernels clauses.  */
> >> +
> >>default:
> >>  OMP_CLAUSE_CHAIN (clauses) = *not_loop_clauses;
> >>  *not_loop_clauses = clauses;
> > 
> > Contrary to your ChangeLog entry, this is not duplicating but is moving
> > OMP_CLAUSE_REDUCTION handling.  Is that intentional?  (And, doesn't
> > anything break in the testsuite?)
> 
> Sorry, I must have mis-phrased it. The spec is unclear here. There are
> three possible ways to interpret 'acc parallel loop reduction':
> 
>   1. acc parallel reduction
>  acc loop

This is what you propose in your patch, but I don't think that makes
sense, or does it?  I'm happy to learn otherwise, but in my current
understanding, a reduction clause needs to be attached (at least) to the
innermost construct where reductions are to be processed.  (Let's also
consider multi-level gang/worker/vector loops/reductions.)  So, either:

>   2. acc parallel
>  acc loop reduction

... this, or even this:

>   3. acc parallel reduction
>  acc loop reduction

..., which I'm not sure what the execution model implementation requires.
(Nathan?)

And while we're at it: the very same question also applies to the private
clause, which -- contrary to all other (as far as I remember) clauses --
also is applicable to both the parallel and loop constructs:

#pragma acc parallel loop private([...])

... is to be decomposed into which of the following:

#pragma acc parallel private([...])
#pragma acc loop

#pragma acc parallel
#pragma acc loop private([...])

#pragma acc parallel private([...])
#pragma acc loop private([...])

(There is no private clause allowed to be specified with the kernels
construct for what it's worth, but that doesn't mean we couldn't use it
internally, of course, if so required.)


> You told me to make all of the front ends consistent, and since I
> started working on fortran first, I had c and c++ follow what it was doing.

Well, I had not asked you to "blindly" ;-) make the front ends
consistent, but rather to figure out why currently C/C++ vs. Fortran are
doing things differently, and then make that consistent, as needed.

> I haven't observed any regressions with this in in place. Then again,
> maybe we don't have sufficient test coverage. I'll do more testing.

Thanks!  (I do see some -- but not very many, curiously! -- regressions
when doing (only) this change on gomp-4_0-branch.)

As far as I'm concerned, this analysis/testing can also be done later on.
(Jakub?)  In that case, for now please just keep OMP_CLAUSE_REDUCTION as
it has been handled before.


Grüße
 Thomas


signature.asc
Description: PGP signature

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Nathan Sidwell]

On 11/05/15 12:01, Thomas Schwinge wrote:


On Thu, 5 Nov 2015 06:47:58 -0800, Cesar Philippidis  
wrote:

On 11/05/2015 04:14 AM, Thomas Schwinge wrote:



Sorry, I must have mis-phrased it. The spec is unclear here. There are
three possible ways to interpret 'acc parallel loop reduction':

   1. acc parallel reduction
  acc loop


This is what you propose in your patch, but I don't think that makes
sense, or does it?  I'm happy to learn otherwise, but in my current
understanding, a reduction clause needs to be attached (at least) to the
innermost construct where reductions are to be processed.  (Let's also


Correct, the  above interpretation must be wrong.


consider multi-level gang/worker/vector loops/reductions.)  So, either:


   2. acc parallel
  acc loop reduction


... this, or even this:


   3. acc parallel reduction
  acc loop reduction


..., which I'm not sure what the execution model implementation requires.
(Nathan?)


interpretation #2 is sufficient, I think. However, both are lacking a 'copy 
(reduction_var)', clause as otherwise there's nothing changing the default data 
attribute of 'firstprivate' (working on that patch).  Perhaps 'reduction' on 
'parallel'  is meant to imply that  (because that's what makes sense), but the 
std doesn't say it.


In summary it's probably safe to implement interpretation #3.  That way we can 
implement the hypothesis that reductions at the outer construct imply copy.



And while we're at it: the very same question also applies to the private
clause, which -- contrary to all other (as far as I remember) clauses --
also is applicable to both the parallel and loop constructs:

 #pragma acc parallel loop private([...])

... is to be decomposed into which of the following:

 #pragma acc parallel private([...])
 #pragma acc loop

 #pragma acc parallel
 #pragma acc loop private([...])

 #pragma acc parallel private([...])
 #pragma acc loop private([...])

(There is no private clause allowed to be specified with the kernels
construct for what it's worth, but that doesn't mean we couldn't use it
internally, of course, if so required.)


I think interpretation #2 or #3 make sense, and I suspect result in the same 
emitted code.


nathan

--
Nathan Sidwell

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Jakub Jelinek]

Hi!

On Wed, Nov 04, 2015 at 09:55:49AM -0800, Cesar Philippidis wrote:

So, you are going to deal with gang parsing incrementally?
Fine with me.

> +/* OpenACC 2.0:
> +   tile ( size-expr-list ) */
> +
> +static tree
> +c_parser_oacc_clause_tile (c_parser *parser, tree list)
> +{
> +  tree c, expr = error_mark_node;
> +  location_t loc, expr_loc;
> +  tree tile = NULL_TREE;
> +
> +  check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile");
> +
> +  loc = c_parser_peek_token (parser)->location;
> +  if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> +return list;
> +
> +  vec *tvec = make_tree_vector ();

Seems I've misread your patch, thought you are using TREE_VEC, while
you are actually using TREE_LIST, but populating it in a weird way.
I think more efficient would be just to
  tree tile = NULL_TREE;
here, then:

> +
> +  vec_safe_push (tvec, expr);

  tile = tree_cons (NULL_TREE, expr, tile);

> +  if (c_parser_next_token_is (parser, CPP_COMMA))
> + c_parser_consume_token (parser);
> +}
> +  while (c_parser_next_token_is_not (parser, CPP_CLOSE_PAREN));
> +
> +  /* Consume the trailing ')'.  */
> +  c_parser_consume_token (parser);
> +
> +  c = build_omp_clause (loc, OMP_CLAUSE_TILE);
> +  tile = build_tree_list_vec (tvec);

  tile = nreverse (tile);
  
> +  OMP_CLAUSE_TILE_LIST (c) = tile;
> +  OMP_CLAUSE_CHAIN (c) = list;
> +  release_tree_vector (tvec);

and remove the release_tree_vector calls.

> +static tree
> +cp_parser_oacc_clause_tile (cp_parser *parser, location_t clause_loc, tree 
> list)

This is already too long line.

> + case OMP_CLAUSE_TILE:
> +   {
> + tree list = OMP_CLAUSE_TILE_LIST (c);
> +
> + while (list)

I'd say
for (tree list = OMP_CLAUSE_TILE_LIST (c);
 list; list = TREE_CHAIN (list))
would be more readable.

> --- a/gcc/gimplify.c
> +++ b/gcc/gimplify.c
> @@ -6995,9 +6995,18 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
> *pre_p,
>   remove = true;
> break;
>  
> + case OMP_CLAUSE_TILE:
> +   for (tree list = OMP_CLAUSE_TILE_LIST (c); !remove && list;
> +list = TREE_CHAIN (list))
> + {
> +   if (gimplify_expr (_VALUE (list), pre_p, NULL,
> +  is_gimple_val, fb_rvalue) == GS_ERROR)
> + remove = true;
> + }

After all, you are already using for here ;)

Otherwise LGTM, but please clear with Thomas, I think he had some issues
with the patch.

Jakub

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Jakub Jelinek]

On Thu, Nov 05, 2015 at 06:10:49PM -0800, Cesar Philippidis wrote:
> I've applied this patch to trunk. It also includes the fortran and
> template changes. Note that there is a new regression in
> gfortran.dg/goacc/combined_loop.f90. Basically, the gimplifier is
> complaining about reduction variables appearing in multiple clauses.
> E.g. 'acc parallel reduction(+:var) copy(var)'. Nathan's upcoming
> gimplifier changes should address that.

If you are relying on the OMP_CLAUSE_MAP_PRIVATE flag that I've added
on gomp-4_1-branch and then removed yesterday, feel free to re-add it,
but of course never set it for OpenMP, just OpenACC constructs
(so for OpenMP keep the gimplifier assertion, for OpenACC set it).

Jakub

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

I've applied this patch to trunk. It also includes the fortran and
template changes. Note that there is a new regression in
gfortran.dg/goacc/combined_loop.f90. Basically, the gimplifier is
complaining about reduction variables appearing in multiple clauses.
E.g. 'acc parallel reduction(+:var) copy(var)'. Nathan's upcoming
gimplifier changes should address that.

Also, because of these reduction problems, I decided not to merge
combined_loops.f90 with combined-directives.f90 yet because the latter
relies on scanning which would fail with the errors detected during
gimplfication. I'm planning on adding a couple of more test cases once
acc reductions are working on trunk.

Cesar
2015-11-05  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  


	gcc/
	* gimplify.c (gimplify_scan_omp_clauses): Add support for
	OMP_CLAUSE_TILE.  Update handling of OMP_CLAUSE_INDEPENDENT.
	(gimplify_adjust_omp_clauses): Likewise.
	* omp-low.c (scan_sharing_clauses): Add support for OMP_CLAUSE_TILE.
	* tree-core.h (enum omp_clause_code): Add OMP_CLAUSE_TILE.
	* tree-pretty-print.c (dump_omp_clause): Handle OMP_CLAUSE_TILE.
	* tree.c (omp_clause_num_ops): Add an entry for OMP_CLAUSE_TILE.
	(omp_clause_code_name): Likewise.
	(walk_tree_1): Handle OMP_CLAUSE_TILE.
	* tree.h (OMP_TILE_LIST): New macro.

	gcc/c-family/
	* c-omp.c (c_oacc_split_loop_clauses): Make TILE, GANG, WORKER, VECTOR,
	AUTO, SEQ, INDEPENDENT and PRIVATE loop clauses.  Associate REDUCTION
	clauses with parallel and kernels and loops.
	* c-pragma.h (enum pragma_omp_clause): Add entries for
	PRAGMA_OACC_CLAUSE_{INDEPENDENT,TILE,DEFAULT}.
	* pt.c (tsubst_omp_clauses): Add support for OMP_CLAUSE_{NUM_GANGS,
	NUM_WORKERS,VECTOR_LENGTH,GANG,WORKER,VECTOR,ASYNC,WAIT,TILE,AUTO,
	INDEPENDENT,SEQ}. 
	(tsubst_expr): Add support for OMP_CLAUSE_{KERNELS,PARALLEL,LOOP}.

	gcc/c/
	* c-parser.c (c_parser_omp_clause_name): Add support for
	PRAGMA_OACC_CLAUSE_INDEPENDENT and PRAGMA_OACC_CLAUSE_TILE.
	(c_parser_omp_clause_default): Add is_oacc argument. Handle
	default(none) in OpenACC.
	(c_parser_oacc_shape_clause): Allow pointer variables as gang static
	arguments.
	(c_parser_oacc_clause_tile): New function.
	(c_parser_oacc_all_clauses): Add support for OMP_CLAUSE_DEFAULT,
	OMP_CLAUSE_INDEPENDENT and OMP_CLAUSE_TILE.
	(OACC_LOOP_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{PRIVATE,INDEPENDENT,
	TILE}.
	(OACC_KERNELS_MASK): Add PRAGMA_OACC_CLAUSE_DEFAULT.
	(OACC_PARALLEL_MASK): Add PRAGMA_OACC_CLAUSE_{DEFAULT,PRIVATE,
	FIRSTPRIVATE}.
	(c_parser_omp_all_clauses): Update call to c_parser_omp_clause_default.
	(c_parser_oacc_update): Update the error message for missing clauses.
	* c-typeck.c (c_finish_omp_clauses): Add support for OMP_CLAUSE_TILE
	and OMP_CLAUSE_INDEPENDENT.

	gcc/cp/
	* parser.c (cp_parser_omp_clause_name): Add support for
	PRAGMA_OACC_CLAUSE_INDEPENDENT and PRAGMA_OACC_CLAUSE_TILE.
	(cp_parser_oacc_shape_clause): Allow pointer variables as gang static
	arguments.
	(cp_parser_oacc_clause_tile): New function.
	(cp_parser_omp_clause_default): Add is_oacc argument. Handle
	default(none) in OpenACC.
	(cp_parser_oacc_all_clauses): Add support for
	(cp_parser_omp_all_clauses): Update call to
	cp_parser_omp_clause_default.
	PRAGMA_OACC_CLAUSE_{DEFAULT,INDEPENDENT,TILE,PRIVATE,FIRSTPRIVATE}.
	(OACC_LOOP_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{PRIVATE,INDEPENDENT,
	TILE}.
	(OACC_KERNELS_MASK): Add PRAGMA_OACC_CLAUSE_DEFAULT.
	(OACC_PARALLEL_MASK): Add PRAGMA_OACC_CLAUSE_{DEFAULT,PRIVATE,
	FIRSTPRIVATE}.
	(cp_parser_oacc_update): Update the error message for missing clauses.
	* semantics.c (finish_omp_clauses): Add support for
	OMP_CLAUSE_INDEPENDENT and OMP_CLAUSE_TILE.

2015-11-05  Cesar Philippidis  

	gcc/fortran/
	* openmp.c (gfc_match_omp_clauses): Update support for the tile
	and default clauses in OpenACC.
	(gfc_match_oacc_update): Error when data clauses are supplied.
	(oacc_compatible_clauses): Delete.
	(resolve_omp_clauses): Give special care for OpenACC reductions.
	Also update error reporting for the tile clause.
	(resolve_oacc_loop_blocks): Update error reporting for the tile clause.
	* trans-openmp.c (gfc_trans_omp_clauses): Update OMP_CLAUSE_SEQ. Add
	OMP_CLAUSE_{AUTO,TILE} and add support the the gang static argument.
	(gfc_trans_oacc_combined_directive): Update the list of clauses which
	are split to acc loops.


2015-11-05  Cesar Philippidis  
	Tom de Vries  
	Nathan Sidwell  
	Thomas Schwinge  

	gcc/testsuite/
	* c-c++-common/goacc/combined-directives.c: New test.
	* c-c++-common/goacc/loop-clauses.c: New test.
	* c-c++-common/goacc/tile.c: New test.
	* c-c++-common/goacc/loop-shape.c: Add test for pointer variable
	as gang static arguments.
	* c-c++-common/goacc/update-1.c: Adjust expected error message.
	* g++.dg/goacc/template.C: New test.
	* 

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

On 11/05/2015 04:14 AM, Thomas Schwinge wrote:

> On Tue, 3 Nov 2015 14:16:59 -0800, Cesar Philippidis  
> wrote:
>> This patch does the following to the c and c++ front ends:
> 
>>  * updates c_oacc_split_loop_clauses to filter out the loop clauses
>>from combined parallel/kernels loops
> 
>>  gcc/c-family/
>>  * c-omp.c (c_oacc_split_loop_clauses): Make TILE, GANG, WORKER, VECTOR,
>>  AUTO, SEQ and independent as loop clauses.  Associate REDUCTION
>>  clauses with parallel and kernels.
> 
>> --- a/gcc/c-family/c-omp.c
>> +++ b/gcc/c-family/c-omp.c
>> @@ -709,12 +709,21 @@ c_oacc_split_loop_clauses (tree clauses, tree 
>> *not_loop_clauses)
>>  
>>switch (OMP_CLAUSE_CODE (clauses))
>>  {
>> +  /* Loop clauses.  */
>>  case OMP_CLAUSE_COLLAPSE:
>> -case OMP_CLAUSE_REDUCTION:
>> +case OMP_CLAUSE_TILE:
>> +case OMP_CLAUSE_GANG:
>> +case OMP_CLAUSE_WORKER:
>> +case OMP_CLAUSE_VECTOR:
>> +case OMP_CLAUSE_AUTO:
>> +case OMP_CLAUSE_SEQ:
>> +case OMP_CLAUSE_INDEPENDENT:
>>OMP_CLAUSE_CHAIN (clauses) = loop_clauses;
>>loop_clauses = clauses;
>>break;
>>  
>> +  /* Parallel/kernels clauses.  */
>> +
>>  default:
>>OMP_CLAUSE_CHAIN (clauses) = *not_loop_clauses;
>>*not_loop_clauses = clauses;
> 
> Contrary to your ChangeLog entry, this is not duplicating but is moving
> OMP_CLAUSE_REDUCTION handling.  Is that intentional?  (And, doesn't
> anything break in the testsuite?)

Sorry, I must have mis-phrased it. The spec is unclear here. There are
three possible ways to interpret 'acc parallel loop reduction':

  1. acc parallel reduction
 acc loop

  2. acc parallel
 acc loop reduction

  3. acc parallel reduction
 acc loop reduction


You told me to make all of the front ends consistent, and since I
started working on fortran first, I had c and c++ follow what it was doing.

I haven't observed any regressions with this in in place. Then again,
maybe we don't have sufficient test coverage. I'll do more testing.

Cesar

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

On 11/04/2015 11:29 PM, Jakub Jelinek wrote:
> On Wed, Nov 04, 2015 at 08:58:32PM -0800, Cesar Philippidis wrote:
>> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
>> index e3f55a7..4424596 100644
>> --- a/gcc/cp/pt.c
>> +++ b/gcc/cp/pt.c
>> @@ -14395,6 +14395,15 @@ tsubst_omp_clauses (tree clauses, bool 
>> declare_simd, bool allow_fields,
>>  case OMP_CLAUSE_PRIORITY:
>>  case OMP_CLAUSE_ORDERED:
>>  case OMP_CLAUSE_HINT:
>> +case OMP_CLAUSE_NUM_GANGS:
>> +case OMP_CLAUSE_NUM_WORKERS:
>> +case OMP_CLAUSE_VECTOR_LENGTH:
>> +case OMP_CLAUSE_GANG:
> 
> GANG has two arguments, so you want to handle it differently, you need
> to tsubst both arguments.

Good catch. Support for the static argument was added after I added
template support in gomp4. I'll fix that.

>> +case OMP_CLAUSE_WORKER:
>> +case OMP_CLAUSE_VECTOR:
>> +case OMP_CLAUSE_ASYNC:
>> +case OMP_CLAUSE_WAIT:
>> +case OMP_CLAUSE_TILE:
> 
> I don't think tile will work well this way, if the only argument is a
> TREE_VEC, then I think you hit:
> case TREE_VEC:
>   /* A vector of template arguments.  */
>   gcc_assert (!type);
>   return tsubst_template_args (t, args, complain, in_decl);
> which does something very much different from making a copy of the TREE_VEC
> and calling tsubst_expr on each argument.
> Thus, either you need to handle it manually here, or think about different
> representation of OMP_CLAUSE_TILE?  It seems you allow at most one tile
> clause, so perhaps you could split the single source tile clause into one
> tile clause per expression in there (the only issue is that the FEs
> emit the clauses in last to first order, so you'd need to nreverse the
> tile clause list before adding it to the list of all clauses).

It shouldn't be difficult to call it manually here.

> Otherwise it looks ok, except:

How about the other patch, with the c and c++ FE changes? Is that one OK
for trunk now? Nathan's going to need it for this firstprivate changes
in the middle end.

>> diff --git a/gcc/testsuite/g++.dg/goacc/template-reduction.C 
>> b/gcc/testsuite/g++.dg/goacc/template-reduction.C
>> new file mode 100644
>> index 000..668eeb3
>> --- /dev/null
>> +++ b/gcc/testsuite/g++.dg/goacc/template-reduction.C
>> +++ b/gcc/testsuite/g++.dg/goacc/template.C
> 
> the testsuite coverage is orders of magnitude smaller than it should be.
> Just look at the amount of OpenMP template tests (both compile time and
> runtime):
> grep template libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep -l template 
> libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep template 
> gcc/testsuite/g++.dg/gomp/* | wc -l; grep -l template 
> gcc/testsuite/g++.dg/gomp/* | wc -l
> 629 # templates
> 45 # tests with templates
> 151 # templates
> 58 # tests with templates
> and even that is really not sufficient.  From my experience, special care is
> needed in template tests to test both non-type dependent and type-dependent
> cases (e.g. some diagnostics is emitted already when parsing the templates
> even when they won't be instantiated at all, other diagnostic is done during
> instantiation), or for e.g. references there are interesting cases where
> a reference to template parameter typename is used or where a reference to
> some time is tsubsted into a template parameter typename.
> E.g. you don't have any test coverage for the vector (num: ...)
> or gang (static: *, num: type_dep)
> or gang (static: type_dep1, type_dep2)
> (which would show you the above issue with the gang clause), or sufficient
> coverage for tile, etc.
> Of course that coverage can be added incrementally.

We'll add more tests incrementally.

Cesar

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

On 11/04/2015 02:24 AM, Jakub Jelinek wrote:
> Have you verified pt.c does the right thing when instantiating the
> OMP_CLAUSE_TILE clause (I mean primarily the TREE_VEC in there)?
> There really should be testcases for that.

Here's a patch which adds template support for the oacc clauses. Is it
ok for trunk?

Cesar
2015-11-04  Cesar Philippidis  

	gcc/cp/
	* pt.c (tsubst_omp_clauses): Add support for OMP_CLAUSE_{NUM_GANGS,
	NUM_WORKERS,VECTOR_LENGTH,GANG,WORKER,VECTOR,ASYNC,WAIT,TILE,AUTO,
	INDEPENDENT,SEQ}. 
	(tsubst_expr): Add support for OMP_CLAUSE_{KERNELS,PARALLEL,LOOP}.

	gcc/testsuite/
	* g++.dg/goacc/template-reduction.C: New test.
	* g++.dg/goacc/template.C: New test.

diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
index e3f55a7..4424596 100644
--- a/gcc/cp/pt.c
+++ b/gcc/cp/pt.c
@@ -14395,6 +14395,15 @@ tsubst_omp_clauses (tree clauses, bool declare_simd, bool allow_fields,
 	case OMP_CLAUSE_PRIORITY:
 	case OMP_CLAUSE_ORDERED:
 	case OMP_CLAUSE_HINT:
+	case OMP_CLAUSE_NUM_GANGS:
+	case OMP_CLAUSE_NUM_WORKERS:
+	case OMP_CLAUSE_VECTOR_LENGTH:
+	case OMP_CLAUSE_GANG:
+	case OMP_CLAUSE_WORKER:
+	case OMP_CLAUSE_VECTOR:
+	case OMP_CLAUSE_ASYNC:
+	case OMP_CLAUSE_WAIT:
+	case OMP_CLAUSE_TILE:
 	  OMP_CLAUSE_OPERAND (nc, 0)
 	= tsubst_expr (OMP_CLAUSE_OPERAND (oc, 0), args, complain, 
 			   in_decl, /*integral_constant_expression_p=*/false);
@@ -14449,6 +14458,9 @@ tsubst_omp_clauses (tree clauses, bool declare_simd, bool allow_fields,
 	case OMP_CLAUSE_THREADS:
 	case OMP_CLAUSE_SIMD:
 	case OMP_CLAUSE_DEFAULTMAP:
+	case OMP_CLAUSE_INDEPENDENT:
+	case OMP_CLAUSE_AUTO:
+	case OMP_CLAUSE_SEQ:
 	  break;
 	default:
 	  gcc_unreachable ();
@@ -15197,6 +15209,15 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
   }
   break;
 
+case OACC_KERNELS:
+case OACC_PARALLEL:
+  tmp = tsubst_omp_clauses (OMP_CLAUSES (t), false, false, args, complain,
+in_decl);
+  stmt = begin_omp_parallel ();
+  RECUR (OMP_BODY (t));
+  finish_omp_construct (TREE_CODE (t), stmt, tmp);
+  break;
+
 case OMP_PARALLEL:
   r = push_omp_privatization_clauses (OMP_PARALLEL_COMBINED (t));
   tmp = tsubst_omp_clauses (OMP_PARALLEL_CLAUSES (t), false, true,
@@ -15227,6 +15248,7 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
 case CILK_FOR:
 case OMP_DISTRIBUTE:
 case OMP_TASKLOOP:
+case OACC_LOOP:
   {
 	tree clauses, body, pre_body;
 	tree declv = NULL_TREE, initv = NULL_TREE, condv = NULL_TREE;
@@ -15235,7 +15257,8 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
 	int i;
 
 	r = push_omp_privatization_clauses (OMP_FOR_INIT (t) == NULL_TREE);
-	clauses = tsubst_omp_clauses (OMP_FOR_CLAUSES (t), false, true,
+	clauses = tsubst_omp_clauses (OMP_FOR_CLAUSES (t), false,
+  TREE_CODE (t) != OACC_LOOP,
   args, complain, in_decl);
 	if (OMP_FOR_INIT (t) != NULL_TREE)
 	  {
@@ -15305,9 +15328,11 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
   pop_omp_privatization_clauses (r);
   break;
 
+case OACC_DATA:
 case OMP_TARGET_DATA:
 case OMP_TARGET:
-  tmp = tsubst_omp_clauses (OMP_CLAUSES (t), false, true,
+  tmp = tsubst_omp_clauses (OMP_CLAUSES (t), false,
+TREE_CODE (t) != OACC_DATA,
 args, complain, in_decl);
   keep_next_level (true);
   stmt = begin_omp_structured_block ();
@@ -15331,6 +15356,16 @@ tsubst_expr (tree t, tree args, tsubst_flags_t complain, tree in_decl,
   add_stmt (t);
   break;
 
+case OACC_ENTER_DATA:
+case OACC_EXIT_DATA:
+case OACC_UPDATE:
+  tmp = tsubst_omp_clauses (OMP_STANDALONE_CLAUSES (t), false, false,
+args, complain, in_decl);
+  t = copy_node (t);
+  OMP_STANDALONE_CLAUSES (t) = tmp;
+  add_stmt (t);
+  break;
+
 case OMP_ORDERED:
   tmp = tsubst_omp_clauses (OMP_ORDERED_CLAUSES (t), false, true,
 args, complain, in_decl);
diff --git a/gcc/testsuite/g++.dg/goacc/template-reduction.C b/gcc/testsuite/g++.dg/goacc/template-reduction.C
new file mode 100644
index 000..668eeb3
--- /dev/null
+++ b/gcc/testsuite/g++.dg/goacc/template-reduction.C
@@ -0,0 +1,104 @@
+// This error is temporary.  Remove when support is added for these clauses
+// in the middle end.
+// { dg-prune-output "sorry, unimplemented" }
+
+extern void abort ();
+
+const int n = 100;
+
+// Check explicit template copy map
+
+template T
+sum (T array[])
+{
+   T s = 0;
+
+#pragma acc parallel loop num_gangs (10) gang reduction (+:s) copy (s, array[0:n])
+  for (int i = 0; i < n; i++)
+s += array[i];
+
+  return s;
+}
+
+// Check implicit template copy map
+
+template T
+sum ()
+{
+  T s = 0;
+  T array[n];
+
+  for (int i = 0; i < n; i++)
+array[i] = i+1;
+
+#pragma acc parallel loop num_gangs (10) gang reduction (+:s) copy (s)
+  for (int i = 0; i < n; i++)
+s += array[i];
+
+  return s;
+}
+
+// Check present and async

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Jakub Jelinek]

On Wed, Nov 04, 2015 at 08:58:32PM -0800, Cesar Philippidis wrote:
> diff --git a/gcc/cp/pt.c b/gcc/cp/pt.c
> index e3f55a7..4424596 100644
> --- a/gcc/cp/pt.c
> +++ b/gcc/cp/pt.c
> @@ -14395,6 +14395,15 @@ tsubst_omp_clauses (tree clauses, bool declare_simd, 
> bool allow_fields,
>   case OMP_CLAUSE_PRIORITY:
>   case OMP_CLAUSE_ORDERED:
>   case OMP_CLAUSE_HINT:
> + case OMP_CLAUSE_NUM_GANGS:
> + case OMP_CLAUSE_NUM_WORKERS:
> + case OMP_CLAUSE_VECTOR_LENGTH:
> + case OMP_CLAUSE_GANG:

GANG has two arguments, so you want to handle it differently, you need
to tsubst both arguments.

> + case OMP_CLAUSE_WORKER:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_ASYNC:
> + case OMP_CLAUSE_WAIT:
> + case OMP_CLAUSE_TILE:

I don't think tile will work well this way, if the only argument is a
TREE_VEC, then I think you hit:
case TREE_VEC:
  /* A vector of template arguments.  */
  gcc_assert (!type);
  return tsubst_template_args (t, args, complain, in_decl);
which does something very much different from making a copy of the TREE_VEC
and calling tsubst_expr on each argument.
Thus, either you need to handle it manually here, or think about different
representation of OMP_CLAUSE_TILE?  It seems you allow at most one tile
clause, so perhaps you could split the single source tile clause into one
tile clause per expression in there (the only issue is that the FEs
emit the clauses in last to first order, so you'd need to nreverse the
tile clause list before adding it to the list of all clauses).

Otherwise it looks ok, except:

> diff --git a/gcc/testsuite/g++.dg/goacc/template-reduction.C 
> b/gcc/testsuite/g++.dg/goacc/template-reduction.C
> new file mode 100644
> index 000..668eeb3
> --- /dev/null
> +++ b/gcc/testsuite/g++.dg/goacc/template-reduction.C
> +++ b/gcc/testsuite/g++.dg/goacc/template.C

the testsuite coverage is orders of magnitude smaller than it should be.
Just look at the amount of OpenMP template tests (both compile time and
runtime):
grep template libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep -l template 
libgomp/testsuite/libgomp.c++/*[^4] | wc -l; grep template 
gcc/testsuite/g++.dg/gomp/* | wc -l; grep -l template 
gcc/testsuite/g++.dg/gomp/* | wc -l
629 # templates
45 # tests with templates
151 # templates
58 # tests with templates
and even that is really not sufficient.  From my experience, special care is
needed in template tests to test both non-type dependent and type-dependent
cases (e.g. some diagnostics is emitted already when parsing the templates
even when they won't be instantiated at all, other diagnostic is done during
instantiation), or for e.g. references there are interesting cases where
a reference to template parameter typename is used or where a reference to
some time is tsubsted into a template parameter typename.
E.g. you don't have any test coverage for the vector (num: ...)
or gang (static: *, num: type_dep)
or gang (static: type_dep1, type_dep2)
(which would show you the above issue with the gang clause), or sufficient
coverage for tile, etc.
Of course that coverage can be added incrementally.

Jakub

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

On 11/04/2015 02:24 AM, Jakub Jelinek wrote:
> On Tue, Nov 03, 2015 at 02:16:59PM -0800, Cesar Philippidis wrote:

>> +
>> +  do
>> +{
>> +  if (c_parser_next_token_is (parser, CPP_MULT))
>> +{
>> +  c_parser_consume_token (parser);
>> +  expr = integer_minus_one_node;
>> +}
>> +  else
> 
> Is this right?  If it is either * or (assignment) expression, then
> I'd expect to parse only CPP_MULT followed by CPP_CLOSE_PAREN
> or CPP_COMMA that way (C parser has 2 tokens look-ahead, so it should be
> fine), so that
> tile (a + b + c, *)
> is parsed as
> (a + b + c); -1
> and so is
> tile (*, a + b)
> as
> -1; (a + b)
> while
> tile (*a, *b)
> is
> *a; *b.
> 
> Guess the gang clause parsing that went into the trunk already has the
> same bug,
> gang (static: *)
> or
> gang (static: *, num: 5)
> should be special, but
> gang (static: *ptr)
> should be
> gang (static: (*ptr))

Thanks for catching that. I'll fix the tile and shape scanners in both
front ends.

>> diff --git a/gcc/cp/semantics.c b/gcc/cp/semantics.c
>> index c73dcd0..14d006b 100644
>> --- a/gcc/cp/semantics.c
>> +++ b/gcc/cp/semantics.c
>> @@ -6704,9 +6704,51 @@ finish_omp_clauses (tree clauses, bool allow_fields, 
>> bool declare_simd)
>>  case OMP_CLAUSE_DEFAULTMAP:
>>  case OMP_CLAUSE__CILK_FOR_COUNT_:
>>  case OMP_CLAUSE_AUTO:
>> +case OMP_CLAUSE_INDEPENDENT:
>>  case OMP_CLAUSE_SEQ:
>>break;
>>  
>> +case OMP_CLAUSE_TILE:
>> +  {
>> +tree list = OMP_CLAUSE_TILE_LIST (c);
>> +
>> +while (list)
>> +  {
>> +t = TREE_VALUE (list);
>> +
>> +if (t == error_mark_node)
>> +  remove = true;
>> +else if (!type_dependent_expression_p (t)
>> + && !INTEGRAL_TYPE_P (TREE_TYPE (t)))
>> +  {
>> +error ("% value must be integral");
>> +remove = true;
>> +  }
>> +else
>> +  {
>> +t = mark_rvalue_use (t);
>> +if (!processing_template_decl)
>> +  {
>> +t = maybe_constant_value (t);
>> +if (TREE_CODE (t) == INTEGER_CST
>> +&& tree_int_cst_sgn (t) != 1
>> +&& t != integer_minus_one_node)
>> +  {
>> +warning_at (OMP_CLAUSE_LOCATION (c), 0,
>> +"% value must be positive");
>> +t = integer_one_node;
>> +  }
>> +  }
>> +t = fold_build_cleanup_point_expr (TREE_TYPE (t), t);
>> +  }
>> +
>> +/* Update list item.  */
>> +TREE_VALUE (list) = t;
>> +list = TREE_CHAIN (list);
>> +  }
>> +  }
>> +  break;
> 
> Have you verified pt.c does the right thing when instantiating the
> OMP_CLAUSE_TILE clause (I mean primarily the TREE_VEC in there)?
> There really should be testcases for that.

I don't think we have any support for templates in trunk yet. Should I
add it to this patch, or should I address that in a follow up patch?

By the way, template support for num_gangs, num_worker and vector_length
depend on this patch
 because the
c++ front end is currently incorrectly trying to do type checking as it
scans those clauses.

>> diff --git a/gcc/gimplify.c b/gcc/gimplify.c
>> index 03203c0..08b192d 100644
>> --- a/gcc/gimplify.c
>> +++ b/gcc/gimplify.c
>> @@ -6997,7 +6997,6 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
>> *pre_p,
>>  
>>  case OMP_CLAUSE_DEVICE_RESIDENT:
>>  case OMP_CLAUSE_USE_DEVICE:
>> -case OMP_CLAUSE_INDEPENDENT:
>>remove = true;
>>break;
>>  
>> @@ -7007,6 +7006,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
>> *pre_p,
>>  case OMP_CLAUSE_COLLAPSE:
>>  case OMP_CLAUSE_AUTO:
>>  case OMP_CLAUSE_SEQ:
>> +case OMP_CLAUSE_INDEPENDENT:
>>  case OMP_CLAUSE_MERGEABLE:
>>  case OMP_CLAUSE_PROC_BIND:
>>  case OMP_CLAUSE_SAFELEN:
>> @@ -7014,6 +7014,7 @@ gimplify_scan_omp_clauses (tree *list_p, gimple_seq 
>> *pre_p,
>>  case OMP_CLAUSE_NOGROUP:
>>  case OMP_CLAUSE_THREADS:
>>  case OMP_CLAUSE_SIMD:
>> +case OMP_CLAUSE_TILE:
>>break;
> 
> No gimplification of the expressions in the tile clause?

Ultimately we're just ignoring it, but for completeness' sake I fixed this.

I'm still testing this patch, but something like this OK for trunk?

cesar
2015-11-04  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  

	gcc/
	* gimplify.c (gimplify_scan_omp_clauses): Add support for
	OMP_CLAUSE_TILE.  Update handling of OMP_CLAUSE_INDEPENDENT.
	(gimplify_adjust_omp_clauses): Likewise.
	* omp-low.c (scan_sharing_clauses): Add support for 

Re: [openacc] tile, independent, default, private and firstprivate support in c/++

[Jakub Jelinek]

On Tue, Nov 03, 2015 at 02:16:59PM -0800, Cesar Philippidis wrote:
> --- a/gcc/c-family/c-omp.c
> +++ b/gcc/c-family/c-omp.c
> @@ -709,12 +709,21 @@ c_oacc_split_loop_clauses (tree clauses, tree 
> *not_loop_clauses)
>  
>switch (OMP_CLAUSE_CODE (clauses))
>  {
> +   /* Loop clauses.  */
>   case OMP_CLAUSE_COLLAPSE:
> - case OMP_CLAUSE_REDUCTION:
> + case OMP_CLAUSE_TILE:
> + case OMP_CLAUSE_GANG:
> + case OMP_CLAUSE_WORKER:
> + case OMP_CLAUSE_VECTOR:
> + case OMP_CLAUSE_AUTO:
> + case OMP_CLAUSE_SEQ:
> + case OMP_CLAUSE_INDEPENDENT:
> OMP_CLAUSE_CHAIN (clauses) = loop_clauses;
> loop_clauses = clauses;
> break;
>  
> +   /* Parallel/kernels clauses.  */
> +

Why the extra empty line where you don't have it above COLLAPSE?
>   default:
> OMP_CLAUSE_CHAIN (clauses) = *not_loop_clauses;
> *not_loop_clauses = clauses;

> @@ -10577,7 +10584,10 @@ c_parser_omp_clause_default (c_parser *parser, tree 
> list)
>else
>  {
>  invalid_kind:
> -  c_parser_error (parser, "expected % or %");
> +  if (is_oacc)
> + c_parser_error (parser, "expected %");
> + else
> +   c_parser_error (parser, "expected % or %");

The indentation is wrong above (last two lines), doesn't 
-Wmisleading-indentation warn about
this?
>  }
>c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, "expected %<)%>");
>  
> @@ -11376,6 +11386,83 @@ c_parser_oacc_clause_async (c_parser *parser, tree 
> list)
>return list;
>  }
>  
> +/* OpenACC 2.0:
> +   tile ( size-expr-list ) */
> +
> +static tree
> +c_parser_oacc_clause_tile (c_parser *parser, tree list)
> +{
> +  tree c, expr = error_mark_node;
> +  location_t loc, expr_loc;
> +  tree tile = NULL_TREE;
> +  vec *tvec = make_tree_vector ();
> +
> +  check_no_duplicate_clause (list, OMP_CLAUSE_TILE, "tile");
> +
> +  loc = c_parser_peek_token (parser)->location;
> +  if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> +{
> +  release_tree_vector (tvec);
> +  return list;
> +}

Better move tvec = make_tree_vector (); below this if and remove
release_tree_vector call?

> +
> +  do
> +{
> +  if (c_parser_next_token_is (parser, CPP_MULT))
> + {
> +   c_parser_consume_token (parser);
> +   expr = integer_minus_one_node;
> + }
> +  else

Is this right?  If it is either * or (assignment) expression, then
I'd expect to parse only CPP_MULT followed by CPP_CLOSE_PAREN
or CPP_COMMA that way (C parser has 2 tokens look-ahead, so it should be
fine), so that
tile (a + b + c, *)
is parsed as
(a + b + c); -1
and so is
tile (*, a + b)
as
-1; (a + b)
while
tile (*a, *b)
is
*a; *b.

Guess the gang clause parsing that went into the trunk already has the
same bug,
gang (static: *)
or
gang (static: *, num: 5)
should be special, but
gang (static: *ptr)
should be
gang (static: (*ptr))

> + {
> +   expr_loc = c_parser_peek_token (parser)->location;
> +   expr = c_parser_expr_no_commas (parser, NULL).value;
> +
> +   if (expr == error_mark_node)
> + goto cleanup_error;
> +
> +   if (!INTEGRAL_TYPE_P (TREE_TYPE (expr)))
> + {
> +   c_parser_error (parser, "% value must be integral");
> +   return list;
> + }
> +
> +   mark_exp_read (expr);
> +   expr = c_fully_fold (expr, false, NULL);
> +
> +   /* Attempt to statically determine when expr isn't positive.  */
> +   c = fold_build2_loc (expr_loc, LE_EXPR, boolean_type_node, expr,
> +build_int_cst (TREE_TYPE (expr), 0));
> +   protected_set_expr_location (c, expr_loc);
> +   if (c == boolean_true_node)
> + {
> +   warning_at (expr_loc, 0,"% value must be positive");
> +   expr = integer_one_node;
> + }
> + }
> +
> +  vec_safe_push (tvec, expr);
> +  if (c_parser_next_token_is (parser, CPP_COMMA))
> + c_parser_consume_token (parser);
> +}
> +  while (c_parser_next_token_is_not (parser, CPP_CLOSE_PAREN));
> +
> +  /* Consume the trailing ')'.  */
> +  c_parser_consume_token (parser);
> +
> +  c = build_omp_clause (loc, OMP_CLAUSE_TILE);
> +  tile = build_tree_list_vec (tvec);
> +  OMP_CLAUSE_TILE_LIST (c) = tile;
> +  OMP_CLAUSE_CHAIN (c) = list;
> +  release_tree_vector (tvec);
> +  return c;
> +
> + cleanup_error:
> +  c_parser_skip_until_found (parser, CPP_CLOSE_PAREN, "expected %<)%>");
> +  release_tree_vector (tvec);
> +  return list;
> +}
> +
>  /* OpenACC:
> wait ( int-expr-list ) */
>  
> @@ -12576,6 +12663,10 @@ c_parser_oacc_all_clauses (c_parser *parser, 
> omp_clause_mask mask,
> clauses = c_parser_oacc_data_clause (parser, c_kind, clauses);
> c_name = "delete";
> break;
> + case PRAGMA_OMP_CLAUSE_DEFAULT:
> +   clauses = c_parser_omp_clause_default (parser, clauses, true);
> +   c_name = 

[openacc] tile, independent, default, private and firstprivate support in c/++

[Cesar Philippidis]

This patch does the following to the c and c++ front ends:

 * parsing support for the tile, independent, default (none),
   private and firstprivate clauses in c and c++

 * updates c_oacc_split_loop_clauses to filter out the loop clauses
   from combined parallel/kernels loops

The c front end already had some support for private and firstprivate in
openacc. However, the c++ front end wasn't associating any of those
clauses with parallel, kernels or acc loops.

For reference, here's the grammar for the tile clause from section 2.7
in version 2.0a of the openacc spec:

tile( size-expr-list )

  where size-expr is one of:

*
int-expr

That '*' symbol complicated the parsing a little, since it's no longer a
primary expression.

I've bootstrapped and regression tested this on x86_64. Is this ok for
trunk?

Cesar

2015-11-03  Cesar Philippidis  
	Thomas Schwinge  
	James Norris  

	gcc/
	* gimplify.c (gimplify_scan_omp_clauses): Add support for
	OMP_CLAUSE_TILE.  Update handling of OMP_CLAUSE_INDEPENDENT.
	(gimplify_adjust_omp_clauses): Likewise.
	* omp-low.c (scan_sharing_clauses): Add support for OMP_CLAUSE_TILE.
	* tree-core.h (enum omp_clause_code): Add OMP_CLAUSE_TILE.
	* tree-pretty-print.c (dump_omp_clause): Handle OMP_CLAUSE_TILE.
	* tree.c (omp_clause_num_ops): Add an entry for OMP_CLAUSE_TILE.
	(omp_clause_code_name): Likewise.
	(walk_tree_1): Handle OMP_CLAUSE_TILE.
	* tree.h (OMP_TILE_LIST): New macro.

	gcc/c-family/
	* c-omp.c (c_oacc_split_loop_clauses): Make TILE, GANG, WORKER, VECTOR,
	AUTO, SEQ and independent as loop clauses.  Associate REDUCTION
	clauses with parallel and kernels.
	* c-pragma.h (enum pragma_omp_clause): Add entries for
	PRAGMA_OACC_CLAUSE_{INDEPENDENT,TILE,DEFAULT}.

	gcc/c/
	* c-parser.c (c_parser_omp_clause_name): Add support for
	PRAGMA_OACC_CLAUSE_INDEPENDENT and PRAGMA_OACC_CLAUSE_TILE.
	(c_parser_omp_clause_default): Add is_oacc argument. Handle
	default(none) in OpenACC.
	(c_parser_oacc_clause_tile): New function.
	(c_parser_oacc_all_clauses): Add support for OMP_CLAUSE_DEFAULT,
	OMP_CLAUSE_INDEPENDENT and OMP_CLAUSE_TILE.
	(OACC_LOOP_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{PRIVATE,INDEPENDENT,
	TILE}.
	(OACC_KERNELS_MASK): Add PRAGMA_OACC_CLAUSE_DEFAULT.
	(OACC_PARALLEL_MASK): Add PRAGMA_OACC_CLAUSE_{DEFAULT,PRIVATE,
	FIRSTPRIVATE}.
	(c_parser_omp_all_clauses): Update call to c_parser_omp_clause_default.
	* c-typeck.c (c_finish_omp_clauses): Add support for OMP_CLAUSE_TILE
	and OMP_CLAUSE_INDEPENDENT.

	gcc/cp/
	* parser.c (cp_parser_omp_clause_name): Add support for
	PRAGMA_OACC_CLAUSE_INDEPENDENT and PRAGMA_OACC_CLAUSE_TILE.
	(cp_parser_oacc_clause_tile): New function.
	(cp_parser_omp_clause_default): Add is_oacc argument. Handle
	default(none) in OpenACC.
	(cp_parser_oacc_all_clauses): Add support for
	(cp_parser_omp_all_clauses): Update call to
	cp_parser_omp_clause_default.
	PRAGMA_OACC_CLAUSE_{DEFAULT,INDEPENDENT,TILE,PRIVATE,FIRSTPRIVATE}.
	(OACC_LOOP_CLAUSE_MASK): Add PRAGMA_OACC_CLAUSE_{PRIVATE,INDEPENDENT,
	TILE}.
	(OACC_KERNELS_MASK): Add PRAGMA_OACC_CLAUSE_DEFAULT.
	(OACC_PARALLEL_MASK): Add PRAGMA_OACC_CLAUSE_{DEFAULT,PRIVATE,
	FIRSTPRIVATE}.
	* semantics.c (finish_omp_clauses): Add support for
	OMP_CLAUSE_INDEPENDENT and OMP_CLAUSE_TILE.

	gcc/testsuite/
	* c-c++-common/goacc/combined-directives.c: New test.
	* c-c++-common/goacc/loop-clauses.c: New test.
	* c-c++-common/goacc/tile.c: New test.

diff --git a/gcc/c-family/c-omp.c b/gcc/c-family/c-omp.c
index 133d079..a04caf3 100644
--- a/gcc/c-family/c-omp.c
+++ b/gcc/c-family/c-omp.c
@@ -709,12 +709,21 @@ c_oacc_split_loop_clauses (tree clauses, tree *not_loop_clauses)
 
   switch (OMP_CLAUSE_CODE (clauses))
 {
+	  /* Loop clauses.  */
 	case OMP_CLAUSE_COLLAPSE:
-	case OMP_CLAUSE_REDUCTION:
+	case OMP_CLAUSE_TILE:
+	case OMP_CLAUSE_GANG:
+	case OMP_CLAUSE_WORKER:
+	case OMP_CLAUSE_VECTOR:
+	case OMP_CLAUSE_AUTO:
+	case OMP_CLAUSE_SEQ:
+	case OMP_CLAUSE_INDEPENDENT:
 	  OMP_CLAUSE_CHAIN (clauses) = loop_clauses;
 	  loop_clauses = clauses;
 	  break;
 
+	  /* Parallel/kernels clauses.  */
+
 	default:
 	  OMP_CLAUSE_CHAIN (clauses) = *not_loop_clauses;
 	  *not_loop_clauses = clauses;
diff --git a/gcc/c-family/c-pragma.h b/gcc/c-family/c-pragma.h
index 69e7392..953c4e3 100644
--- a/gcc/c-family/c-pragma.h
+++ b/gcc/c-family/c-pragma.h
@@ -153,6 +153,7 @@ enum pragma_omp_clause {
   PRAGMA_OACC_CLAUSE_DEVICEPTR,
   PRAGMA_OACC_CLAUSE_GANG,
   PRAGMA_OACC_CLAUSE_HOST,
+  PRAGMA_OACC_CLAUSE_INDEPENDENT,
   PRAGMA_OACC_CLAUSE_NUM_GANGS,
   PRAGMA_OACC_CLAUSE_NUM_WORKERS,
   PRAGMA_OACC_CLAUSE_PRESENT,
@@ -162,6 +163,7 @@ enum pragma_omp_clause {
   PRAGMA_OACC_CLAUSE_PRESENT_OR_CREATE,
   PRAGMA_OACC_CLAUSE_SELF,
   PRAGMA_OACC_CLAUSE_SEQ,
+  PRAGMA_OACC_CLAUSE_TILE,
   PRAGMA_OACC_CLAUSE_VECTOR,
   PRAGMA_OACC_CLAUSE_VECTOR_LENGTH,
   PRAGMA_OACC_CLAUSE_WAIT,
@@ -169,6 +171,7 @@ enum