diff mbox series

[omp-low.c,fortran] Simple fix for optional argument handling with OpenMP's use_device_ptr

Message ID d090c696-67d6-a7b0-771f-7e86f922a11d@codesourcery.com
State New
Headers show
Series [omp-low.c,fortran] Simple fix for optional argument handling with OpenMP's use_device_ptr | expand

Commit Message

Tobias Burnus Oct. 1, 2019, 2:19 p.m. UTC 
Hi all,
[For those who got it twice, I actually forget to include the mailing 
lists in the first round. Ups.]

this patch fixes the bug that with "optional" the wrong pointer is used 
with "use_device_ptr"; the bug is already observable without doing 
actual offloading.

Namely, "present(ptr)" checks whether the passed argument is != NULL. 
While using "ptr" – e.g. as "associated(ptr)" – workes the (once) 
dereferenced dummy argument, which matches the actual argument.


The test case is written such that the pointer passed to 
"use_device_ptr" is present.*

Built and regtested on x86_64-gnu-linux without device; I am currently 
doing a full bootstrap + regtesting and want to test it also with nvptx 
offloading. Assuming no issue pops up:

OK for the trunk?


Regarding the patches:

* The first tiny patch is mine

* The second patch which added the lang_hook omp_is_optional_argument is 
the one posted at https://gcc.gnu.org/ml/gcc-patches/2019-07/msg01743.html
This one was approved by Jakub and I only did two things:
(a) re-diff-ed it (trivial as fuzzy worked)
(b) I followed both suggestions of Jakub (PARAM_DECL + adding "( )")


[Motivation of this patch is – besides fixing an issue – to get the 
second patch it, which makes it easier to consolidate some other bits 
and pieces.]


Thanks,

Tobias

* OpenACC (Sec. 2.17) demands that a variable 'arg' in "clauses has no 
effect at runtime if PRESENT(arg) is .false." – Hence, one needs to go 
beyond this patch. That's done in the patch series at 
https://gcc.gnu.org/ml/gcc-patches/2019-07/threads.html#00960 – the 
patch lang_hook patch of this email is 2/5 of that series.

Comments

Jakub Jelinek Oct. 2, 2019, 9:53 a.m. UTC | #1 
On Tue, Oct 01, 2019 at 04:19:11PM +0200, Tobias Burnus wrote:
> OK for the trunk?

Ok, with a small formatting nit below.

> --- 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 (TREE_CODE (decl) == PARM_DECL && DECL_LANG_SPECIFIC (decl)
> +	  && GFC_DECL_OPTIONAL_ARGUMENT (decl));
> +}

I think our coding style is that if the whole && or || condition fits
on one line, it goes on one line, if it doesn't fit, each && or ||
subexpression goes on a separate line, so no mixing of 2 subexpressions on
this line, 3 on following line, 1 on another one.  So the above should be
  return (TREE_CODE (decl) == PARM_DECL
	  && DECL_LANG_SPECIFIC (decl)
	  && GFC_DECL_OPTIONAL_ARGUMENT (decl));

And, as I said earlier, more work will be needed on the OpenMP side to handle
optional arguments e.g. in data sharing clauses properly.

	Jakub
diff mbox series

Patch

	Patch from:
	https://gcc.gnu.org/ml/gcc-patches/2019-07/msg01743.html
	Re: [PATCH 2/5, OpenACC] Support Fortran optional arguments in the firstprivate clause

	Changes: Rediffed, review changes added (added parentheses, PARM_DECL check)

	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              |  3 ++-
 9 files changed, 40 insertions(+), 1 deletion(-)

diff --git a/gcc/fortran/f95-lang.c b/gcc/fortran/f95-lang.c
index 6b9f490d2bb..2467cd968af 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 c2c5d9d1b6a..a113f08e26b 100644
--- a/gcc/fortran/trans-decl.c
+++ b/gcc/fortran/trans-decl.c
@@ -2687,6 +2687,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 b4c77aebf4d..88ecc331166 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 (TREE_CODE (decl) == PARM_DECL && 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 6ebb71de152..405e88dd1c4 100644
--- a/gcc/fortran/trans.h
+++ b/gcc/fortran/trans.h
@@ -786,6 +786,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);
@@ -999,6 +1000,7 @@  struct GTY(()) lang_decl {
   tree token, caf_offset;
   unsigned int scalar_allocatable : 1;
   unsigned int scalar_pointer : 1;
+  unsigned int optional_arg : 1;
 };
 
 
@@ -1013,6 +1015,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 a059841b3df..55d5fe01495 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 a45579b3325..9d2714a5b1d 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 66be94f6ff9..5ef6e251698 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 80d42aff3c8..bbaa7b11707 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 5db182c6841..a0e5041d3f2 100644
--- a/gcc/omp-low.c
+++ b/gcc/omp-low.c
@@ -11395,7 +11395,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.  */