Re: [patch] OpenACC fortran front end

[Thomas Schwinge <thomas at codesourcery dot com>]

Hi!

On Thu, 13 Nov 2014 17:44:40 -0800, Cesar Philippidis <cesar@codesourcery.com> wrote:
> On 11/13/2014 08:43 AM, Jakub Jelinek wrote:
> > Can you please avoid the TODOs in the source?  If it is not the right
> > thing, either do something better, or file a PR to schedule such work for
> > the future.

Should we use the existing openmp keyword for this,
<https://gcc.gnu.org/bugzilla/describekeywords.cgi>, or get a new openacc
keyword added?


> Using -fopenmp with -fopenacc is broken. See
> 
> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=63858

Another issue: I noticed that when compiling fixed-form code such as
libgomp/testsuite/libgomp.oacc-fortran/data-already-2.f with both
-fopenacc -fopenmp, the data constructs "disappear" (see
-ftree-dump-original, for example).

We have a (tiny, addmittedly) little bit of testing for C/C++,
gcc/testsuite/*/*goacc-gomp, but should also to Fortran add a goacc-gomp
testsuite.


> 	gcc/fortran/
> 	* f95-lang.c (DEF_GOACC_BUILTIN_COMPILER): Remove bogus TODO.

> --- a/gcc/fortran/f95-lang.c
> +++ b/gcc/fortran/f95-lang.c
> @@ -1189,7 +1189,6 @@ gfc_init_builtin_functions (void)
>        gfc_define_builtin ("__builtin_" name, builtin_types[type], \
>  			  code, name, attr);
>  #undef DEF_GOACC_BUILTIN_COMPILER
> -      /* TODO: this is not doing the right thing.  */
>  #define DEF_GOACC_BUILTIN_COMPILER(code, name, type, attr) \
>        gfc_define_builtin (name, builtin_types[type], code, name, attr);
>  #include "../oacc-builtins.def"

(OK to not include the TODO marker in the trunk submission, but) I don't
think it's "bogus", but really doesn't work, as discussed before,
<http://news.gmane.org/find-root.php?message_id=%3C87k350n6zl.fsf%40schwinge.name%3E>.


> --- a/gcc/fortran/parse.c
> +++ b/gcc/fortran/parse.c
> @@ -3818,7 +3818,9 @@ parse_critical_block (void)
>  
>    for (sd = gfc_state_stack; sd; sd = sd->previous) 
>      if (sd->state == COMP_OMP_STRUCTURED_BLOCK)
> -      gfc_error_now ("CRITICAL block inside of OpenMP or OpenACC region at %C");
> +      gfc_error_now (is_oacc (sd)
> +		     ? "CRITICAL block inside of OpenACC region at %C"
> +		     : "CRITICAL block inside of OpenMP region at %C");

Can't use %s and Âis_oacc () ? "OpenACC" : "OpenMP"Â here, too?

> +/* Return true if this state data represents an OpenACC region.  */
> +bool
> +is_oacc (gfc_state_data *sd)
> +{
> +  switch (sd->construct->op)
> +    {
> +    case EXEC_OACC_PARALLEL_LOOP:break;

Misplaced break statement?  (Doesn't that result in a compiler warning,
"non-void function returns", or similar?)

> +    case EXEC_OACC_PARALLEL:
> +    case EXEC_OACC_KERNELS_LOOP:
> +    case EXEC_OACC_KERNELS:
> +    case EXEC_OACC_DATA:
> +    case EXEC_OACC_HOST_DATA:
> +    case EXEC_OACC_LOOP:
> +    case EXEC_OACC_UPDATE:
> +    case EXEC_OACC_WAIT:
> +    case EXEC_OACC_CACHE:
> +    case EXEC_OACC_ENTER_DATA:
> +    case EXEC_OACC_EXIT_DATA:
> +      return true;
> +
> +    default:
> +      return false;
> +    }
> +}


> --- a/gcc/testsuite/gfortran.dg/gomp/omp_do1.f90
> +++ b/gcc/testsuite/gfortran.dg/gomp/omp_do1.f90
> @@ -44,7 +44,7 @@ outer: do i = 1, 30
>    end do outer
>  last: do i = 1, 30
>  !$omp parallel
> -    if (i .eq. 21) exit last ! { dg-error "leaving OpenMP or OpenACC structured block" }
> +    if (i .eq. 21) exit last ! { dg-error "leaving OpenMP structured block" }
>  !$omp end parallel
>    end do last
>  !$omp parallel do shared (i)

Does that suggest that we have no corresponding testing for OpenACC?


A few more issues that I noted (but this is still not a comprehensive
code review).  Not knowing which of those you'd like to address right
now, I have not yet filed any PRs.


gcc/fortran/dump-parse-tree.c:show_omp_namelist only handles the OpenMP
gcc/fortran/gfortran.h:gfc_omp_map_op OMP_MAP_* but not those that we're
adding for the OpenACC interfaces.


Doesn't gcc/fortran/frontend-passes:gfc_code_walker need to be updated
for OpenACC?


I still think (as discussed before, at the end of
<http://news.gmane.org/find-root.php?message_id=%3C87a97yrjf7.fsf%40schwinge.name%3E>),
that the handling of the openacc flag in
gcc/fortran/openmp.c:resolve_omp_clauses with regard to Â(list !=
OMP_LIST_MAP || openacc)Â, and further down, resolve_oacc_data_clauses
and resolve_oacc_deviceptr_clause is strange.


What is the purpose of gcc/fortran/gfortran.h:OMP_LIST_FIRST?  In
gcc/fortran/gfortran.h, this is made an alias to OMP_LIST_PRIVATE, which
is then, by the OMP_LIST_FIRST name, used in
gcc/fortran/openmp.c:oacc_compatible_clauses.  Should the latter just use
the OMP_LIST_PRIVATE name, and OMP_LIST_FIRST be removed?


GrÃÃe,
 Thomas

Attachment: signature.asc
Description: PGP signature