[patch, openmp/openacc] Allow Fortran parameters in map/copy.

[patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 1093 bytes --]

This patch allows Fortran "parameter" named constants to be named in 
OpenMP map and OpenACC copy directives.

Right now, the compiler just says something like:

      !$omp target data map(tofrom:N,x)
                                  1
   Error: Object 'n' is not a variable at (1)

This is technically correct, but is surprising to the user and AFAICT 
the OpenMP/OpenACC documents don't say you can't name parameters, so I'd 
like it to be allowed.

Also, I suspect the support request that led me here originated from 
code originally written for another toolchain, so this is a portability 
issue.

With this patch the compiler allows the parameter in the directive, but 
just ignores it (deletes it), so that it has no effect. This is safe 
since the named constant will have been substituted for the actual value 
everywhere it occurs. If that is not true in some case (can it happen?), 
I believe it will be automatically mapped in the same way other 
variables are.

Using parameters in device_resident, deviceptr, private, reduction, 
etc., remains an error.

OK to commit?

Andrew

[-- Attachment #2: 190705-map-parameters.patch --]
[-- Type: text/x-patch, Size: 4113 bytes --]

Allow Fortran parameters in map/copy directives.

2019-07-05  Andrew Stubbs  <ams@codesourcery.com>

	gcc/fortran/
	* gfortran.h (gfc_free_omp_namelist_entry): New prototype.
	* match.c (gfc_free_omp_namelist): Break contents out into ...
	(gfc_free_omp_namelist_entry): ... here.
	* openmp.c (resolve_omp_clauses): Delete parameters in map to and from.

	gcc/testsuite/
	* gfortran.dg/goacc/parameter.f95: Allow parameters in copy.
	* gfortran.dg/gomp/map-1.f90: Allow parameters in map, only.

diff --git a/gcc/fortran/gfortran.h b/gcc/fortran/gfortran.h
index b1f7bd0604a..5363cbd331b 100644
--- a/gcc/fortran/gfortran.h
+++ b/gcc/fortran/gfortran.h
@@ -3181,6 +3181,7 @@ void gfc_free_iterator (gfc_iterator *, int);
 void gfc_free_forall_iterator (gfc_forall_iterator *);
 void gfc_free_alloc_list (gfc_alloc *);
 void gfc_free_namelist (gfc_namelist *);
+void gfc_free_omp_namelist_entry (gfc_omp_namelist *);
 void gfc_free_omp_namelist (gfc_omp_namelist *);
 void gfc_free_equiv (gfc_equiv *);
 void gfc_free_equiv_until (gfc_equiv *, gfc_equiv *);
diff --git a/gcc/fortran/match.c b/gcc/fortran/match.c
index 0f3b2132122..51c9e9ef5ed 100644
--- a/gcc/fortran/match.c
+++ b/gcc/fortran/match.c
@@ -5373,6 +5373,23 @@ gfc_free_namelist (gfc_namelist *name)
 }
 
 
+/* Free an OpenMP namelist entry.  */
+
+void
+gfc_free_omp_namelist_entry (gfc_omp_namelist *name)
+{
+  gfc_free_expr (name->expr);
+  if (name->udr)
+    {
+      if (name->udr->combiner)
+	gfc_free_statement (name->udr->combiner);
+      if (name->udr->initializer)
+	gfc_free_statement (name->udr->initializer);
+      free (name->udr);
+    }
+  free (name);
+}
+
 /* Free an OpenMP namelist structure.  */
 
 void
@@ -5382,17 +5399,8 @@ gfc_free_omp_namelist (gfc_omp_namelist *name)
 
   for (; name; name = n)
     {
-      gfc_free_expr (name->expr);
-      if (name->udr)
-	{
-	  if (name->udr->combiner)
-	    gfc_free_statement (name->udr->combiner);
-	  if (name->udr->initializer)
-	    gfc_free_statement (name->udr->initializer);
-	  free (name->udr);
-	}
       n = name->next;
-      free (name);
+      gfc_free_omp_namelist_entry (name);
     }
 }
 
diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 1c7bce6c300..d06221b41ed 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -4165,6 +4165,22 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 		   "clause at %L", &code->loc);
     }
 
+  /* Remove no-op mappings, such as parameters.  */
+  gfc_omp_namelist **parent = &omp_clauses->lists[OMP_LIST_MAP];
+  for (n = omp_clauses->lists[OMP_LIST_MAP]; n; n = n->next)
+    {
+      if ((n->u.map_op == OMP_MAP_TO
+	   || n->u.map_op == OMP_MAP_FROM
+	   || n->u.map_op == OMP_MAP_TOFROM)
+	  && n->sym->attr.flavor == FL_PARAMETER)
+	{
+	  *parent = n->next;
+	  gfc_free_omp_namelist_entry (n);
+	}
+      else
+	parent = &n->next;
+    }
+
   /* Check that no symbol appears on multiple clauses, except that
      a symbol can appear on both firstprivate and lastprivate.  */
   for (list = 0; list < OMP_LIST_NUM; list++)
diff --git a/gcc/testsuite/gfortran.dg/goacc/parameter.f95 b/gcc/testsuite/gfortran.dg/goacc/parameter.f95
index 84274611915..edfaeeadc76 100644
--- a/gcc/testsuite/gfortran.dg/goacc/parameter.f95
+++ b/gcc/testsuite/gfortran.dg/goacc/parameter.f95
@@ -7,7 +7,7 @@ contains
     integer :: i
     integer, parameter :: a = 1
     !$acc declare device_resident (a) ! { dg-error "PARAMETER" }
-    !$acc data copy (a) ! { dg-error "not a variable" }
+    !$acc data copy (a) ! This is permitted.
     !$acc end data
     !$acc data deviceptr (a) ! { dg-error "not a variable" }
     !$acc end data
diff --git a/gcc/testsuite/gfortran.dg/gomp/map-1.f90 b/gcc/testsuite/gfortran.dg/gomp/map-1.f90
index e78b56c8f39..abe448a33f9 100644
--- a/gcc/testsuite/gfortran.dg/gomp/map-1.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/map-1.f90
@@ -18,7 +18,7 @@ subroutine test(aas)
   !$omp target map(j)
   !$omp end target
 
-  !$omp target map(p) ! { dg-error "Object 'p' is not a variable" }
+  !$omp target map(p)
   !$omp end target
 
   !$omp target map(j(1))

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Jakub Jelinek]

On Fri, Jul 05, 2019 at 12:31:17PM +0100, Andrew Stubbs wrote:
> This patch allows Fortran "parameter" named constants to be named in OpenMP
> map and OpenACC copy directives.
> 
> Right now, the compiler just says something like:
> 
>      !$omp target data map(tofrom:N,x)
>                                  1
>   Error: Object 'n' is not a variable at (1)
> 
> This is technically correct, but is surprising to the user and AFAICT the
> OpenMP/OpenACC documents don't say you can't name parameters, so I'd like it
> to be allowed.

I don't think you can do it in OpenMP, I can discuss in the language
committee, but the definition of variable is:

"A named data storage block, for which the value can be defined and redefined during
the execution of a program."

so I don't believe Fortran parameters are OpenMP variables.
Scalar Fortran parameters don't even have any data storage block associated
with them at all.

	Jakub

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Andrew Stubbs]

On 05/07/2019 12:49, Jakub Jelinek wrote:
> On Fri, Jul 05, 2019 at 12:31:17PM +0100, Andrew Stubbs wrote:
>> This patch allows Fortran "parameter" named constants to be named in OpenMP
>> map and OpenACC copy directives.
>>
>> Right now, the compiler just says something like:
>>
>>       !$omp target data map(tofrom:N,x)
>>                                   1
>>    Error: Object 'n' is not a variable at (1)
>>
>> This is technically correct, but is surprising to the user and AFAICT the
>> OpenMP/OpenACC documents don't say you can't name parameters, so I'd like it
>> to be allowed.
> 
> I don't think you can do it in OpenMP, I can discuss in the language
> committee, but the definition of variable is:
> 
> "A named data storage block, for which the value can be defined and redefined during
> the execution of a program."
> 
> so I don't believe Fortran parameters are OpenMP variables.
> Scalar Fortran parameters don't even have any data storage block associated
> with them at all.

Agreed, they are not variables (nor variable), and copying them about is 
entirely pointless (although, if you simply remove the error check GCC 
will happily create storage for 'N' and do the copies).

And yet, this issue has generated a support request from a blocked 
customer, and I'd like to improve their experience.

I could generate a warning, or add a note to the error message, but in 
the interests of portability I thought just doing the right thing would 
be nicer.

Any suggestions?

Andrew

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Jakub Jelinek]

On Fri, Jul 05, 2019 at 01:10:00PM +0100, Andrew Stubbs wrote:
> > so I don't believe Fortran parameters are OpenMP variables.
> > Scalar Fortran parameters don't even have any data storage block associated
> > with them at all.
> 
> Agreed, they are not variables (nor variable), and copying them about is
> entirely pointless (although, if you simply remove the error check GCC will
> happily create storage for 'N' and do the copies).
> 
> And yet, this issue has generated a support request from a blocked customer,
> and I'd like to improve their experience.
> 
> I could generate a warning, or add a note to the error message, but in the
> interests of portability I thought just doing the right thing would be
> nicer.
> 
> Any suggestions?

I don't know what OpenACC says, perhaps it is different.

For OpenMP, a warning is not good enough, because if Fortran PARAMETER is
not a variable, then the program is invalid and needs to be rejected.
You could improve the diagnostics by adding some explanation message that fortran
PARAMETER is not a variable.

	Jakub

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Jakub Jelinek]

On Fri, Jul 05, 2019 at 02:15:12PM +0200, Jakub Jelinek wrote:
> On Fri, Jul 05, 2019 at 01:10:00PM +0100, Andrew Stubbs wrote:
> > > so I don't believe Fortran parameters are OpenMP variables.
> > > Scalar Fortran parameters don't even have any data storage block associated
> > > with them at all.
> > 
> > Agreed, they are not variables (nor variable), and copying them about is
> > entirely pointless (although, if you simply remove the error check GCC will
> > happily create storage for 'N' and do the copies).
> > 
> > And yet, this issue has generated a support request from a blocked customer,
> > and I'd like to improve their experience.
> > 
> > I could generate a warning, or add a note to the error message, but in the
> > interests of portability I thought just doing the right thing would be
> > nicer.
> > 
> > Any suggestions?
> 
> I don't know what OpenACC says, perhaps it is different.
> 
> For OpenMP, a warning is not good enough, because if Fortran PARAMETER is
> not a variable, then the program is invalid and needs to be rejected.
> You could improve the diagnostics by adding some explanation message that fortran
> PARAMETER is not a variable.

Got it confirmed, it might change in a future OpenMP release; and there has
been a change already in OpenMP 3.1 that might have intended to allow
Fortran PARAMETERs in firstprivate clause, but because the glossary hasn't
been changed, it is not valid even in firstprivate clause.

Note, if there is adjusted wording for Fortran PARAMETERs, it shouldn't be
done just for map clause, but all other OpenMP data sharing clauses.  Even
if PARAMETERs are eventually allowed as variables, it will be still limited
to firstprivate clause and perhaps some sorts of map clauses, nothing else,
e.g. for non-firstprivate privatization clauses you really need a definable
object or allocatable.

	Jakub

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 853 bytes --]

On 05/07/2019 15:04, Jakub Jelinek wrote:
>> For OpenMP, a warning is not good enough, because if Fortran PARAMETER is
>> not a variable, then the program is invalid and needs to be rejected.
>> You could improve the diagnostics by adding some explanation message that fortran
>> PARAMETER is not a variable.
> 
> Got it confirmed, it might change in a future OpenMP release; and there has
> been a change already in OpenMP 3.1 that might have intended to allow
> Fortran PARAMETERs in firstprivate clause, but because the glossary hasn't
> been changed, it is not valid even in firstprivate clause.

OK, here is an alternative patch that merely tries to make the error 
message more informative.

Basically, the user needs to get past "it isn't working but I need that 
value in this kernel", so hopefully this will help get them there.

WDYT?

Andrew

[-- Attachment #2: 190705-map-parameters-2.patch --]
[-- Type: text/x-patch, Size: 1676 bytes --]

Tweak error message for mapped parameters.

2019-07-05  Andrew Stubbs  <ams@codesourcery.com>

	gcc/fortran/
	* openmp.c (resolve_omp_clauses): Add custom error messages for
	parameters in map clauses.

	gcc/testsuite/
	* gfortran.dg/gomp/map-1.f90: Update error message text.

diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 1c7bce6c300..e05bf84faf2 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -4208,8 +4208,21 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 		  continue;
 	      }
 	  }
-	gfc_error ("Object %qs is not a variable at %L", n->sym->name,
-		   &n->where);
+	if (list == OMP_LIST_MAP
+	    && n->sym->attr.flavor == FL_PARAMETER)
+	  {
+	    if (openacc)
+	      gfc_error ("Parameter %qs is not a variable at %L; it probably"
+			 " doesn't need to be copied", n->sym->name,
+			 &n->where);
+	    else
+	      gfc_error ("Parameter %qs is not a variable at %L; it probably"
+			 " doesn't need to be mapped", n->sym->name,
+			 &n->where);
+	  }
+	else
+	  gfc_error ("Object %qs is not a variable at %L", n->sym->name,
+		     &n->where);
       }
 
   for (list = 0; list < OMP_LIST_NUM; list++)
diff --git a/gcc/testsuite/gfortran.dg/gomp/map-1.f90 b/gcc/testsuite/gfortran.dg/gomp/map-1.f90
index e78b56c8f39..8967b4dd0b5 100644
--- a/gcc/testsuite/gfortran.dg/gomp/map-1.f90
+++ b/gcc/testsuite/gfortran.dg/gomp/map-1.f90
@@ -18,7 +18,7 @@ subroutine test(aas)
   !$omp target map(j)
   !$omp end target
 
-  !$omp target map(p) ! { dg-error "Object 'p' is not a variable" }
+  !$omp target map(p) ! { dg-error "Parameter 'p' is not a variable" }
   !$omp end target
 
   !$omp target map(j(1))

Re: [patch, openmp/openacc] Allow Fortran parameters in map/copy.

[Jakub Jelinek]

On Fri, Jul 05, 2019 at 03:40:55PM +0100, Andrew Stubbs wrote:
> On 05/07/2019 15:04, Jakub Jelinek wrote:
> > > For OpenMP, a warning is not good enough, because if Fortran PARAMETER is
> > > not a variable, then the program is invalid and needs to be rejected.
> > > You could improve the diagnostics by adding some explanation message that fortran
> > > PARAMETER is not a variable.
> > 
> > Got it confirmed, it might change in a future OpenMP release; and there has
> > been a change already in OpenMP 3.1 that might have intended to allow
> > Fortran PARAMETERs in firstprivate clause, but because the glossary hasn't
> > been changed, it is not valid even in firstprivate clause.
> 
> OK, here is an alternative patch that merely tries to make the error message
> more informative.
> 
> Basically, the user needs to get past "it isn't working but I need that
> value in this kernel", so hopefully this will help get them there.
> 
> WDYT?

I don't like it, I'd end the message where you put ;, the rest doesn't make
sense and will just confuse users.  For one, the compiler should know if the
parameter needs to be copied or not, so doesn't need to talk about
probability thereof, and if it needs to be copied, it should be the compiler
that arranges that (PR90779) and the user has no way to overide or force
that behavior.

	Jakub

[committed, openmp/openacc] Tweak error message for mapped parameters

[Andrew Stubbs]

[-- Attachment #1: Type: text/plain, Size: 777 bytes --]

On 05/07/2019 15:49, Jakub Jelinek wrote:
>> OK, here is an alternative patch that merely tries to make the error message
>> more informative.
>>
>> Basically, the user needs to get past "it isn't working but I need that
>> value in this kernel", so hopefully this will help get them there.
>>
>> WDYT?
> 
> I don't like it, I'd end the message where you put ;, the rest doesn't make
> sense and will just confuse users.  For one, the compiler should know if the
> parameter needs to be copied or not, so doesn't need to talk about
> probability thereof, and if it needs to be copied, it should be the compiler
> that arranges that (PR90779) and the user has no way to overide or force
> that behavior.

Here's what Jakub approved via IRC. Now committed.

Thanks Jakub

Andrew

[-- Attachment #2: 190705-map-parameters-3.patch --]
[-- Type: text/x-patch, Size: 1105 bytes --]

Tweak error message for mapped parameters.

2019-07-05  Andrew Stubbs  <ams@codesourcery.com>

	gcc/fortran/
	* openmp.c (resolve_omp_clauses): Add custom error messages for
	parameters in map clauses.

diff --git a/gcc/fortran/openmp.c b/gcc/fortran/openmp.c
index 1c7bce6c300..44fcb9db8c6 100644
--- a/gcc/fortran/openmp.c
+++ b/gcc/fortran/openmp.c
@@ -4208,8 +4208,21 @@ resolve_omp_clauses (gfc_code *code, gfc_omp_clauses *omp_clauses,
 		  continue;
 	      }
 	  }
-	gfc_error ("Object %qs is not a variable at %L", n->sym->name,
-		   &n->where);
+	if (list == OMP_LIST_MAP
+	    && n->sym->attr.flavor == FL_PARAMETER)
+	  {
+	    if (openacc)
+	      gfc_error ("Object %qs is not a variable at %L; parameters"
+			 " cannot be and need not be copied", n->sym->name,
+			 &n->where);
+	    else
+	      gfc_error ("Object %qs is not a variable at %L; parameters"
+			 " cannot be and need not be mapped", n->sym->name,
+			 &n->where);
+	  }
+	else
+	  gfc_error ("Object %qs is not a variable at %L", n->sym->name,
+		     &n->where);
       }
 
   for (list = 0; list < OMP_LIST_NUM; list++)