Re: [PATCH 1/6] [GOMP4] OpenACC 1.0+ support in fortran front-end

[Tobias Burnus <burnus at net-b dot de>]

Ilmir Usmanov wrote:
>     OpenACC 1.0 support to fortran FE -- core.
> --- a/gcc/fortran/dump-parse-tree.c
> +++ b/gcc/fortran/dump-parse-tree.c
> @@ -1031,9 +1031,22 @@ show_omp_node (int level, gfc_code *c)
>   {
>     gfc_omp_clauses *omp_clauses = NULL;
>     const char *name = NULL;
> +  bool is_oacc = false;

I'd also update the comment before the function and mention OpenACC 
there. It currently reads:


/* Show a single OpenMP directive node and everything underneath it
   if necessary.  */


> +  fprintf (dumpfile, "!$%s %s", is_oacc?"ACC":"OMP", name);

Add spaces around "?" and ":"

> @@ -1215,7 +1334,7 @@ show_omp_node (int level, gfc_code *c)
> -  fprintf (dumpfile, "!$OMP END %s", name);
> +  fprintf (dumpfile, "!$%s END %s", is_oacc?"ACC":"OMP", name);

Ditto.

> --- a/gcc/fortran/gfortran.h
> +++ b/gcc/fortran/gfortran.h
>
> +/* Likewise to gfc_namelist, but contains expressions.  */
> +typedef struct gfc_exprlist
> +{
> +  struct gfc_expr *expr;
> +  struct gfc_exprlist *next;
> +}
> +gfc_exprlist;

I don't feel strong about it, but I think it is more GCC / gfortran 
style to use "gfc_expr_list" instead of "gfc_exprlist".

> +  /* OpenACC. */
> +  struct gfc_expr *async_expr;
> +  struct gfc_expr *gang_expr;
> +  struct gfc_expr *worker_expr;
> +  struct gfc_expr *vector_expr;
> +  struct gfc_expr *num_gangs_expr;
> +  struct gfc_expr *num_workers_expr;
> +  struct gfc_expr *vector_length_expr;
> +  struct gfc_expr *non_clause_wait_expr;
> +  gfc_exprlist *waitlist;
> +  gfc_exprlist *tilelist;
> +  bool async, gang, worker, vector, seq, independent;
> +  bool wait, par_auto, gang_static;

I wonder whether it would make sense to use bit fields here.

> --- a/gcc/fortran/match.c
> +++ b/gcc/fortran/match.c
> @@ -2595,6 +2595,33 @@ match_exit_cycle (gfc_statement st, gfc_exec_op op)
>     if (cnt > 0
>         && o != NULL
>         && o->state == COMP_OMP_STRUCTURED_BLOCK
> +      && (o->head->op == EXEC_OACC_LOOP
> +          || o->head->op == EXEC_OACC_PARALLEL_LOOP))
> +    {
> +      int collapse = 1;
> +      gcc_assert (o->head->next != NULL
> +                  && (o->head->next->op == EXEC_DO
> +                      || o->head->next->op == EXEC_DO_WHILE)

Two questions:

a) Does this part work well when both -fopenmp and -fopenacc is used? I 
mean: "!$acc loop" followed/preceded by "!$omp do"? I could imagine that 
there could be clashes, especially when - e.g. - collapse doesn't match.

b) Do you also handle DO CONCURRENT - either by rejecting it or by 
accepting it? Namely,

!$acc loop
do concurrent(i=1:5)
end do
!$acc end loop
end

Side remark, with -fopenmp, it does ICE: 
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60127


Talking about "!$acc loop": I vaguely remember that OpenACC 1.0's spec 
doesn't have "!$acc end loop" while I have seen OpenACC programs which 
use it. How do you handle "!$acc end loop"?

Looking at parse.c, it seems to simply error out. I wonder whether one 
should be a bit more graceful. For instance, the following examples use 
"!$acc end loop": 
http://devblogs.nvidia.com/parallelforall/openacc-example-part-2/ [If I 
remember correctly, pgf95 and Cray ftn silently accepts "end loop" while 
pathf95 accepts it with a warning.]

And looking at the spec of OpenACC 1.0 and 2.0a, the "end loop" seems to 
be invalid. How about following PathScale's ENZO and accepting "end 
loop" with a warning? Or at least error out with a good error message.


> +      if (st == ST_EXIT && cnt <= collapse)
> +        {
> +          gfc_error ("EXIT statement at %C terminating !$ACC LOOP loop");
> +          return MATCH_ERROR;
> +        }
> +      if (st == ST_CYCLE && cnt < collapse)
> +        {
> +          gfc_error ("CYCLE statement at %C to non-innermost collapsed"
> +                     " !$ACC LOOP loop");
> +          return MATCH_ERROR;
> +        }
> +    }

I wonder whether one should include for OpenMP and OpenACC some 
additional checks as done for DO CONCURRENT or whether those aren't 
required. I think image controll statements like Fortran 2008's CRITICAL 
block might cause problems with OpenACC/OpenMP concurrency. (Given that 
OpenACC/OpenMP likely ignores Fortran 2008's coarrays, one might defer 
this until OpenACC/OpenMP has been updated for Fortran 2008.)


> +  if (gfc_pure (NULL))
> +    {
> +      gfc_error_now ("OpenACC directives at %C may not appear in PURE "
> +                     "or ELEMENTAL procedures");

Using gfc_pure() you do not check for ELEMENTAL: Since Fortran 2008, 
there are also IMPURE ELEMENTAL procedures. I don't know the spec, but I 
don't really see a reason why OpenACC shouldn't be permitted in IMPURE 
ELEMENTAL procedures. (BTW: "ELEMENTAL" implies PURE unless an explicit 
IMPURE is used.)

In any case, either drop "or ELEMENTAL" or also check for the elemental 
attribute.

> +  if (gfc_implicit_pure (NULL))
> +    gfc_current_ns->proc_name->attr.implicit_pure = 0;

I believe that will fail with BLOCK - cf. gfc_implicit_pure()

real function foo(n)
  integer, value :: n
  BLOCK
     integer i
     real sum
     !$acc loop reduce(+:sum)
     do i=1, n
        sum += sin(real(i))
     end do
  END BLOCK
end


> +  /* All else has failed, so give up.  See if any of the matchers has
> +       stored an error message of some sort.  */

Indenting of the second line looks wrong. And "all else" sounds language 
wise strange to me.


> +static void
> +verify_token_free (const char* token, int length, bool last_was_use_stmt)
> +{

This function should have a comment describing what it does.

> +static bool
> +verify_token_fixed (const char *token, int length, bool last_was_use_stmt)

Also here, a short comment line before the function would be helpful.

> --- a/gcc/fortran/scanner.c
> +++ b/gcc/fortran/scanner.c
> +static int openmp_flag, openacc_flag; /* If !$omp/!&acc occurred in current comment line */

Is this really !&acc and not !$acc?

> +            else if (gfc_option.gfc_flag_openmp&& !gfc_option.gfc_flag_openacc)

Space before "&&".

> +	  /* If -fopenmp/-fopenacc, we need to handle here 2 things:
> +	     1) don't treat !$omp/!$acc|c$omp/c$acc|*$omp / *$acc as comments,

Is there a reason for the spaces around the last "/"?

> +              gcc_assert(gfc_wide_tolower (c) == (unsigned char ) "!$acc"[i]);

Spurious " " before ")".


Tobias