[PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Jakub Jelinek]

Hi!

Paolo asked me to mail this version of the patch, more variants are
discussed in the PR.
Bootstrapped/regtested on x86_64-linux and regtested on ppc-linux
(on the latter with
-FAIL: noclass execution - gij test
-FAIL: pr11951 execution - gij test
-FAIL: throwit execution - gij test
-FAIL: pr29812 execution - gij test
-FAIL: ExtraClassLoader execution - source compiled test
-FAIL: ExtraClassLoader -findirect-dispatch execution - source compiled test
-FAIL: ExtraClassLoader -O3 execution - source compiled test
-FAIL: ExtraClassLoader -O3 -findirect-dispatch execution - source compiled test
effect).  Ok for trunk?

2007-08-29  Jakub Jelinek  <jakub@redhat.com>

	PR middle-end/32758
	* dce.c (dce_process_block): Don't delete setters of
	artificially used registers.

	* gcc.dg/cleanup-12.c: New test.

--- gcc/dce.c.jj	2007-08-13 15:11:18.000000000 +0200
+++ gcc/dce.c	2007-08-29 16:34:00.000000000 +0200
@@ -527,6 +527,7 @@ static bool
 dce_process_block (basic_block bb, bool redo_out)
 {
   bitmap local_live = BITMAP_ALLOC (&dce_tmp_bitmap_obstack);
+  bitmap au;
   rtx insn;
   bool block_changed;
   struct df_ref **def_rec, **use_rec;
@@ -569,6 +570,15 @@ dce_process_block (basic_block bb, bool 
 	bitmap_set_bit (local_live, DF_REF_REGNO (use));
     }
 
+  /* These regs are considered always live so if they end up dying
+     because of some def, we need to bring the back again.
+     Calling df_simulate_fixup_sets has the disadvantage of calling
+     df_has_eh_preds once per insn, so we cache the information here.  */
+  if (df_has_eh_preds (bb))
+    au = df->eh_block_artificial_uses;
+  else
+    au = df->regular_block_artificial_uses;
+
   FOR_BB_INSNS_REVERSE (bb, insn)
     if (INSN_P (insn))
       {
@@ -580,7 +590,8 @@ dce_process_block (basic_block bb, bool 
 
 	    /* The insn is needed if there is someone who uses the output.  */
 	    for (def_rec = DF_INSN_DEFS (insn); *def_rec; def_rec++)
-	      if (bitmap_bit_p (local_live, DF_REF_REGNO (*def_rec)))
+	      if (bitmap_bit_p (local_live, DF_REF_REGNO (*def_rec))
+		  || bitmap_bit_p (au, DF_REF_REGNO (*def_rec)))
 		{
 		  needed = true;
 		  break;
--- gcc/testsuite/gcc.dg/cleanup-12.c.jj	2007-08-29 13:48:19.000000000 +0200
+++ gcc/testsuite/gcc.dg/cleanup-12.c	2007-08-29 13:48:19.000000000 +0200
@@ -0,0 +1,69 @@
+/* PR middle-end/32758 */
+/* HP-UX libunwind.so doesn't provide _UA_END_OF_STACK */
+/* { dg-do run } */
+/* { dg-options "-O2 -fexceptions" } */
+/* { dg-skip-if "" { "ia64-*-hpux11.*" }  { "*" } { "" } } */
+/* Verify unwind info in presence of alloca.  */
+
+#include <unwind.h>
+#include <stdlib.h>
+#include <string.h>
+
+static _Unwind_Reason_Code
+force_unwind_stop (int version, _Unwind_Action actions,
+		   _Unwind_Exception_Class exc_class,
+		   struct _Unwind_Exception *exc_obj,
+		   struct _Unwind_Context *context,
+		   void *stop_parameter)
+{
+  if (actions & _UA_END_OF_STACK)
+    abort ();
+  return _URC_NO_REASON;
+}
+
+static void force_unwind (void)
+{
+  struct _Unwind_Exception *exc = malloc (sizeof (*exc));
+  memset (&exc->exception_class, 0, sizeof (exc->exception_class));
+  exc->exception_cleanup = 0;
+
+#ifndef __USING_SJLJ_EXCEPTIONS__
+  _Unwind_ForcedUnwind (exc, force_unwind_stop, 0);
+#else
+  _Unwind_SjLj_ForcedUnwind (exc, force_unwind_stop, 0);
+#endif
+
+  abort ();
+}
+
+__attribute__((noinline))
+void foo (void *x __attribute__((unused)))
+{
+  force_unwind ();
+}
+
+__attribute__((noinline))
+int bar (unsigned int x)
+{
+  void *y = __builtin_alloca (x);
+  foo (y);
+  return 1;
+}
+
+static void handler (void *p __attribute__((unused)))
+{
+  exit (0);
+}
+
+__attribute__((noinline))
+static void doit ()
+{
+  char dummy __attribute__((cleanup (handler)));
+  bar (1024);
+}
+
+int main ()
+{
+  doit ();
+  abort ();
+}

	Jakub

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Seongbae Park (박성배, 朴成培)]

Hi Jakub,

Can you show me an example of the bad DCE your change prevents ?
On the face of it, the change looks wrong,
as the artificial uses for a block aren't meant to be alive everywhere.

Beside, it's a per-block set, so the patch as it is
(even if making them live everywhere is what we want to do)
is fragile - it should really use df_get_artificial_uses(bb_index)
to initialize a temporary bitmap.

Seongbae

On 8/29/07, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> Paolo asked me to mail this version of the patch, more variants are
> discussed in the PR.
> Bootstrapped/regtested on x86_64-linux and regtested on ppc-linux
> (on the latter with
> -FAIL: noclass execution - gij test
> -FAIL: pr11951 execution - gij test
> -FAIL: throwit execution - gij test
> -FAIL: pr29812 execution - gij test
> -FAIL: ExtraClassLoader execution - source compiled test
> -FAIL: ExtraClassLoader -findirect-dispatch execution - source compiled test
> -FAIL: ExtraClassLoader -O3 execution - source compiled test
> -FAIL: ExtraClassLoader -O3 -findirect-dispatch execution - source compiled test
> effect).  Ok for trunk?
>
> 2007-08-29  Jakub Jelinek  <jakub@redhat.com>
>
>         PR middle-end/32758
>         * dce.c (dce_process_block): Don't delete setters of
>         artificially used registers.
>
>         * gcc.dg/cleanup-12.c: New test.
>
> --- gcc/dce.c.jj        2007-08-13 15:11:18.000000000 +0200
> +++ gcc/dce.c   2007-08-29 16:34:00.000000000 +0200
> @@ -527,6 +527,7 @@ static bool
>  dce_process_block (basic_block bb, bool redo_out)
>  {
>    bitmap local_live = BITMAP_ALLOC (&dce_tmp_bitmap_obstack);
> +  bitmap au;
>    rtx insn;
>    bool block_changed;
>    struct df_ref **def_rec, **use_rec;
> @@ -569,6 +570,15 @@ dce_process_block (basic_block bb, bool
>         bitmap_set_bit (local_live, DF_REF_REGNO (use));
>      }
>
> +  /* These regs are considered always live so if they end up dying
> +     because of some def, we need to bring the back again.
> +     Calling df_simulate_fixup_sets has the disadvantage of calling
> +     df_has_eh_preds once per insn, so we cache the information here.  */
> +  if (df_has_eh_preds (bb))
> +    au = df->eh_block_artificial_uses;
> +  else
> +    au = df->regular_block_artificial_uses;
> +
>    FOR_BB_INSNS_REVERSE (bb, insn)
>      if (INSN_P (insn))
>        {
> @@ -580,7 +590,8 @@ dce_process_block (basic_block bb, bool
>
>             /* The insn is needed if there is someone who uses the output.  */
>             for (def_rec = DF_INSN_DEFS (insn); *def_rec; def_rec++)
> -             if (bitmap_bit_p (local_live, DF_REF_REGNO (*def_rec)))
> +             if (bitmap_bit_p (local_live, DF_REF_REGNO (*def_rec))
> +                 || bitmap_bit_p (au, DF_REF_REGNO (*def_rec)))
>                 {
>                   needed = true;
>                   break;
> --- gcc/testsuite/gcc.dg/cleanup-12.c.jj        2007-08-29 13:48:19.000000000 +0200
> +++ gcc/testsuite/gcc.dg/cleanup-12.c   2007-08-29 13:48:19.000000000 +0200
> @@ -0,0 +1,69 @@
> +/* PR middle-end/32758 */
> +/* HP-UX libunwind.so doesn't provide _UA_END_OF_STACK */
> +/* { dg-do run } */
> +/* { dg-options "-O2 -fexceptions" } */
> +/* { dg-skip-if "" { "ia64-*-hpux11.*" }  { "*" } { "" } } */
> +/* Verify unwind info in presence of alloca.  */
> +
> +#include <unwind.h>
> +#include <stdlib.h>
> +#include <string.h>
> +
> +static _Unwind_Reason_Code
> +force_unwind_stop (int version, _Unwind_Action actions,
> +                  _Unwind_Exception_Class exc_class,
> +                  struct _Unwind_Exception *exc_obj,
> +                  struct _Unwind_Context *context,
> +                  void *stop_parameter)
> +{
> +  if (actions & _UA_END_OF_STACK)
> +    abort ();
> +  return _URC_NO_REASON;
> +}
> +
> +static void force_unwind (void)
> +{
> +  struct _Unwind_Exception *exc = malloc (sizeof (*exc));
> +  memset (&exc->exception_class, 0, sizeof (exc->exception_class));
> +  exc->exception_cleanup = 0;
> +
> +#ifndef __USING_SJLJ_EXCEPTIONS__
> +  _Unwind_ForcedUnwind (exc, force_unwind_stop, 0);
> +#else
> +  _Unwind_SjLj_ForcedUnwind (exc, force_unwind_stop, 0);
> +#endif
> +
> +  abort ();
> +}
> +
> +__attribute__((noinline))
> +void foo (void *x __attribute__((unused)))
> +{
> +  force_unwind ();
> +}
> +
> +__attribute__((noinline))
> +int bar (unsigned int x)
> +{
> +  void *y = __builtin_alloca (x);
> +  foo (y);
> +  return 1;
> +}
> +
> +static void handler (void *p __attribute__((unused)))
> +{
> +  exit (0);
> +}
> +
> +__attribute__((noinline))
> +static void doit ()
> +{
> +  char dummy __attribute__((cleanup (handler)));
> +  bar (1024);
> +}
> +
> +int main ()
> +{
> +  doit ();
> +  abort ();
> +}
>
>         Jakub
-- 
#pragma ident "Seongbae Park, compiler, http://seongbae.blogspot.com"

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Jakub Jelinek]

On Wed, Aug 29, 2007 at 11:19:25AM -0700, Seongbae Park wrote:
> Can you show me an example of the bad DCE your change prevents ?

Bugzilla has it, but I can repeat it.

void foo (void *x __attribute__((unused)));

__attribute__((noinline))
int bar (unsigned int x)
{
  void *y = __builtin_alloca (x);
  foo (y);
  return 1;
}

with -O2 -fexceptions with the patch generates:

.bar:
.LFB2:
        mflr 0
.LCFI0:
        std 31,-8(1)
.LCFI1:
        addi 3,3,30
        std 0,16(1)
.LCFI2:
        stdu 1,-128(1)
.LCFI3:
        rldicr 3,3,31,32
        rldicr 3,3,33,59
        neg 3,3
        ld 0,0(1)
        mr 31,1			# <----- this one
.LCFI4:				# <----- and this label
        stdux 0,1,3
        addi 3,1,112
        bl .foo
        nop
        ld 1,0(1)
        li 3,1
        ld 0,16(1)
        ld 31,-8(1)
        mtlr 0
        blr
        .long 0
        .byte 0,0,0,1,128,1,0,0
.LFE2:

while without the patch removes the above marked mr 31,1
instruction.  Register 31 is on ppc hard frame pointer, this
function uses alloca and subtracts non-constant amount from
the stack pointer (register 1).  Eventhough the hard frame
pointer isn't visibly used anywhere (except instructions
which save resp. restore it as it is a call saved register),
it is used by the EH, so with -fexceptions it must be live
and have proper value at least on all call instructions
where the stack is decreased by alloca (bl .foo in this case)
and e.g. with -fasynchronous-unwind-tables on every single
instruction when the stack is decreased. 
To make debugging possible it is always needed on all such
instructions, otherwise the debugger can't find out the
size of the stack frame.

> On the face of it, the change looks wrong,
> as the artificial uses for a block aren't meant to be alive everywhere.

Nope, it has to be alive everywhere, at least on the stdux 0,1,3
through ld 1,0(1) (the former decreases stack by reg3 bytes,
the latter loads a value from the stack the former insn saved)
instructions.

> Beside, it's a per-block set, so the patch as it is
> (even if making them live everywhere is what we want to do)
> is fragile - it should really use df_get_artificial_uses(bb_index)
> to initialize a temporary bitmap.

So why df_simulate_fixup_sets uses it directly then?
df_get_artificial_uses(bb_index) contains also other regs
that don't have these properties like hfp, fp, ap
- EH_USES, EH_RETURN_DATA_REGNO.

	Jakub

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Seongbae Park (박성배, 朴成培)]

On 8/29/07, Jakub Jelinek <jakub@redhat.com> wrote:
> On Wed, Aug 29, 2007 at 11:19:25AM -0700, Seongbae Park wrote:
> > Can you show me an example of the bad DCE your change prevents ?
>
> Bugzilla has it, but I can repeat it.

Thanks. Sorry I didn't notice the PR number in the subject or ChangeLog :(

> void foo (void *x __attribute__((unused)));
>
> __attribute__((noinline))
> int bar (unsigned int x)
> {
>   void *y = __builtin_alloca (x);
>   foo (y);
>   return 1;
> }
>
> with -O2 -fexceptions with the patch generates:
>
> .bar:
> .LFB2:
>         mflr 0
> .LCFI0:
>         std 31,-8(1)
> .LCFI1:
>         addi 3,3,30
>         std 0,16(1)
> .LCFI2:
>         stdu 1,-128(1)
> .LCFI3:
>         rldicr 3,3,31,32
>         rldicr 3,3,33,59
>         neg 3,3
>         ld 0,0(1)
>         mr 31,1                 # <----- this one
> .LCFI4:                         # <----- and this label
>         stdux 0,1,3
>         addi 3,1,112
>         bl .foo
>         nop
>         ld 1,0(1)
>         li 3,1
>         ld 0,16(1)
>         ld 31,-8(1)
>         mtlr 0
>         blr
>         .long 0
>         .byte 0,0,0,1,128,1,0,0
> .LFE2:
>
> while without the patch removes the above marked mr 31,1
> instruction.  Register 31 is on ppc hard frame pointer, this
> function uses alloca and subtracts non-constant amount from
> the stack pointer (register 1).  Eventhough the hard frame
> pointer isn't visibly used anywhere (except instructions
> which save resp. restore it as it is a call saved register),
> it is used by the EH, so with -fexceptions it must be live
> and have proper value at least on all call instructions
> where the stack is decreased by alloca (bl .foo in this case)
> and e.g. with -fasynchronous-unwind-tables on every single
> instruction when the stack is decreased.
> To make debugging possible it is always needed on all such
> instructions, otherwise the debugger can't find out the
> size of the stack frame.

> > On the face of it, the change looks wrong,
> > as the artificial uses for a block aren't meant to be alive everywhere.
>
> Nope, it has to be alive everywhere, at least on the stdux 0,1,3
> through ld 1,0(1) (the former decreases stack by reg3 bytes,
> the latter loads a value from the stack the former insn saved)
> instructions.
>
> > Beside, it's a per-block set, so the patch as it is
> > (even if making them live everywhere is what we want to do)
> > is fragile - it should really use df_get_artificial_uses(bb_index)
> > to initialize a temporary bitmap.
>
> So why df_simulate_fixup_sets uses it directly then?
> df_get_artificial_uses(bb_index) contains also other regs
> that don't have these properties like hfp, fp, ap
> - EH_USES, EH_RETURN_DATA_REGNO.
>
>         Jakub

Kenny explained your patch to me, and I just read the PR.
I think this patch is ok.

We have lots of cleanup to do regarding artificial uses
(namely, getting rid of it completely)
but that's df maintainer's job to do
(of course, anyone is welcome to clean this part up)
and given what we have now, the patch is fine.

There are other bugs (e.g. ud-chain based dce
would happily delete the same insn) but I'll clean those up later.

Thanks for the fix!
-- 
#pragma ident "Seongbae Park, compiler, http://seongbae.blogspot.com"

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Andreas Tobler]

Just to mention it for all, this patch solve(d|s) a blocker (my opinion, 
even if libajva is not 1st prio ;)) on ppc-linux|darwin when invoking 
gij. Before it was not possible to launch an interpreted class file on 
the mentioned targets. Now it is!

Jakub, thanks a lot (again).

Regards,
Andreas

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Jakub Jelinek]

On Wed, Aug 29, 2007 at 12:04:54PM -0700, Seongbae Park wrote:
> There are other bugs (e.g. ud-chain based dce
> would happily delete the same insn) but I'll clean those up later.

Doesn't mark_artificial_uses take care of that for ud-chain dce?

	Jakub

Re: [PATCH] Fix DCE from removing instructions setting hard frame pointer or other artificially used registers (PR middle-end/32758)

[Kenneth Zadeck]

Jakub Jelinek wrote:
> On Wed, Aug 29, 2007 at 12:04:54PM -0700, Seongbae Park wrote:
>   
>> There are other bugs (e.g. ud-chain based dce
>> would happily delete the same insn) but I'll clean those up later.
>>     
>
> Doesn't mark_artificial_uses take care of that for ud-chain dce?
>
> 	Jakub
>   
No, for basically the same reason that caused your failure.  The
artificial uses are only at the end of the block and if you have two
defs in the block, the last one masks the first one and the first one
looks dead.

What "saves" us is the fact that this version of dce is only run before
reload/ra when the prologues and epilogues have not been generated. 
There is none of this kind of code tolerated before the prologue and
epilogue generation. 

As seongbae said, the goal is to just get rid of the artificial uses by
making everything explicit in the intermediate code.  This cleans up a
lot of problems.

Kenny