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

login
register
mail settings
Submitter Easwaran Raman
Date June 21, 2011, 1:38 a.m.
Message ID <BANLkTi=gb-G47LAP8uQkiXqLz=dJE047Oy6-N3F8Dfef_NU=6Q@mail.gmail.com>
Download mbox | patch
Permalink /patch/101230/
State New
Headers show

Comments

Easwaran Raman - June 21, 2011, 1:38 a.m.
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.
Richard Guenther - June 21, 2011, 8:45 a.m.
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);
>
Eric Botcazou - June 22, 2011, 1:08 p.m.
> 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.
Jakub Jelinek - June 22, 2011, 1:14 p.m.
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
Richard Guenther - June 22, 2011, 1:19 p.m.
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
>
Eric Botcazou - June 22, 2011, 2:13 p.m.
> 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.

Patch

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);