Mark variables addressable if they are copied using libcall in RTL expander

Mark variables addressable if they are copied using libcall in RTL expander

[Easwaran Raman]

This fixes bugs introduced by r175063. OK for trunk if there are no
test regressions?

-Easwaran


2011-06-20  Easwaran Raman  <eraman@google.com>

        PR rtl-optimization/49429
        * expr.c (emit_block_move_hints):  Mark MEM_EXPR(x) and
        MEM_EXPR(y) addressable if emit_block_move_via_libcall is
        used to copy y into x.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c  (revision 175081)
+++ gcc/expr.c  (working copy)
@@ -1181,8 +1181,19 @@ emit_block_move_hints (rtx x, rtx y, rtx size, enu
   else if (may_use_call
           && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (x))
           && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (y)))
-    retval = emit_block_move_via_libcall (x, y, size,
-                                         method == BLOCK_OP_TAILCALL);
+    {
+      /* Since x and y are passed to a libcall, mark the corresponding
+         tree EXPR as addressable.  */
+      tree y_expr = MEM_EXPR (y);
+      tree x_expr = MEM_EXPR (x);
+      if (y_expr)
+        mark_addressable (y_expr);
+      if (x_expr)
+        mark_addressable (x_expr);
+      retval = emit_block_move_via_libcall (x, y, size,
+                                           method == BLOCK_OP_TAILCALL);
+    }
+
   else
     emit_block_move_via_loop (x, y, size, align);

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Richard Guenther]

On Tue, Jun 21, 2011 at 3:38 AM, Easwaran Raman <eraman@google.com> wrote:
> This fixes bugs introduced by r175063. OK for trunk if there are no
> test regressions?

I fear this isn't enough considering pass-by-value aggregates that
are callee copied.  And I guess there are other cases.  Eric, what
do you suggest here?

Thanks,
Richard.

> -Easwaran
>
>
> 2011-06-20  Easwaran Raman  <eraman@google.com>
>
>        PR rtl-optimization/49429
>        * expr.c (emit_block_move_hints):  Mark MEM_EXPR(x) and
>        MEM_EXPR(y) addressable if emit_block_move_via_libcall is
>        used to copy y into x.
>
> Index: gcc/expr.c
> ===================================================================
> --- gcc/expr.c  (revision 175081)
> +++ gcc/expr.c  (working copy)
> @@ -1181,8 +1181,19 @@ emit_block_move_hints (rtx x, rtx y, rtx size, enu
>   else if (may_use_call
>           && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (x))
>           && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (y)))
> -    retval = emit_block_move_via_libcall (x, y, size,
> -                                         method == BLOCK_OP_TAILCALL);
> +    {
> +      /* Since x and y are passed to a libcall, mark the corresponding
> +         tree EXPR as addressable.  */
> +      tree y_expr = MEM_EXPR (y);
> +      tree x_expr = MEM_EXPR (x);
> +      if (y_expr)
> +        mark_addressable (y_expr);
> +      if (x_expr)
> +        mark_addressable (x_expr);
> +      retval = emit_block_move_via_libcall (x, y, size,
> +                                           method == BLOCK_OP_TAILCALL);
> +    }
> +
>   else
>     emit_block_move_via_loop (x, y, size, align);
>

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Eric Botcazou]

> I fear this isn't enough considering pass-by-value aggregates that
> are callee copied.  And I guess there are other cases.  Eric, what
> do you suggest here?

I agree that there are probably other cases, but this seems to be the best way 
out for now.  There is still similar code in the expander for asm operands 
(dominated by the same code in the gimplifier) so calling mark_addressable 
this late is presumably acceptable.

-- 
Eric Botcazou

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Jakub Jelinek]

On Wed, Jun 22, 2011 at 03:08:21PM +0200, Eric Botcazou wrote:
> > I fear this isn't enough considering pass-by-value aggregates that
> > are callee copied.  And I guess there are other cases.  Eric, what
> > do you suggest here?
> 
> I agree that there are probably other cases, but this seems to be the best way 
> out for now.  There is still similar code in the expander for asm operands 
> (dominated by the same code in the gimplifier) so calling mark_addressable 
> this late is presumably acceptable.

Won't that cause problems later on during expansion of the same function?
The decl won't suddenly be a gimple reg anymore.  I mean, shouldn't the
marking be postponed until expansion finishes?

	Jakub

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Richard Guenther]

On Wed, Jun 22, 2011 at 3:14 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Jun 22, 2011 at 03:08:21PM +0200, Eric Botcazou wrote:
>> > I fear this isn't enough considering pass-by-value aggregates that
>> > are callee copied.  And I guess there are other cases.  Eric, what
>> > do you suggest here?
>>
>> I agree that there are probably other cases, but this seems to be the best way
>> out for now.  There is still similar code in the expander for asm operands
>> (dominated by the same code in the gimplifier) so calling mark_addressable
>> this late is presumably acceptable.
>
> Won't that cause problems later on during expansion of the same function?
> The decl won't suddenly be a gimple reg anymore.  I mean, shouldn't the
> marking be postponed until expansion finishes?

It's not a register anyway as we've seen a MEM_EXPR with it.

Richard.

>        Jakub
>

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Eric Botcazou]

> I fear this isn't enough considering pass-by-value aggregates that
> are callee copied.

It's indeed not sufficient for arguments passed by reference but callee-copied.

This is PR target/49454.  For gcc.c-torture/execute/20000717-1.c:

typedef struct trio { int a, b, c; } trio;

int
foo (trio t, int i)
{
  return bar (i, t);
}

yiedls in the .optimized dump:

foo (struct trio t, int i)
{
  int D.1968;
  struct trio t.0;

<bb 2>:
  t.0 = t;
  D.1968_2 = bar (i_1(D), t.0);
  return D.1968_2;
}

and the aggregate copy is elided by DSE because t.0 isn't may_be_aliased.  This 
seems to be a pre-existing bug though: its address is passed to bar in RTL.

-- 
Eric Botcazou

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Easwaran Raman]

On Wed, Jun 22, 2011 at 7:13 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> I fear this isn't enough considering pass-by-value aggregates that
>> are callee copied.
>
> It's indeed not sufficient for arguments passed by reference but callee-copied.
>
> This is PR target/49454.  For gcc.c-torture/execute/20000717-1.c:
>
> typedef struct trio { int a, b, c; } trio;
>
> int
> foo (trio t, int i)
> {
>  return bar (i, t);
> }
>
> yiedls in the .optimized dump:
>
> foo (struct trio t, int i)
> {
>  int D.1968;
>  struct trio t.0;
>
> <bb 2>:
>  t.0 = t;
>  D.1968_2 = bar (i_1(D), t.0);
>  return D.1968_2;
> }
>
> and the aggregate copy is elided by DSE because t.0 isn't may_be_aliased.  This
> seems to be a pre-existing bug though: its address is passed to bar in RTL.
>
> --
> Eric Botcazou
>

Is the following patch a reasonable fix for this case?  I assume I
should add similar code inside emit_library_call_value_1.

-Easwaran


--- gcc/calls.c	(revision 175081)
+++ gcc/calls.c	(working copy)
@@ -1073,6 +1073,8 @@ initialize_argument_information (int num_actuals A
 	  callee_copies
 	    = reference_callee_copied (args_so_far, TYPE_MODE (type),
 				       type, argpos < n_named_args);
+          if (callee_copies)
+            mark_addressable (args[i].tree_value);

 	  /* If we're compiling a thunk, pass through invisible references
 	     instead of making a copy.  */

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Eric Botcazou]

> Is the following patch a reasonable fix for this case?

The lines should be moved to within the first branch of the subsequent "if". 
They aren't needed if the second branch is taken because, in this case, we're 
back to the usual caller-copied scheme where we pass the address of the copy.

> I assume I should add similar code inside emit_library_call_value_1.

Yes, we need the same treatment for 'val' in the MEM_P (val) && !must_copy case 
as the one applied in emit_block_move_hints.


But these problems show that there is a slight discrepancy between what dse.c 
really needs (is the address of the variable taken?) and what may_be_aliased 
answers (might the variable be indirectly modified?).  Another viewpoint is to 
say that there is a slight discrepancy between Tree and RTL level when it comes 
to the address-taken property.  Not clear what to do about it so I think that 
we should try this kludgy way and see how it fares.

-- 
Eric Botcazou

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Richard Guenther]

On Thu, Jun 23, 2011 at 10:00 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Is the following patch a reasonable fix for this case?
>
> The lines should be moved to within the first branch of the subsequent "if".
> They aren't needed if the second branch is taken because, in this case, we're
> back to the usual caller-copied scheme where we pass the address of the copy.
>
>> I assume I should add similar code inside emit_library_call_value_1.
>
> Yes, we need the same treatment for 'val' in the MEM_P (val) && !must_copy case
> as the one applied in emit_block_move_hints.
>
>
> But these problems show that there is a slight discrepancy between what dse.c
> really needs (is the address of the variable taken?) and what may_be_aliased
> answers (might the variable be indirectly modified?).  Another viewpoint is to
> say that there is a slight discrepancy between Tree and RTL level when it comes
> to the address-taken property.  Not clear what to do about it so I think that
> we should try this kludgy way and see how it fares.

Yeah, I agree.  Unless we want to really do alias analysis on RTL (and not just
export what the tree level has in some way) let's go forward with this.

So, what's the patch(es) that need approval now?

Thanks,
Richard.

> --
> Eric Botcazou
>

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Eric Botcazou]

> So, what's the patch(es) that need approval now?

Original expr.c patch for PR rtl-optimization/49429 + adjusted and augmented 
calls.c patch for PR target/49454.  Everything is in this thread.

Easwaran, would you mind posting a consolidated patch?

-- 
Eric Botcazou

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Easwaran Raman]

On Thu, Jun 23, 2011 at 3:22 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> So, what's the patch(es) that need approval now?
>
> Original expr.c patch for PR rtl-optimization/49429 + adjusted and augmented
> calls.c patch for PR target/49454.  Everything is in this thread.
>
> Easwaran, would you mind posting a consolidated patch?
>
> --
> Eric Botcazou
>
Here is the revised patch. Bootstraps and all tests pass on
x86_64-unknown-linux. OK for trunk?


2011-06-23  Easwaran Raman  <eraman@google.com>

       PR rtl-optimization/49429
       PR target/49454
       * expr.c (emit_block_move_hints):  Mark MEM_EXPR(x) and
       MEM_EXPR(y) addressable if emit_block_move_via_libcall is
       used to copy y into x.
       * calls.c (initialize_argument_information): Mark
       an argument addressable if it is passed by invisible reference.
       (emit_library_call_value_1): Mark  MEM_EXPR (val) addressable
       if it is passed by reference.

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 175346)
+++ gcc/expr.c	(working copy)
@@ -1181,8 +1181,19 @@ emit_block_move_hints (rtx x, rtx y, rtx size, enu
   else if (may_use_call
 	   && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (x))
 	   && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (y)))
-    retval = emit_block_move_via_libcall (x, y, size,
-					  method == BLOCK_OP_TAILCALL);
+    {
+      /* Since x and y are passed to a libcall, mark the corresponding
+         tree EXPR as addressable.  */
+      tree y_expr = MEM_EXPR (y);
+      tree x_expr = MEM_EXPR (x);
+      if (y_expr)
+        mark_addressable (y_expr);
+      if (x_expr)
+        mark_addressable (x_expr);
+      retval = emit_block_move_via_libcall (x, y, size,
+					    method == BLOCK_OP_TAILCALL);
+    }
+
   else
     emit_block_move_via_loop (x, y, size, align);

Index: gcc/calls.c
===================================================================
--- gcc/calls.c	(revision 175346)
+++ gcc/calls.c	(working copy)
@@ -1084,6 +1084,8 @@ initialize_argument_information (int num_actuals A
 		  && TREE_CODE (base) != SSA_NAME
 		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
 	    {
+              mark_addressable (args[i].tree_value);
+
 	      /* We can't use sibcalls if a callee-copied argument is
 		 stored in the current function's frame.  */
 	      if (!call_from_thunk_p && DECL_P (base) && !TREE_STATIC (base))
@@ -3524,7 +3526,12 @@ emit_library_call_value_1 (int retval, rtx orgfun,
 	    }

 	  if (MEM_P (val) && !must_copy)
-	    slot = val;
+            {
+              tree val_expr = MEM_EXPR (val);
+              if (val_expr)
+                mark_addressable (val_expr);
+	      slot = val;
+            }
 	  else
 	    {
 	      slot = assign_temp (lang_hooks.types.type_for_mode (mode, 0),

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Jakub Jelinek]

On Thu, Jun 23, 2011 at 12:02:35PM -0700, Easwaran Raman wrote:
> +      if (y_expr)
> +        mark_addressable (y_expr);

Please watch formatting, a tab should be used instead of 8 spaces.

> +      if (x_expr)
> +        mark_addressable (x_expr);

Ditto.

> @@ -1084,6 +1084,8 @@ initialize_argument_information (int num_actuals A
>  		  && TREE_CODE (base) != SSA_NAME
>  		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>  	    {
> +              mark_addressable (args[i].tree_value);
> +

Likewise, plus the line is indented too much, each level should be indented
by 2 chars.

	Jakub

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Easwaran Raman]

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

On Thu, Jun 23, 2011 at 12:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> On Thu, Jun 23, 2011 at 12:02:35PM -0700, Easwaran Raman wrote:
>> +      if (y_expr)
>> +        mark_addressable (y_expr);
>
> Please watch formatting, a tab should be used instead of 8 spaces.
>
>> +      if (x_expr)
>> +        mark_addressable (x_expr);
>
> Ditto.
>
>> @@ -1084,6 +1084,8 @@ initialize_argument_information (int num_actuals A
>>                 && TREE_CODE (base) != SSA_NAME
>>                 && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>>           {
>> +              mark_addressable (args[i].tree_value);
>> +
>
> Likewise, plus the line is indented too much, each level should be indented
> by 2 chars.
>
>        Jakub
>

I have attached a new patch that fixes the formatting  issues.

Thanks,
Easwaran

[-- Attachment #2: dse_fix.patch --]
[-- Type: text/x-patch, Size: 1798 bytes --]

Index: gcc/expr.c
===================================================================
--- gcc/expr.c	(revision 175346)
+++ gcc/expr.c	(working copy)
@@ -1181,8 +1181,19 @@ emit_block_move_hints (rtx x, rtx y, rtx size, enu
   else if (may_use_call
 	   && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (x))
 	   && ADDR_SPACE_GENERIC_P (MEM_ADDR_SPACE (y)))
-    retval = emit_block_move_via_libcall (x, y, size,
-					  method == BLOCK_OP_TAILCALL);
+    {
+      /* Since x and y are passed to a libcall, mark the corresponding
+	 tree EXPR as addressable.  */
+      tree y_expr = MEM_EXPR (y);
+      tree x_expr = MEM_EXPR (x);
+      if (y_expr)
+	mark_addressable (y_expr);
+      if (x_expr)
+	mark_addressable (x_expr);
+      retval = emit_block_move_via_libcall (x, y, size,
+					    method == BLOCK_OP_TAILCALL);
+    }
+
   else
     emit_block_move_via_loop (x, y, size, align);
 
Index: gcc/calls.c
===================================================================
--- gcc/calls.c	(revision 175346)
+++ gcc/calls.c	(working copy)
@@ -1084,6 +1084,8 @@ initialize_argument_information (int num_actuals A
 		  && TREE_CODE (base) != SSA_NAME
 		  && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
 	    {
+	      mark_addressable (args[i].tree_value);
+
 	      /* We can't use sibcalls if a callee-copied argument is
 		 stored in the current function's frame.  */
 	      if (!call_from_thunk_p && DECL_P (base) && !TREE_STATIC (base))
@@ -3524,7 +3526,12 @@ emit_library_call_value_1 (int retval, rtx orgfun,
 	    }
 
 	  if (MEM_P (val) && !must_copy)
-	    slot = val;
+	    {
+	      tree val_expr = MEM_EXPR (val);
+	      if (val_expr)
+		mark_addressable (val_expr);
+	      slot = val;
+	    }
 	  else
 	    {
 	      slot = assign_temp (lang_hooks.types.type_for_mode (mode, 0),

Re: Mark variables addressable if they are copied using libcall in RTL expander

[Richard Guenther]

On Thu, Jun 23, 2011 at 11:47 PM, Easwaran Raman <eraman@google.com> wrote:
> On Thu, Jun 23, 2011 at 12:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> On Thu, Jun 23, 2011 at 12:02:35PM -0700, Easwaran Raman wrote:
>>> +      if (y_expr)
>>> +        mark_addressable (y_expr);
>>
>> Please watch formatting, a tab should be used instead of 8 spaces.
>>
>>> +      if (x_expr)
>>> +        mark_addressable (x_expr);
>>
>> Ditto.
>>
>>> @@ -1084,6 +1084,8 @@ initialize_argument_information (int num_actuals A
>>>                 && TREE_CODE (base) != SSA_NAME
>>>                 && (!DECL_P (base) || MEM_P (DECL_RTL (base)))))
>>>           {
>>> +              mark_addressable (args[i].tree_value);
>>> +
>>
>> Likewise, plus the line is indented too much, each level should be indented
>> by 2 chars.
>>
>>        Jakub
>>
>
> I have attached a new patch that fixes the formatting  issues.

Ok.

Thanks,
Richard.

> Thanks,
> Easwaran
>