diff mbox series

[v3] omp-offload.cc: Fix value-expr handling of 'declare target link' vars [PR115637] (was: [Patch] gimplify.cc: Handle VALUE_EXPR of MEM_REF's ADDR_EXPR argument [PR115637])

Message ID 0973962b-900d-48d5-92fa-44964f99d3ce@baylibre.com
State New
Headers show
Series [v3] omp-offload.cc: Fix value-expr handling of 'declare target link' vars [PR115637] (was: [Patch] gimplify.cc: Handle VALUE_EXPR of MEM_REF's ADDR_EXPR argument [PR115637]) | expand

Commit Message

Tobias Burnus July 31, 2024, 11:21 a.m. UTC 
Hi Richard, hi all,

Richard Biener wrote:
> Looking at pass_omp_target_link::execute I wonder iff find_link_var_op
> shouldn't simply do the substitution?  Aka

This seems to work ...

> --- a/gcc/omp-offload.cc
> +++ b/gcc/omp-offload.cc
> @@ -2893,6 +2893,7 @@ find_link_var_op (tree *tp, int *walk_subtrees, void *)
>         && is_global_var (t)
>         && lookup_attribute ("omp declare target link", DECL_ATTRIBUTES (t)))
>       {
> +      *tp = unshare_expr (DECL_VALUE_EXPR (t));
>         *walk_subtrees = 0;
>         return t;
>       }
>
> which then makes the stmt obviously not gimple?

... except that 'return t' prevents updating other value-expr in the 
same stmt, but that can be fixed.

Updated patch attached.

Thanks for the suggestion!

Tobias

Comments

Richard Biener July 31, 2024, 12:10 p.m. UTC | #1 
On Wed, Jul 31, 2024 at 1:21 PM Tobias Burnus <tburnus@baylibre.com> wrote:
>
> Hi Richard, hi all,
>
> Richard Biener wrote:
>
> Looking at pass_omp_target_link::execute I wonder iff find_link_var_op
> shouldn't simply do the substitution?  Aka
>
> This seems to work ...
>
> --- a/gcc/omp-offload.cc
> +++ b/gcc/omp-offload.cc
> @@ -2893,6 +2893,7 @@ find_link_var_op (tree *tp, int *walk_subtrees, void *)
>        && is_global_var (t)
>        && lookup_attribute ("omp declare target link", DECL_ATTRIBUTES (t)))
>      {
> +      *tp = unshare_expr (DECL_VALUE_EXPR (t));
>        *walk_subtrees = 0;
>        return t;
>      }
>
> which then makes the stmt obviously not gimple?
>
> ... except that 'return t' prevents updating other value-expr in the same stmt, but that can be fixed.
>
> Updated patch attached.

You can pass a

  walk_stmt_info wi;
  wi->data = NULL;

to walk_gimple_stmt and set wi->data instead of using a global
variable (or make wi->data point
to a local variable for some more indirection).

OK as-is or with cleanup as suggested above.

Thanks,
Richard.

> Thanks for the suggestion!
>
> Tobias
diff mbox series

Patch

omp-offload.cc: Fix value-expr handling of 'declare target link' vars

As the PR and included testcase shows, replacing 'arr2' by its value expression
'*arr2$13$linkptr' failed for
  MEM <uint128_t> [(c_char * {ref-all})&arr2]
which left 'arr2' in the code as unknown symbol. Now expand the value expression
already in pass_omp_target_link::execute's process_link_var_op walk_gimple_stmt
walk - and don't rely on gimple_regimplify_operands.

        PR middle-end/115637

gcc/ChangeLog:

	* gimplify.cc (gimplify_body): Fix macro name in the comment.
	* omp-offload.cc (found_link_var): New global var.
	(find_link_var_op): Rename to ...
	(process_link_var_op): ... this. Replace value expr; set
	found_link_var.
	(pass_omp_target_link::execute): Update walk_gimple_stmt call.

libgomp/ChangeLog:

	* testsuite/libgomp.fortran/declare-target-link.f90: Uncomment
	now working code.

Co-authored-by: Richard Biener <rguenther@suse.de

 gcc/gimplify.cc                                           |  2 +-
 gcc/omp-offload.cc                                        | 12 +++++++++---
 libgomp/testsuite/libgomp.fortran/declare-target-link.f90 | 15 ++++++---------
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index c77a53bdfce..30bfecf67e5 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -19423,7 +19423,7 @@  gimplify_body (tree fndecl, bool do_parms)
   DECL_SAVED_TREE (fndecl) = NULL_TREE;
 
   /* If we had callee-copies statements, insert them at the beginning
-     of the function and clear DECL_VALUE_EXPR_P on the parameters.  */
+     of the function and clear DECL_HAS_VALUE_EXPR_P on the parameters.  */
   if (!gimple_seq_empty_p (parm_stmts))
     {
       tree parm;
diff --git a/gcc/omp-offload.cc b/gcc/omp-offload.cc
index 35313c2ecf3..e9a51bdb76d 100644
--- a/gcc/omp-offload.cc
+++ b/gcc/omp-offload.cc
@@ -2881,10 +2881,12 @@  public:
   unsigned execute (function *) final override;
 };
 
+static bool found_link_var;
+
 /* Callback for walk_gimple_stmt used to scan for link var operands.  */
 
 static tree
-find_link_var_op (tree *tp, int *walk_subtrees, void *)
+process_link_var_op (tree *tp, int *walk_subtrees, void *)
 {
   tree t = *tp;
 
@@ -2893,8 +2895,10 @@  find_link_var_op (tree *tp, int *walk_subtrees, void *)
       && is_global_var (t)
       && lookup_attribute ("omp declare target link", DECL_ATTRIBUTES (t)))
     u{
+      *tp = unshare_expr (DECL_VALUE_EXPR (t));
       *walk_subtrees = 0;
-      return t;
+      found_link_var = true;
+      return NULL_TREE;
     }
 
   return NULL_TREE;
@@ -2924,7 +2928,9 @@  pass_omp_target_link::execute (function *fun)
 	      gimple_call_set_arg (gsi_stmt (gsi), 1, null_pointer_node);
 	      update_stmt (gsi_stmt (gsi));
 	    }
-	  if (walk_gimple_stmt (&gsi, NULL, find_link_var_op, NULL))
+	  found_link_var = false;
+	  walk_gimple_stmt (&gsi, NULL, process_link_var_op, NULL);
+	  if (found_link_var)
 	    gimple_regimplify_operands (gsi_stmt (gsi), &gsi);
 	}
     }
diff --git a/libgomp/testsuite/libgomp.fortran/declare-target-link.f90 b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
index 2ce212d114f..44c67f925bd 100644
--- a/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
+++ b/libgomp/testsuite/libgomp.fortran/declare-target-link.f90
@@ -1,5 +1,7 @@ 
 ! { dg-additional-options "-Wall" }
+
 ! PR fortran/115559
+! PR middle-end/115637
 
 module m
    integer :: A
@@ -73,24 +75,19 @@  contains
     !$omp target map(from:res)
       res = run_device1()
     !$omp end target
-    print *, res
-    ! FIXME: arr2 not link mapped -> PR115637
-    ! if (res /= -11436) stop 5
-    if (res /= -11546) stop 5 ! FIXME
+    ! print *, res
+    if (res /= -11436) stop 5
   end
   integer function run_device1()
     !$omp declare target
     integer :: i
     run_device1 = -99
-    ! FIXME: arr2 not link mapped -> PR115637
-    !   arr2 = [11,22,33,44]
+    arr2 = [11,22,33,44]
     if (any (arr(10:50) /= [(i, i=10,50)])) then
       run_device1 = arr(11)
       return
     end if
-    ! FIXME: -> PR115637
-    ! run_device1 = sum(arr(10:13) + arr2)
-    run_device1 = sum(arr(10:13) ) ! FIXME
+    run_device1 = sum(arr(10:13) + arr2)
     do i = 10, 50
       arr(i) = 3 - 10 * arr(i)
     end do