[2/5,OpenACC] Support Fortran optional arguments in the firstprivate clause

Reference types used by Fortran often need to be treated specially in 
the OACC lowering to deal with the referenced object as well as the 
reference itself. However, as optional arguments can be null, they are 
pointer types rather than reference types, so the code to detect these 
situations needs to be updated.

	gcc/
	* omp-general.c (omp_is_optional_argument): New.
	* omp-general.h (omp_is_optional_argument): New.
	* omp-low.c (lower_omp_target): Use size of referenced object when
	optional argument used as argument to firstprivate.  Create temporary
	for received value and take the address for new_var if the original
	variable was an optional argument.
---
  gcc/omp-general.c | 14 ++++++++++++++
  gcc/omp-general.h |  1 +
  gcc/omp-low.c     |  8 +++++---
  3 files changed, 20 insertions(+), 3 deletions(-)

  		       value.  */
@@ -11461,7 +11462,8 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, 
omp_context *ctx)
  	      {
  		gcc_checking_assert (is_gimple_omp_oacc (ctx->stmt));
  		s = TREE_TYPE (ovar);
-		if (TREE_CODE (s) == REFERENCE_TYPE)
+		if (TREE_CODE (s) == REFERENCE_TYPE
+		    || omp_is_optional_argument (ovar))
  		  s = TREE_TYPE (s);
  		s = TYPE_SIZE_UNIT (s);
  	      }

On Fri, Jul 12, 2019 at 12:36:13PM +0100, Kwok Cheung Yeung wrote:
> --- a/gcc/omp-general.c
> +++ b/gcc/omp-general.c
> @@ -48,6 +48,20 @@ omp_find_clause (tree clauses, enum omp_clause_code kind)
>    return NULL_TREE;
>  }
> 
> +/* Return true if DECL is a Fortran optional argument.  */
> +
> +bool
> +omp_is_optional_argument (tree decl)
> +{
> +  /* A passed-by-reference Fortran optional argument is similar to
> +     a normal argument, but since it can be null the type is a
> +     POINTER_TYPE rather than a REFERENCE_TYPE.  */
> +  return lang_GNU_Fortran ()
> +        && TREE_CODE (decl) == PARM_DECL
> +        && DECL_BY_REFERENCE (decl)
> +	 && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE;
> +}

This should be done through a langhook.
Are really all PARM_DECLs wtih DECL_BY_REFERENCE and pointer type optional
arguments?  I mean, POINTER_TYPE is used for a lot of cases.

	Jakub

On 12/07/2019 12:41 pm, Jakub Jelinek wrote:
> This should be done through a langhook.
> Are really all PARM_DECLs wtih DECL_BY_REFERENCE and pointer type optional
> arguments?  I mean, POINTER_TYPE is used for a lot of cases.

Hmmm... I thought it was the case that if you pass an argument in by reference 
(the default) in Fortran, the PARM_DECL will always be a reference to the 
argument type if non-optional, or a pointer if optional. However, fixed-shape 
arrays are passed in via a pointer whether optional or not...

I also experimented with passing in a pointer by value, but it seems like that 
is not allowed. e.g.

   subroutine foo(x)
     integer, pointer, value :: x
   end subroutine foo

results in:

    11 |     integer, pointer, value :: x
       |                           1
Error: VALUE attribute conflicts with POINTER attribute at (1)

Are there any more examples in Fortran where a PARM_DECL is a pointer type 
without being optional?

In the Fortran FE, optional arguments are indicated by setting attr.optional on 
the gfc_symbol for the parameter, but the OMP lowering works on a tree - is it 
somehow possible to get from the tree back to the gfc_symbol? If so, that would 
be a more reliable method of detecting optional arguments.

Thanks

Kwok

Am 17.07.19 um 19:53 schrieb Kwok Cheung Yeung:
> On 12/07/2019 12:41 pm, Jakub Jelinek wrote:
>> This should be done through a langhook.
>> Are really all PARM_DECLs wtih DECL_BY_REFERENCE and pointer type
>> optional
>> arguments?  I mean, POINTER_TYPE is used for a lot of cases.
>
> Hmmm... I thought it was the case that if you pass an argument in by
> reference (the default) in Fortran, the PARM_DECL will always be a
> reference to the argument type if non-optional, or a pointer if
> optional. However, fixed-shape arrays are passed in via a pointer
> whether optional or not...

[I have to admit that I have not yet read the OpenACC (nor OpenMP 5)
spec to know the semantics and whether it matters if something is a true
pointer or just optional.]


The following is a rather special case (matching a C "void *" pointer),
which is useless without later casting the argument ("c_f_pointer"
call), but it is a pointer argument which is not by reference and not
optional.


use iso_c_binding
implicit none
call foo(c_null_ptr)
contains
  subroutine foo(x)
    type(c_ptr), value :: x ! Matches a C 'void *' pointer
  end subroutine foo
end

Maybe there are more methods, but that requires some pondering.


> In the Fortran FE, optional arguments are indicated by setting
> attr.optional on the gfc_symbol for the parameter, but the OMP
> lowering works on a tree - is it somehow possible to get from the tree
> back to the gfc_symbol? If so, that would be a more reliable method of
> detecting optional arguments. 

The gfc_symbol etc. is gone. The only possibility is to store some extra
data in the language-dependent part of the tree, i.e. using
DECL_LANG_SPECIFIC. Cf. lang_decl in trans.h and the various #defines
which use DECL_LANG_SPECIFIC.

Cheers,

Tobias

Hi all,

I played around and came up with another second way one gets a single "*" without
'optional'.

I haven't checked whether which of those match the proposed omp_is_optional_argument's
+        && DECL_BY_REFERENCE (decl)
+	 && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE;
nor whether some checks reject any of those with OpenACC (or OpenMP).

In any case, the dump of "type(c_ptr),value", "integer, dimension(1)" and "integer,optional"
is:

static void foo (void *, integer(kind=4)[1] *, integer(kind=4) *);


Actually, if one combines VALUE with OPTIONAL, it gets even more interesting.
To implement it, one has two choices:
* pass a copy by reference (or NULL)
* pass by value (including a dummy value if absent) and denote the state as
  extra argument.

The latter is done in gfortran, cf. PR fortran/35203, following the IBM compiler.


I am not sure whether it does need special care fore OpenACC (or OpenMP 5) offloading,
but that's at least a case which is not handled by the patch.


Actually, there is a bug: the declaration of the function and the definition of
the function is not the same - one misses the hidden argument :-(

That's now PR fortran/91196.


Fortran code - which now also contains VALUE, OPTIONAL:

use iso_c_binding
implicit none
logical(kind=c_bool) :: is_present
integer :: y(1)
y(1) = 5
is_present = foo(c_null_ptr, y)
contains
  logical(kind=c_bool) function foo(x, y, z, z2)
    type(c_ptr), value :: x ! Matches a C 'void *' pointer
    integer, target :: y(1)
    integer, optional :: z
    integer, value, optional :: z2

    foo = present(z2)
  end function foo
end


Tobias

On 18/07/2019 10:28 am, Tobias Burnus wrote:
> Hi all,
> 
> I played around and came up with another second way one gets a single "*" without
> 'optional'.
> 
> I haven't checked whether which of those match the proposed omp_is_optional_argument's
> +        && DECL_BY_REFERENCE (decl)
> +	 && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE;
> nor whether some checks reject any of those with OpenACC (or OpenMP).
> 
> In any case, the dump of "type(c_ptr),value", "integer, dimension(1)" and "integer,optional"
> is:
> 
> static void foo (void *, integer(kind=4)[1] *, integer(kind=4) *);
> 

The case of the fixed-length array currently doesn't work properly with 
omp_is_optional_argument, as it returns true whether or not it is optional. 
Indeed, the PARM_DECL doesn't seem to change between optional and non-optional, 
so it is probably impossible to discern via the tree unless some extra 
information is added by the front-end.

However, optional arrays still 'just work' with my patches (most of the 
testcases include tests for arrays in optional arguments). I believe this is 
because the existing code must already deal with pointers to arrays, so the 
false positive simply does not matter on the codepath taken. The new case of a 
null pointer (in the case of a non-present optional argument) was dealt with by 
making operations on null pointers into NOPs.

> 
> Actually, if one combines VALUE with OPTIONAL, it gets even more interesting.
> To implement it, one has two choices:
> * pass a copy by reference (or NULL)
> * pass by value (including a dummy value if absent) and denote the state as
>    extra argument.
> 
> The latter is done in gfortran, cf. PR fortran/35203, following the IBM compiler.
> 
> 
> I am not sure whether it does need special care fore OpenACC (or OpenMP 5) offloading,
> but that's at least a case which is not handled by the patch.
> 

Optional by-value arguments are tested in optional-data-copyin-by-value.f90. 
They do not need extra handling, since from the OACC lowering perspective they 
are just two bog-standard integral values of no special interest.

Kwok

On 12/07/2019 12:41 pm, Jakub Jelinek wrote:
>> +/* Return true if DECL is a Fortran optional argument.  */
>> +
>> +bool
>> +omp_is_optional_argument (tree decl)
>> +{
>> +  /* A passed-by-reference Fortran optional argument is similar to
>> +     a normal argument, but since it can be null the type is a
>> +     POINTER_TYPE rather than a REFERENCE_TYPE.  */
>> +  return lang_GNU_Fortran ()
>> +        && TREE_CODE (decl) == PARM_DECL
>> +        && DECL_BY_REFERENCE (decl)
>> +	 && TREE_CODE (TREE_TYPE (decl)) == POINTER_TYPE;
>> +}
> 
> This should be done through a langhook.
> Are really all PARM_DECLs wtih DECL_BY_REFERENCE and pointer type optional
> arguments?  I mean, POINTER_TYPE is used for a lot of cases.
> 

I have now added a language hook for omp_is_optional_argument, implemented only 
for Fortran. In the Fortran FE, I have introduced a language-specific decl flag 
that is set to true during the construction of the PARM_DECLs for a function if 
the symbol for the parameter has attr.optional set. The implementation of 
omp_is_optional_argument checks this flag - this way, there should be no false 
positives.

This implementation of omp_is_optional_argument returns true for both 
by-reference and by-value optional arguments, so an extra check of the type is 
needed to differentiate between the two. I have also made by-reference optional 
arguments return true for gfc_omp_privatize_by_reference (since they should 
behave like regular arguments if present), which simplifies the code a little.

 >  		if (omp_is_reference (new_var)
 > 		    && TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE)

As is, this code in lower_omp_target will always reject optional arguments, so 
it must be changed. This was introduced in commit r261025 (SVN trunk) as a fix 
for PR 85879, but it also breaks support for arrays in firstprivate, which was 
probably an unintended side-effect. I have found that allowing POINTER_TYPEs 
that are also DECL_BY_REFERENCE through in this condition allows both optional 
arguments and arrays to work without regressing the tests in r261025.

Okay for trunk?

Kwok

	gcc/fortran/
	* f95-lang.c (LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT): Define to
	gfc_omp_is_optional_argument.
	* trans-decl.c (create_function_arglist): Set
	GFC_DECL_OPTIONAL_ARGUMENT in the generated decl if the parameter is
	optional.
	* trans-openmp.c (gfc_omp_is_optional_argument): New.
	(gfc_omp_privatize_by_reference): Return true if the decl is an
	optional pass-by-reference argument.
	* trans.h (gfc_omp_is_optional_argument): New declaration.
	(lang_decl): Add new optional_arg field.
	(GFC_DECL_OPTIONAL_ARGUMENT): New macro.

	gcc/
	* langhooks-def.h (LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT): Default to
	false.
	(LANG_HOOKS_DECLS): Add LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT.
	* langhooks.h (omp_is_optional_argument): New hook.
	* omp-general.c (omp_is_optional_argument): New.
	* omp-general.h (omp_is_optional_argument): New declaration.
	* omp-low.c (lower_omp_target): Create temporary for received value
	and take the address for new_var if the original variable was a
	DECL_BY_REFERENCE.  Use size of referenced object when a
	pass-by-reference optional argument used as argument to firstprivate.
---
  gcc/fortran/f95-lang.c     |  2 ++
  gcc/fortran/trans-decl.c   |  5 +++++
  gcc/fortran/trans-openmp.c | 13 +++++++++++++
  gcc/fortran/trans.h        |  4 ++++
  gcc/langhooks-def.h        |  2 ++
  gcc/langhooks.h            |  3 +++
  gcc/omp-general.c          |  8 ++++++++
  gcc/omp-general.h          |  1 +
  gcc/omp-low.c              |  7 +++++--
  9 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index 6b9f490..2467cd9 100644
--- a/gcc/fortran/f95-lang.c
+++ b/gcc/fortran/f95-lang.c
@@ -113,6 +113,7 @@ static const struct attribute_spec gfc_attribute_table[] =
  #undef LANG_HOOKS_TYPE_FOR_MODE
  #undef LANG_HOOKS_TYPE_FOR_SIZE
  #undef LANG_HOOKS_INIT_TS
+#undef LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT
  #undef LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE
  #undef LANG_HOOKS_OMP_PREDETERMINED_SHARING
  #undef LANG_HOOKS_OMP_REPORT_DECL
@@ -145,6 +146,7 @@ static const struct attribute_spec gfc_attribute_table[] =
  #define LANG_HOOKS_TYPE_FOR_MODE	gfc_type_for_mode
  #define LANG_HOOKS_TYPE_FOR_SIZE	gfc_type_for_size
  #define LANG_HOOKS_INIT_TS		gfc_init_ts
+#define LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT	gfc_omp_is_optional_argument
  #define LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE	gfc_omp_privatize_by_reference
  #define LANG_HOOKS_OMP_PREDETERMINED_SHARING	gfc_omp_predetermined_sharing
  #define LANG_HOOKS_OMP_REPORT_DECL		gfc_omp_report_decl
diff --git a/gcc/fortran/trans-decl.c b/gcc/fortran/trans-decl.c
index 64ce4bb..14f4d21 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -2632,6 +2632,11 @@ create_function_arglist (gfc_symbol * sym)
  	  && (!f->sym->attr.proc_pointer
  	      && f->sym->attr.flavor != FL_PROCEDURE))
  	DECL_BY_REFERENCE (parm) = 1;
+      if (f->sym->attr.optional)
+	{
+	  gfc_allocate_lang_decl (parm);
+	  GFC_DECL_OPTIONAL_ARGUMENT (parm) = 1;
+	}

        gfc_finish_decl (parm);
        gfc_finish_decl_attrs (parm, &f->sym->attr);
diff --git a/gcc/fortran/trans-openmp.c b/gcc/fortran/trans-openmp.c
index 8eae7bc..a117bcc 100644
--- a/gcc/fortran/trans-openmp.c
+++ b/gcc/fortran/trans-openmp.c
@@ -47,6 +47,15 @@ along with GCC; see the file COPYING3.  If not see

  int ompws_flags;

+/* True if OpenMP should treat this DECL as an optional argument.  */
+
+bool
+gfc_omp_is_optional_argument (const_tree decl)
+{
+  return DECL_LANG_SPECIFIC (decl)
+	 && GFC_DECL_OPTIONAL_ARGUMENT (decl);
+}
+
  /* True if OpenMP should privatize what this DECL points to rather
     than the DECL itself.  */

@@ -59,6 +68,10 @@ gfc_omp_privatize_by_reference (const_tree decl)
        && (!DECL_ARTIFICIAL (decl) || TREE_CODE (decl) == PARM_DECL))
      return true;

+  if (TREE_CODE (type) == POINTER_TYPE
+      && gfc_omp_is_optional_argument (decl))
+    return true;
+
    if (TREE_CODE (type) == POINTER_TYPE)
      {
        /* Array POINTER/ALLOCATABLE have aggregate types, all user variables
diff --git a/gcc/fortran/trans.h b/gcc/fortran/trans.h
index 0305d33..7f9f6e8 100644
--- a/gcc/fortran/trans.h
+++ b/gcc/fortran/trans.h
@@ -776,6 +776,7 @@ struct array_descr_info;
  bool gfc_get_array_descr_info (const_tree, struct array_descr_info *);

  /* In trans-openmp.c */
+bool gfc_omp_is_optional_argument (const_tree);
  bool gfc_omp_privatize_by_reference (const_tree);
  enum omp_clause_default_kind gfc_omp_predetermined_sharing (tree);
  tree gfc_omp_report_decl (tree);
@@ -989,6 +990,7 @@ struct GTY(()) lang_decl {
    tree token, caf_offset;
    unsigned int scalar_allocatable : 1;
    unsigned int scalar_pointer : 1;
+  unsigned int optional_arg : 1;
  };


@@ -1003,6 +1005,8 @@ struct GTY(()) lang_decl {
    (DECL_LANG_SPECIFIC (node)->scalar_allocatable)
  #define GFC_DECL_SCALAR_POINTER(node) \
    (DECL_LANG_SPECIFIC (node)->scalar_pointer)
+#define GFC_DECL_OPTIONAL_ARGUMENT(node) \
+  (DECL_LANG_SPECIFIC (node)->optional_arg)
  #define GFC_DECL_GET_SCALAR_ALLOCATABLE(node) \
    (DECL_LANG_SPECIFIC (node) ? GFC_DECL_SCALAR_ALLOCATABLE (node) : 0)
  #define GFC_DECL_GET_SCALAR_POINTER(node) \
diff --git a/gcc/langhooks-def.h b/gcc/langhooks-def.h
index a059841..55d5fe0 100644
--- a/gcc/langhooks-def.h
+++ b/gcc/langhooks-def.h
@@ -236,6 +236,7 @@ extern tree lhd_unit_size_without_reusable_padding (tree);
  #define LANG_HOOKS_WARN_UNUSED_GLOBAL_DECL lhd_warn_unused_global_decl
  #define LANG_HOOKS_POST_COMPILATION_PARSING_CLEANUPS NULL
  #define LANG_HOOKS_DECL_OK_FOR_SIBCALL	lhd_decl_ok_for_sibcall
+#define LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT hook_bool_const_tree_false
  #define LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE hook_bool_const_tree_false
  #define LANG_HOOKS_OMP_PREDETERMINED_SHARING lhd_omp_predetermined_sharing
  #define LANG_HOOKS_OMP_REPORT_DECL lhd_pass_through_t
@@ -261,6 +262,7 @@ extern tree lhd_unit_size_without_reusable_padding (tree);
    LANG_HOOKS_WARN_UNUSED_GLOBAL_DECL, \
    LANG_HOOKS_POST_COMPILATION_PARSING_CLEANUPS, \
    LANG_HOOKS_DECL_OK_FOR_SIBCALL, \
+  LANG_HOOKS_OMP_IS_OPTIONAL_ARGUMENT, \
    LANG_HOOKS_OMP_PRIVATIZE_BY_REFERENCE, \
    LANG_HOOKS_OMP_PREDETERMINED_SHARING, \
    LANG_HOOKS_OMP_REPORT_DECL, \
diff --git a/gcc/langhooks.h b/gcc/langhooks.h
index a45579b..9d2714a 100644
--- a/gcc/langhooks.h
+++ b/gcc/langhooks.h
@@ -222,6 +222,9 @@ struct lang_hooks_for_decls
    /* True if this decl may be called via a sibcall.  */
    bool (*ok_for_sibcall) (const_tree);

+  /* True if OpenMP should treat DECL as a Fortran optional argument.  */
+  bool (*omp_is_optional_argument) (const_tree);
+
    /* True if OpenMP should privatize what this DECL points to rather
       than the DECL itself.  */
    bool (*omp_privatize_by_reference) (const_tree);
diff --git a/gcc/omp-general.c b/gcc/omp-general.c
index 8086f9a..1a09dc1 100644
--- a/gcc/omp-general.c
+++ b/gcc/omp-general.c
@@ -48,6 +48,14 @@ omp_find_clause (tree clauses, enum omp_clause_code kind)
    return NULL_TREE;
  }

+/* Return true if DECL is a Fortran optional argument.  */
+
+bool
+omp_is_optional_argument (tree decl)
+{
+  return lang_hooks.decls.omp_is_optional_argument (decl);
+}
+
  /* Return true if DECL is a reference type.  */

  bool
diff --git a/gcc/omp-general.h b/gcc/omp-general.h
index 80d42af..bbaa7b1 100644
--- a/gcc/omp-general.h
+++ b/gcc/omp-general.h
@@ -73,6 +73,7 @@ struct omp_for_data
  #define OACC_FN_ATTRIB "oacc function"

  extern tree omp_find_clause (tree clauses, enum omp_clause_code kind);
+extern bool omp_is_optional_argument (tree decl);
  extern bool omp_is_reference (tree decl);
  extern void omp_adjust_for_condition (location_t loc, enum tree_code *cond_code,
  				      tree *n2, tree v, tree step);
diff --git a/gcc/omp-low.c b/gcc/omp-low.c
index a855c5b..1c7b43b 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11173,7 +11173,8 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, 
omp_context *ctx)
  	      {
  		gcc_assert (is_gimple_omp_oacc (ctx->stmt));
  		if (omp_is_reference (new_var)
-		    && TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE)
+		    && (TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE
+		        || DECL_BY_REFERENCE (var)))
  		  {
  		    /* Create a local object to hold the instance
  		       value.  */
@@ -11461,7 +11462,9 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, 
omp_context *ctx)
  	      {
  		gcc_checking_assert (is_gimple_omp_oacc (ctx->stmt));
  		s = TREE_TYPE (ovar);
-		if (TREE_CODE (s) == REFERENCE_TYPE)
+		if (TREE_CODE (s) == REFERENCE_TYPE
+		    || (TREE_CODE (s) == POINTER_TYPE
+			&& omp_is_optional_argument (ovar)))
  		  s = TREE_TYPE (s);
  		s = TYPE_SIZE_UNIT (s);
  	      }

On Mon, Jul 29, 2019 at 09:55:52PM +0100, Kwok Cheung Yeung wrote:
> +/* True if OpenMP should treat this DECL as an optional argument.  */
> +
> +bool
> +gfc_omp_is_optional_argument (const_tree decl)
> +{
> +  return DECL_LANG_SPECIFIC (decl)
> +	 && GFC_DECL_OPTIONAL_ARGUMENT (decl);

I think this ought to be written as:
  return (DECL_LANG_SPECIFIC (decl)
	  && GFC_DECL_OPTIONAL_ARGUMENT (decl));
because otherwise for emacs users (not me) emacs mishandles it.
Also, I wonder if it shouldn't start with TREE_CODE (decl) == PARM_DECL
check, anything else isn't a dummy argument and so can't be optional.

> +}
> +
>  /* True if OpenMP should privatize what this DECL points to rather
>     than the DECL itself.  */
> 

Otherwise LGTM.

	Jakub

Hi Kwok, Tobias!

On 2019-07-29T21:55:52+0100, Kwok Cheung Yeung <kcy@codesourcery.com> wrote:
>  >  		if (omp_is_reference (new_var)
>  > 		    && TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE)
>
> As is, this code in lower_omp_target will always reject optional arguments, so 
> it must be changed. This was introduced in commit r261025 (SVN trunk) as a fix 
> for PR 85879, but it also breaks support for arrays in firstprivate, which was 
> probably an unintended side-effect.

Do we have sufficient testsuite coverage for "arrays in firstprivate", in
all languages?


Grüße
 Thomas


> I have found that allowing POINTER_TYPEs 
> that are also DECL_BY_REFERENCE through in this condition allows both optional 
> arguments and arrays to work without regressing the tests in r261025.

> 	* omp-low.c (lower_omp_target): Create temporary for received value
> 	and take the address for new_var if the original variable was a
> 	DECL_BY_REFERENCE.  [...]

> --- a/gcc/omp-low.c
> +++ b/gcc/omp-low.c
> @@ -11173,7 +11173,8 @@ lower_omp_target (gimple_stmt_iterator *gsi_p, 
> omp_context *ctx)
>   	      {
>   		gcc_assert (is_gimple_omp_oacc (ctx->stmt));
>   		if (omp_is_reference (new_var)
> -		    && TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE)
> +		    && (TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE
> +		        || DECL_BY_REFERENCE (var)))
>   		  {
>   		    /* Create a local object to hold the instance
>   		       value.  */


Grüße
 Thomas

On 07/10/2019 10:25 am, Thomas Schwinge wrote:
> Hi Kwok, Tobias!
> 
> On 2019-07-29T21:55:52+0100, Kwok Cheung Yeung <kcy@codesourcery.com> wrote:
>>   >  		if (omp_is_reference (new_var)
>>   > 		    && TREE_CODE (TREE_TYPE (new_var)) != POINTER_TYPE)
>>
>> As is, this code in lower_omp_target will always reject optional arguments, so
>> it must be changed. This was introduced in commit r261025 (SVN trunk) as a fix
>> for PR 85879, but it also breaks support for arrays in firstprivate, which was
>> probably an unintended side-effect.
> 
> Do we have sufficient testsuite coverage for "arrays in firstprivate", in
> all languages?
> 

I don't know about other languages, but for Fortran, definitely not. The 
only testcase that exercises this AFAIK is optional-firstprivate.f90 in 
part 5 of this patch series.

Kwok