[WIP,PR,fortran/72741] Rework Fortran OpenACC routine clause handling (was: [PATCH] OpenACC routines in fortran modules)

Hi!

This is still hacky and WIP; posting for Cesar and Tobias to have a look.
I'm still not too much of a Fortran person.  ;-)

On Fri, 1 Jul 2016 13:40:58 -0700, Cesar Philippidis <cesar@codesourcery.com> wrote:
> It turns out that the acc routine parallelism isn't being recorded in
> fortran .mod files. This is a problem because then the ME can't validate
> if a routine has compatible parallelism with the call site. This patch
> does two things:
> 
>  1. Encode gang, worker, vector and seq level parallelism in module
>     files. This introduces a new oacc_function enum, which I ended
>     up using to record the parallelism of standalone acc routines too.

Building on top of this patch, and on top of
<https://gcc.gnu.org/ml/gcc-patches/2016-07/msg01910.html> "[gomp4] Fix
PR72741", I reworked these patches (effectively reverting a lot of
Cesar's earlier changes, which nevertheless gave good guidance to me,
about which code I needed to touch).  With this patch, we now handle more
Fortran OpenACC routine directive use/misuse (see the test case changes),
much in spirit of what I discussed in <http://gcc.gnu.org/PR72741>
"Fortran OpenACC routine directive doesn't properly handle clauses
specifying the level of parallelism", minus items that Cesar already
clarified for me, where Fortran is different from what I expected,
different from the C/C++ environment I'm more used to.  This now also
paves the way for adding Fortran support to my recent patch
<https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00069.html> "Use
verify_oacc_routine_clauses", and then ultimately
<https://gcc.gnu.org/ml/gcc-patches/2016-08/msg00071.html> "Repeated use
of the OpenACC routine directive".

However, my changes are still hacky and WIP, still contains a bunch of
TODOs.  Can you, Cesar and/or Tobias, please advise on these?

>  2. Extends gfc_match_oacc_routine to add acc routine directive support
>     for intrinsic procedures such as abort.
> 
> Is this patch OK for trunk? I included support for intrinsic procedures
> because it was necessary with my previous patch which treated all calls
> to non-acc routines from within an OpenACC offloaded region as errors.
> Now that it has been determined that those patches should be link time
> errors, we technically don't need to add acc routine support for
> intrinsic procedures. So I can drop that part of the patch if necessary.

That could've been a patch separate from the others, as it's doing a
separate thing.  We will want to handle intrinsics used with the OpenACC
routine directive with a name (but it certainly isn't a priority).  I
left in these changes, and also extended them a bit.

First some comments on Cesar's patch:

> --- a/gcc/fortran/module.c
> +++ b/gcc/fortran/module.c

>  [...]
> +DECL_MIO_NAME (oacc_function)
>  [...]

As discussed between Cesar and Tobias, these module.c/symbol.c changes
introduce an incompatibility in the Fortran module file format, which
we'll want to avoid.  Reverting that to use individual bit flags instead
of the "enum oacc_function", I think that we're safe (but I have not
verified that).  On the other hand, given that I'm not at all handling in
module.c/symbol.c the new "locus omp_clauses_locus" and "struct
symbol_attribute *next" members that I'm adding to "symbol_attribute",
I'm not sure whether I'm actually testing this properly.  ;-) I guess I'm
not.

> --- a/gcc/fortran/openmp.c
> +++ b/gcc/fortran/openmp.c

> @@ -1814,7 +1824,10 @@ gfc_match_oacc_routine (void)
>  	  != MATCH_YES))
>      return MATCH_ERROR;
>  
> -  if (sym != NULL)
> +  if (isym != NULL)
> +    /* There is nothing to do for intrinsic procedures.  */
> +    ;

We will want to check that no incompatible clauses are being specified,
for example (but, low priority).  I'm adding a hacky implementation of
that.

> +  else if (sym != NULL)
>      {
>        n = gfc_get_oacc_routine_name ();
>        n->sym = sym;

> --- a/gcc/fortran/trans-decl.c
> +++ b/gcc/fortran/trans-decl.c
> @@ -1327,11 +1327,26 @@ add_attributes_to_decl (symbol_attribute sym_attr, tree list)
>      list = tree_cons (get_identifier ("omp declare target"),
>  		      NULL_TREE, list);
>  
> -  if (sym_attr.oacc_function)
> +  if (sym_attr.oacc_function != OACC_FUNCTION_NONE)
>      {
>        tree dims = NULL_TREE;
>        int ix;
> -      int level = sym_attr.oacc_function - 1;
> +      int level = GOMP_DIM_MAX;
> +
> +      switch (sym_attr.oacc_function)
> +	{
> +	case OACC_FUNCTION_GANG:
> +	  level = GOMP_DIM_GANG;
> +	  break;
> +	case OACC_FUNCTION_WORKER:
> +	  level = GOMP_DIM_WORKER;
> +	  break;
> +	case OACC_FUNCTION_VECTOR:
> +	  level = GOMP_DIM_VECTOR;
> +	  break;
> +	case OACC_FUNCTION_SEQ:
> +	default:;
> +	}
>  
>        for (ix = GOMP_DIM_MAX; ix--;)
>  	dims = tree_cons (build_int_cst (boolean_type_node, ix >= level),

As discussed before, this should use the generic omp-low.c functions,
which I've implemented.

> --- /dev/null
> +++ b/gcc/testsuite/gfortran.dg/goacc/routine-7.f90
> @@ -0,0 +1,69 @@
> +! Test acc routines inside modules.
> +
> +! { dg-additional-options "-O0" }

-O0 to prevent inlining of functions tagged with OpenACC routine
directives, or another reason?

> --- a/libgomp/testsuite/libgomp.oacc-fortran/routine-7.f90
> +++ b/libgomp/testsuite/libgomp.oacc-fortran/routine-7.f90
> @@ -1,121 +1,95 @@
> +! Test acc routines inside modules.
>  
>  ! { dg-do run }
> -! { dg-additional-options "-cpp" }
>  
> -#define M 8
> -#define N 32
> +module routines
> +  integer, parameter :: N = 32
>  
> -program main
> -  integer :: i
> -  integer :: a(N)
> -  integer :: b(M * N)
> -
> -  do i = 1, N
> -    a(i) = 0
> -  end do
> +contains
> +  subroutine vector (a)
> +    implicit none
> +    !$acc routine vector
> +    integer, intent (inout) :: a(N)
> +    integer :: i
> [...]

This seems to completely rewrite the test case.  Is that intentional, or
should the original test case be preserved, and the changed/new/rewritten
one be added as a new test case?

Now, my hacky WIP patch.

One big chunk of the gcc/fortran/gfortran.h changes is just to move some
stuff around, without any changes, so that I can use "locus" in
"symbol_attribute".

I very much "cargo cult"ed all that "oacc_routine*" bit flag stuff in
module.c/symbol.c, replicating what's being done for "omp target
declare", without really knowing what I'm doing there.  I will appreciate
test cases actually exercising this code -- which doesn't currently at
all handle the new "locus omp_clauses_locus" and "struct symbol_attribute
*next" members that I'm adding to "symbol_attribute", as I've mentioned
before.  (But I suppose it should?)

We're not implementing the OpenACC device_type clause at the moment, so
the "TODO: handle device_type clauses" comment in
gcc/fortran/openmp.c:gfc_match_oacc_routine is not a concern right now.

With these changes, we're now actually also paying attention the clauses
specified with the OpenACC routine directive with a name -- one of the
things mentioned as missing in <http://gcc.gnu.org/PR72741> "Fortran
OpenACC routine directive doesn't properly handle clauses specifying the
level of parallelism".

To handle several "pending" OpenACC routine directives, I had to add the
"struct symbol_attribute *next" member to "symbol_attribute" -- I hope
that doesn't disqualify the proposed changes as too ugly.  (Several other
structs already contain such "next" pointers, and the use is very much
confined to only the OpenACC routine directive.)  I will of course be
happy to learn about a better/different way to do this.

commit ca4a098dab72f27c6e1121aa7e5e49764921974e
Author: Thomas Schwinge <thomas@codesourcery.com>
Date:   Thu Aug 11 16:34:22 2016 +0200

    [WIP] [PR fortran/72741] Rework Fortran OpenACC routine clause handling
---
 gcc/fortran/gfortran.h                             | 275 +++++++++++----------
 gcc/fortran/module.c                               |  34 ++-
 gcc/fortran/openmp.c                               | 106 ++++----
 gcc/fortran/symbol.c                               | 135 +++++++++-
 gcc/fortran/trans-decl.c                           | 106 ++++++--
 .../gfortran.dg/goacc/oaccdevlow-routine.f95       |   2 +-
 gcc/testsuite/gfortran.dg/goacc/pr72741-2.f        |  39 +++
 .../gfortran.dg/goacc/pr72741-intrinsic-1.f        |  16 ++
 gcc/testsuite/gfortran.dg/goacc/pr72741.f90        |  14 +-
 9 files changed, 501 insertions(+), 226 deletions(-)

Grüße
 Thomas

[WIP,PR,fortran/72741] Rework Fortran OpenACC routine clause handling (was: [PATCH] OpenACC routines in fortran modules)

Commit Message

Comments

Patch