diff mbox series

gimplify.cc: Handle VALUE_EXPR of MEM_REF's ADDR_EXPR argument [PR115637]

Message ID a7bd8da2-73bf-4733-956a-dc9a146d9368@baylibre.com
State New
Headers show
Series gimplify.cc: Handle VALUE_EXPR of MEM_REF's ADDR_EXPR argument [PR115637] | expand

Commit Message

Tobias Burnus July 29, 2024, 7:25 p.m. UTC 
The problem is code like:

   MEM <uint128_t> [(c_char * {ref-all})&arr2]

where arr2 is the value expr '*arr2$13$linkptr'
(i.e. indirect ref + decl name).

Insidepass_omp_target_link::execute, there is a call to 
gimple_regimplify_operands but the value expression is not 
expanded.There are two problems: ADDR_EXPR is no handling this and while 
MEM_REF has some code for it, it doesn't handle this either. The 
attached code fixes this. Tested on x86_64-gnu-linux with nvidia 
offloading. Comments, remarks, OK? Better suggestions? * * * In 
gimplify_expr for MEM_REF, there is a call to is_gimple_mem_ref_addr which checks for ADD_EXPR
but not for value expressions. The attached match handles
the case explicitly, but, alternatively, we might want
move it to is_gimple_mem_ref_addr (not checked whether it
makes sense or not).

Where is_gimple_mem_ref_addr is defined as:

/* Return true if T is a valid address operand of a MEM_REF.  */

bool
is_gimple_mem_ref_addr (tree t)
{
   return (is_gimple_reg (t)
           || TREE_CODE (t) == INTEGER_CST
           || (TREE_CODE (t) == ADDR_EXPR
               && (CONSTANT_CLASS_P (TREE_OPERAND (t, 0))
                   || decl_address_invariant_p (TREE_OPERAND (t, 0)))));
}

Tobias

Comments

Richard Biener July 30, 2024, 8:59 a.m. UTC | #1 
On Mon, Jul 29, 2024 at 9:26 PM Tobias Burnus <tburnus@baylibre.com> wrote:
>
> The problem is code like:
>
>    MEM <uint128_t> [(c_char * {ref-all})&arr2]
>
> where arr2 is the value expr '*arr2$13$linkptr'
> (i.e. indirect ref + decl name).
>
> Insidepass_omp_target_link::execute, there is a call to
> gimple_regimplify_operands but the value expression is not
> expanded.There are two problems: ADDR_EXPR is no handling this and while
> MEM_REF has some code for it, it doesn't handle this either. The
> attached code fixes this. Tested on x86_64-gnu-linux with nvidia
> offloading. Comments, remarks, OK? Better suggestions? * * * In
> gimplify_expr for MEM_REF, there is a call to is_gimple_mem_ref_addr which checks for ADD_EXPR
> but not for value expressions. The attached match handles
> the case explicitly, but, alternatively, we might want
> move it to is_gimple_mem_ref_addr (not checked whether it
> makes sense or not).
>
> Where is_gimple_mem_ref_addr is defined as:
>
> /* Return true if T is a valid address operand of a MEM_REF.  */
>
> bool
> is_gimple_mem_ref_addr (tree t)
> {
>    return (is_gimple_reg (t)
>            || TREE_CODE (t) == INTEGER_CST
>            || (TREE_CODE (t) == ADDR_EXPR
>                && (CONSTANT_CLASS_P (TREE_OPERAND (t, 0))
>                    || decl_address_invariant_p (TREE_OPERAND (t, 0)))));
> }

I think iff then decl_address_invariant_p should be amended.

Why is the gimplify_addr_expr hunk needed?  It should get
to gimplifying the VAR_DECL/PARM_DECL by recursion?

> Tobias
Tobias Burnus July 30, 2024, 5:33 p.m. UTC | #2 
Richard Biener wrote:
> On Mon, Jul 29, 2024 at 9:26 PM Tobias Burnus<tburnus@baylibre.com>  wrote:
>> Inside pass_omp_target_link::execute, there is a call to
>> gimple_regimplify_operands but the value expression is not
>> expanded.[...]
>>
>> Where is_gimple_mem_ref_addr is defined as:
>>
>> /* Return true if T is a valid address operand of a MEM_REF.  */
>>
>> bool
>> is_gimple_mem_ref_addr (tree t)
>> {
>>     return (is_gimple_reg (t)
>>             || TREE_CODE (t) == INTEGER_CST
>>             || (TREE_CODE (t) == ADDR_EXPR
>>                 && (CONSTANT_CLASS_P (TREE_OPERAND (t, 0))
>>                     || decl_address_invariant_p (TREE_OPERAND (t, 0)))));
>> }
> I think iff then decl_address_invariant_p should be amended.

This does not work - at least not for my use case if OpenMP
link variables - due to ordering issues.

For the device compilers, the VALUE_EXPR is added in lto_main
or in do_whole_program_analysis (same file: lto/lto.cc) by
callingoffload_handle_link_vars. The value expression is then later expanded via pass_omp_target_link::execute, but in between the following happens:

lto_main  callssymbol_table::compile, which then calls
cgraph_node::expand  and that executes

    res |= verify_types_in_gimple_reference (lhs, true); for lhs being: 
MEM <uint128_t> [(c_char * {ref-all})&arr2]
But when adding the has-value-expr check either directly to is_gimple_mem_ref_addr or to the decl_address_invariant_pit calls, the following condition becomes true the called function in 
tree-cfg.cc:

3302          if (!is_gimple_mem_ref_addr (TREE_OPERAND (expr, 0))
3303              || (TREE_CODE (TREE_OPERAND (expr, 0)) == ADDR_EXPR
3304                  && verify_address (TREE_OPERAND (expr, 0), false)))
3305            {
3306              error ("invalid address operand in %qs", code_name);

* * * Thus, I am now back to the previous change, except for:

> Why is the gimplify_addr_expr hunk needed?  It should get
> to gimplifying the VAR_DECL/PARM_DECL by recursion?

Indeed. I wonder why I had (thought to) need it before; possibly
because it was needed or thought to be needed when trying to trace
this down.

Previous patch - except for that bit removed - attached.

Thoughts, better ideas?

Tobias
Richard Biener July 31, 2024, 8:31 a.m. UTC | #3 
On Tue, Jul 30, 2024 at 7:33 PM Tobias Burnus <tburnus@baylibre.com> wrote:
>
> Richard Biener wrote:
> > On Mon, Jul 29, 2024 at 9:26 PM Tobias Burnus<tburnus@baylibre.com>  wrote:
> >> Inside pass_omp_target_link::execute, there is a call to
> >> gimple_regimplify_operands but the value expression is not
> >> expanded.[...]
> >>
> >> Where is_gimple_mem_ref_addr is defined as:
> >>
> >> /* Return true if T is a valid address operand of a MEM_REF.  */
> >>
> >> bool
> >> is_gimple_mem_ref_addr (tree t)
> >> {
> >>     return (is_gimple_reg (t)
> >>             || TREE_CODE (t) == INTEGER_CST
> >>             || (TREE_CODE (t) == ADDR_EXPR
> >>                 && (CONSTANT_CLASS_P (TREE_OPERAND (t, 0))
> >>                     || decl_address_invariant_p (TREE_OPERAND (t, 0)))));
> >> }
> > I think iff then decl_address_invariant_p should be amended.
>
> This does not work - at least not for my use case if OpenMP
> link variables - due to ordering issues.
>
> For the device compilers, the VALUE_EXPR is added in lto_main
> or in do_whole_program_analysis (same file: lto/lto.cc) by
> callingoffload_handle_link_vars. The value expression is then later expanded via pass_omp_target_link::execute, but in between the following happens:
>
> lto_main  callssymbol_table::compile, which then calls
> cgraph_node::expand  and that executes
>
>     res |= verify_types_in_gimple_reference (lhs, true); for lhs being:
> MEM <uint128_t> [(c_char * {ref-all})&arr2]
> But when adding the has-value-expr check either directly to is_gimple_mem_ref_addr or to the decl_address_invariant_pit calls, the following condition becomes true the called function in
> tree-cfg.cc:
>
> 3302          if (!is_gimple_mem_ref_addr (TREE_OPERAND (expr, 0))
> 3303              || (TREE_CODE (TREE_OPERAND (expr, 0)) == ADDR_EXPR
> 3304                  && verify_address (TREE_OPERAND (expr, 0), false)))
> 3305            {
> 3306              error ("invalid address operand in %qs", code_name);
>
> * * * Thus, I am now back to the previous change, except for:
>
> > Why is the gimplify_addr_expr hunk needed?  It should get
> > to gimplifying the VAR_DECL/PARM_DECL by recursion?
>
> Indeed. I wonder why I had (thought to) need it before; possibly
> because it was needed or thought to be needed when trying to trace
> this down.
>
> Previous patch - except for that bit removed - attached.
>
> Thoughts, better ideas?

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

diff --git a/gcc/omp-offload.cc b/gcc/omp-offload.cc
index 35313c2ecf3..cf9e5b715ab 100644
--- 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?

>
> Tobias
diff mbox series

Patch

gimplify.cc: Handle VALUE_EXPR of MEM_REF's ADDR_EXPR argument [PR115637]

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.

	PR middle-end/115637

gcc/ChangeLog:

	* gimplify.cc (gimplify_addr_expr): Handle value-expr arg.
	(gimplify_expr): For MEM_REF and an ADDR_EXPR, also check
	for value-expr arguments.
	(gimplify_body): Fix macro name in the comment.

libgomp/ChangeLog:

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

 gcc/gimplify.cc                                          | 16 ++++++++++++++--
 .../testsuite/libgomp.fortran/declare-target-link.f90    | 15 ++++++---------
 2 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/gcc/gimplify.cc b/gcc/gimplify.cc
index ab323d764e8..d548dc2cdf6 100644
--- a/gcc/gimplify.cc
+++ b/gcc/gimplify.cc
@@ -6888,6 +6888,13 @@  gimplify_addr_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p)
   enum gimplify_status ret;
   location_t loc = EXPR_LOCATION (*expr_p);
 
+  if (VAR_P (op0) || TREE_CODE (op0) == PARM_DECL)
+    {
+      ret = gimplify_var_or_parm_decl (&TREE_OPERAND (expr, 0));
+      if (ret == GS_ERROR)
+	return ret;
+      op0 = TREE_OPERAND (expr, 0);
+    }
   switch (TREE_CODE (op0))
     {
     case INDIRECT_REF:
@@ -18251,8 +18258,13 @@  gimplify_expr (tree *expr_p, gimple_seq *pre_p, gimple_seq *post_p,
 	     in suitable form.  Re-gimplifying would mark the address
 	     operand addressable.  Always gimplify when not in SSA form
 	     as we still may have to gimplify decls with value-exprs.  */
+	  tmp = TREE_OPERAND (*expr_p, 0);
 	  if (!gimplify_ctxp || !gimple_in_ssa_p (cfun)
-	      || !is_gimple_mem_ref_addr (TREE_OPERAND (*expr_p, 0)))
+	      || (!is_gimple_mem_ref_addr (tmp)
+		  || (TREE_CODE (tmp) == ADDR_EXPR
+		      && (VAR_P (TREE_OPERAND (tmp, 0))
+			  || TREE_CODE (TREE_OPERAND (tmp, 0)) == PARM_DECL)
+		      && DECL_HAS_VALUE_EXPR_P (TREE_OPERAND (tmp, 0)))))
 	    {
 	      ret = gimplify_expr (&TREE_OPERAND (*expr_p, 0), pre_p, post_p,
 				   is_gimple_mem_ref_addr, fb_rvalue);
@@ -19422,7 +19434,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/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