PR 41469 followup: improve __builtin_stack_restore removal

PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

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

I had a look at when we were and were not removing useless 
__builtin_stack_restore functions.  It turns out that the constraints in 
optimize_stack_restore are just way too tight.  I changed:

(1) We allow zero outgoing edges on the block as well as the single edge 
to EXIT.  This allows the restore to be elided in the case of an EH exit 
path (along with other builtin noreturn functions like __builtin_trap).

(2) We don't have to be the single use of a __builtin_stack_save.  I'm 
not sure why someone thought that should be a requirement.  Now, if 
we're not the single use, we simply don't try to delete the save.  This 
allows us to iteratively remove the N uses and the last one can remove 
the save.  There's a new test for this.

Tested on x86_64, with c, c++, fortran, ada.


r~

[-- Attachment #2: d-pr41469-2 --]
[-- Type: text/plain, Size: 4234 bytes --]

	* tree-ssa-ccp.c (optimize_stack_restore): Relax the conditions under
	which we remove __builtin_stack_restore.

testsuite/
	* gcc.c-torture/compile/pr41469.c: Add -fexceptions.
	* testsuite/gcc.dg/tree-ssa/pr41469-1.c: New.


diff --git a/gcc/testsuite/gcc.c-torture/compile/pr41469.c b/gcc/testsuite/gcc.c-torture/compile/pr41469.c
index 7813cb3..b89ee63 100644
--- a/gcc/testsuite/gcc.c-torture/compile/pr41469.c
+++ b/gcc/testsuite/gcc.c-torture/compile/pr41469.c
@@ -1,3 +1,5 @@
+/* { dg-options "-fexceptions" } */
+
 void
 af (void *a)
 {
diff --git a/gcc/testsuite/gcc.dg/tree-ssa/pr41469-1.c b/gcc/testsuite/gcc.dg/tree-ssa/pr41469-1.c
index 0c0a280..cee2c08 100644
--- a/gcc/testsuite/gcc.dg/tree-ssa/pr41469-1.c
+++ b/gcc/testsuite/gcc.dg/tree-ssa/pr41469-1.c
@@ -11,10 +11,6 @@ bf (void)
   af (v);
 }
 
-/* ??? At the moment __builtin_stack_save doesn't get eliminated at the 
-   tree level.  Probably we're no properly decrementing use counts while
-   we remove __builtin_stack_restore instances.  That said, the expansion
-   of __builtin_stack_save is removed at the rtl level, so it's not very
-   urgent to look into.  */
+/* { dg-final { scan-tree-dump-not "__builtin_stack_save" "optimized"} } */
 /* { dg-final { scan-tree-dump-not "__builtin_stack_restore" "optimized"} } */
 /* { dg-final { cleanup-tree-dump "optimized" } } */
diff --git a/gcc/tree-ssa-ccp.c b/gcc/tree-ssa-ccp.c
index 4542403..85159b2 100644
--- a/gcc/tree-ssa-ccp.c
+++ b/gcc/tree-ssa-ccp.c
@@ -3089,9 +3089,8 @@ fold_stmt_inplace (gimple stmt)
 static tree
 optimize_stack_restore (gimple_stmt_iterator i)
 {
-  tree callee, rhs;
-  gimple stmt, stack_save;
-  gimple_stmt_iterator stack_save_gsi;
+  tree callee;
+  gimple stmt;
 
   basic_block bb = gsi_bb (i);
   gimple call = gsi_stmt (i);
@@ -3115,32 +3114,49 @@ optimize_stack_restore (gimple_stmt_iterator i)
 	return NULL_TREE;
 
       if (DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_RESTORE)
-	break;
+	goto second_stack_restore;
     }
 
-  if (gsi_end_p (i)
-      && (! single_succ_p (bb)
-	  || single_succ_edge (bb)->dest != EXIT_BLOCK_PTR))
+  if (!gsi_end_p (i))
     return NULL_TREE;
 
-  stack_save = SSA_NAME_DEF_STMT (gimple_call_arg (call, 0));
-  if (gimple_code (stack_save) != GIMPLE_CALL
-      || gimple_call_lhs (stack_save) != gimple_call_arg (call, 0)
-      || stmt_could_throw_p (stack_save)
-      || !has_single_use (gimple_call_arg (call, 0)))
-    return NULL_TREE;
+  /* Allow one successor of the exit block, or zero successors.  */
+  switch (EDGE_COUNT (bb->succs))
+    {
+    case 0:
+      break;
+    case 1:
+      if (single_succ_edge (bb)->dest != EXIT_BLOCK_PTR)
+	return NULL_TREE;
+      break;
+    default:
+      return NULL_TREE;
+    }
+ second_stack_restore:
 
-  callee = gimple_call_fndecl (stack_save);
-  if (!callee
-      || DECL_BUILT_IN_CLASS (callee) != BUILT_IN_NORMAL
-      || DECL_FUNCTION_CODE (callee) != BUILT_IN_STACK_SAVE
-      || gimple_call_num_args (stack_save) != 0)
-    return NULL_TREE;
+  /* If there's exactly one use, then zap the call to __builtin_stack_save.
+     If there are multiple uses, then the last one should remove the call.
+     In any case, whether the call to __builtin_stack_save can be removed
+     or not is irrelevant to removing the call to __builtin_stack_restore.  */
+  if (has_single_use (gimple_call_arg (call, 0)))
+    {
+      gimple stack_save = SSA_NAME_DEF_STMT (gimple_call_arg (call, 0));
+      if (is_gimple_call (stack_save))
+	{
+	  callee = gimple_call_fndecl (stack_save);
+	  if (callee
+	      && DECL_BUILT_IN_CLASS (callee) == BUILT_IN_NORMAL
+	      && DECL_FUNCTION_CODE (callee) == BUILT_IN_STACK_SAVE)
+	    {
+	      gimple_stmt_iterator stack_save_gsi;
+	      tree rhs;
 
-  stack_save_gsi = gsi_for_stmt (stack_save);
-  rhs = build_int_cst (TREE_TYPE (gimple_call_arg (call, 0)), 0);
-  if (!update_call_from_tree (&stack_save_gsi, rhs))
-    return NULL_TREE;
+	      stack_save_gsi = gsi_for_stmt (stack_save);
+	      rhs = build_int_cst (TREE_TYPE (gimple_call_arg (call, 0)), 0);
+	      update_call_from_tree (&stack_save_gsi, rhs);
+	    }
+	}
+    }
 
   /* No effect, so the statement will be deleted.  */
   return integer_zero_node;

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> Tested on x86_64, with c, c++, fortran, ada.

The exact version you have installed?  It has introduced 3 ACATS failures:
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02538.html

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

On 09/28/2009 10:41 AM, Eric Botcazou wrote:
>> Tested on x86_64, with c, c++, fortran, ada.
>
> The exact version you have installed?  It has introduced 3 ACATS failures:
>    http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02538.html

I don't see those here.


r~

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> I don't see those here.

I have them on 3 machines (i586, i686, x86-64), Andreas has them on ia64:
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02541.html

They are of the form "error: BB 21 can not throw but has an EH edge".

Reverting your change fixes them.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

On 09/28/2009 11:01 AM, Eric Botcazou wrote:
>> I don't see those here.
>
> I have them on 3 machines (i586, i686, x86-64), Andreas has them on ia64:
>    http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02541.html
>
> They are of the form "error: BB 21 can not throw but has an EH edge".
>
> Reverting your change fixes them.
>

http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02573.html

I have two different failures, but not the ones you mention.


r~

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

On 09/28/2009 01:04 PM, Richard Henderson wrote:
> http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02573.html
>
> I have two different failures, but not the ones you mention.

And I'll say further that neither of the two failures that I see
are reproducible when run by hand.


r~

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02573.html
>
> I have two different failures, but not the ones you mention.

They apparently disappeared between revision 152233
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02559.html
and revision 152237
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02574.html

Probably revision 152236 fixed/masked them.

They appeared between revision 152226
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02512.html
and revision 152227
  http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02524.html

which doesn't leave much room.

I don't remember seeing similar failures during the past weeks and I 
personally saw them at 152230 for the first time, so I think r152227 
triggered/uncovered something.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

On 09/28/2009 02:13 PM, Eric Botcazou wrote:
> They appeared between revision 152226
>    http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02512.html
> and revision 152227
>    http://gcc.gnu.org/ml/gcc-testresults/2009-09/msg02524.html
>
> which doesn't leave much room.
>
> I don't remember seeing similar failures during the past weeks and I
> personally saw them at 152230 for the first time, so I think r152227
> triggered/uncovered something.

Hum.  Well it's true that I didn't test the patch on r(N-1);
the patch was tested on something that was current Friday evening.
I committed the patch Sunday afternoon after updating and not
seeing any conflicts.

But given that it's not showing up now, I'm not too concerned.
I don't see how the nothrow builtins that I was manipulating
could lead to EH CFG errors, so I assume it was something
totally unrelated which has since been fixed.


r~

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> But given that it's not showing up now, I'm not too concerned.
> I don't see how the nothrow builtins that I was manipulating
> could lead to EH CFG errors, so I assume it was something
> totally unrelated which has since been fixed.

OK, here's the story: your patch triggered the 3 failures because because it
allowed the compiler to better optimize the testcases after the "fab" pass.

For a reduced version of c37003a, "dse2" was handed down

_ada_c37003a ()
{
  void * D.2079;
  struct c37003a__rec1 * r1a.7;
  void * D.2064;
  struct c37003a__rec1 * r1.4;

<bb 2>:
  D.2064_71 = alloca (8);
  r1.4_72 = (struct c37003a__rec1 *) D.2064_71;
  r1.4_72->a1[1]{lb: 1 sz: 4} = 1;

<bb 3>:
  r1.4_72->a2[1]{lb: 1 sz: 4} = 1;
  goto <bb 5>;

<L23>:
  goto <bb 11> (<L21>);

<bb 5>:
  D.2079_94 = alloca (8);
  r1a.7_95 = (struct c37003a__rec1 *) D.2079_94;
  r1a.7_95->a1[1]{lb: 1 sz: 4} = 1;
  goto <bb 7>;

<L24>:
  goto <bb 11> (<L21>);

<bb 7>:
  r1a.7_95->a2[1]{lb: 1 sz: 4} = 1;
  goto <bb 9>;

<L25>:
  goto <bb 11> (<L21>);

<bb 9>:
  return;

<L22>:

<L21>:
  resx 1

}

and eliminated the 2 dead stores in BB2 and BB5 without removing the EH edges,
yielding

c37003a.adb:1:1: error: BB 2 can not throw but has an EH edge
c37003a.adb:1:1: error: BB 5 can not throw but has an EH edge

Unlike tree-ssa-dom.c, tree-ssa-dse.c doesn't have a need_eh_cleanup machinery
so that's not very surprising; that it only showed up yesterday is more.

After Michael's patch:

        * passes.c (init_optimization_passes): Move pass_fold_builtins
        after last phiopt pass.

the "fab" pass runs much later so the stores are not dead by the time "dse2"
is run.  As a matter of fact, there are not eliminated at all, "optimized" 
still has them:

_ada_c37003a ()
{
  void * D.2196;
  void * saved_stack.8;
  void * D.2079;
  struct c37003a__rec1 * r1a.7;
  void * D.2064;
  struct c37003a__rec1 * r1.4;

<bb 2>:
  saved_stack.8_3 = 0B;
  D.2064_71 = alloca (8);
  r1.4_72 = (struct c37003a__rec1 *) D.2064_71;
  r1.4_72->a1[1]{lb: 1 sz: 4} = 1;

<bb 3>:
  r1.4_72->a2[1]{lb: 1 sz: 4} = 1;

<bb 4>:
  D.2079_94 = alloca (8);
  r1a.7_95 = (struct c37003a__rec1 *) D.2079_94;
  r1a.7_95->a1[1]{lb: 1 sz: 4} = 1;

<bb 5>:
  r1a.7_95->a2[1]{lb: 1 sz: 4} = 1;

<bb 6>:
  GIMPLE_NOP
  return;

<L21>:
  GIMPLE_NOP
  D.2196_126 = __builtin_eh_pointer (1);
  _Unwind_Resume (D.2196_126);

}

so it looks like the benefits of your patch have been partially voided.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Michael Matz]

Hi,

On Tue, 29 Sep 2009, Eric Botcazou wrote:

> > But given that it's not showing up now, I'm not too concerned.
> > I don't see how the nothrow builtins that I was manipulating
> > could lead to EH CFG errors, so I assume it was something
> > totally unrelated which has since been fixed.
> 
> After Michael's patch:
> 
>         * passes.c (init_optimization_passes): Move pass_fold_builtins
>         after last phiopt pass.
> 
> the "fab" pass runs much later so the stores are not dead by the time 
> "dse2" is run.  As a matter of fact, there are not eliminated at all, 
> "optimized" still has them:

Hmpf.  We could move pass_fold_builtins before dse2, but that means moving 
it also before forwprop, which unfortunately can (and does) create new 
builtin folding opportunities.  As it uses fold_stmt_inplace they would be 
gone missing.  This was also the case before my patch, but still.

Hmm, the stack_restore optimizer doesn't really fit into the fold_builtin 
framework.  OTOH it seems to be a simple data flow problem, so perhaps it 
can be put into one of our several other optimization passes.


Ciao,
Michael.

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

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

> I had a look at when we were and were not removing useless
> __builtin_stack_restore functions.  It turns out that the constraints in
> optimize_stack_restore are just way too tight.  I changed:
>
> (1) We allow zero outgoing edges on the block as well as the single edge
> to EXIT.  This allows the restore to be elided in the case of an EH exit
> path (along with other builtin noreturn functions like __builtin_trap).

This breaks the newly enabled ACATS test cb1010c at -O on x86-64: a 
__builtin_stack_restore just before a RESX is eliminated in a cleanup.

I presume that's wrong, hence the attached patch.  However, this introduces 
the following couple of regressions for the testcase:

FAIL: gcc.dg/tree-ssa/pr41469-1.c scan-tree-dump-not 
optimized "__builtin_stack_
save"
FAIL: gcc.dg/tree-ssa/pr41469-1.c scan-tree-dump-not 
optimized "__builtin_stack_
restore

because it precisely has a __builtin_stack_restore just before a RESX:

<L1>:
  __builtin_stack_restore (saved_stack.3_1);
  resx 1

Tested on i586-suse-linux and x86_64-suse-linux, OK for mainline?


2009-11-04  Eric Botcazou  <ebotcazou@adacore.com>

	* tree-ssa-ccp.c (optimize_stack_restore): Do not optimize away
	a stack restore just before a RESX.


2009-11-04  Eric Botcazou  <ebotcazou@adacore.com>

	* gcc.dg/tree-ssa/pr41469-1.c: Remove.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 1809 bytes --]

Index: tree-ssa-ccp.c
===================================================================
--- tree-ssa-ccp.c	(revision 153881)
+++ tree-ssa-ccp.c	(working copy)
@@ -3085,22 +3085,24 @@ fold_stmt_inplace (gimple stmt)
   return changed;
 }
 
-/* Try to optimize out __builtin_stack_restore.  Optimize it out
-   if there is another __builtin_stack_restore in the same basic
-   block and no calls or ASM_EXPRs are in between, or if this block's
-   only outgoing edge is to EXIT_BLOCK and there are no calls or
-   ASM_EXPRs after this __builtin_stack_restore.  */
+/* Try to optimize out __builtin_stack_restore.  Optimize it out if there is
+   another __builtin_stack_restore in the same basic block and no calls or
+   ASM_EXPRs or RESXs are in between, or if this block's only outgoing edge
+   is to EXIT_BLOCK and there are no calls or ASM_EXPRs or RESXs after this
+    __builtin_stack_restore.  */
 
 static tree
 optimize_stack_restore (gimple_stmt_iterator i)
 {
   tree callee;
   gimple stmt;
+  enum gimple_code code;
 
   basic_block bb = gsi_bb (i);
   gimple call = gsi_stmt (i);
+  code = gimple_code (call);
 
-  if (gimple_code (call) != GIMPLE_CALL
+  if (code != GIMPLE_CALL
       || gimple_call_num_args (call) != 1
       || TREE_CODE (gimple_call_arg (call, 0)) != SSA_NAME
       || !POINTER_TYPE_P (TREE_TYPE (gimple_call_arg (call, 0))))
@@ -3109,9 +3111,11 @@ optimize_stack_restore (gimple_stmt_iter
   for (gsi_next (&i); !gsi_end_p (i); gsi_next (&i))
     {
       stmt = gsi_stmt (i);
-      if (gimple_code (stmt) == GIMPLE_ASM)
+      code = gimple_code (stmt);
+
+      if (code == GIMPLE_ASM || code == GIMPLE_RESX)
 	return NULL_TREE;
-      if (gimple_code (stmt) != GIMPLE_CALL)
+      if (code != GIMPLE_CALL)
 	continue;
 
       callee = gimple_call_fndecl (stmt);

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Richard Henderson]

On 11/04/2009 02:06 PM, Eric Botcazou wrote:
> This breaks the newly enabled ACATS test cb1010c at -O on x86-64: a
> __builtin_stack_restore just before a RESX is eliminated in a cleanup.
>
> I presume that's wrong...

It's only wrong if the RESX resolves within the function, i.e. the block 
has successors.  If there are no successors to the bb, then we're about 
to (effectively) perform a longjmp, and there's no point in keeping the 
stack restore.

But we already check for zero successors or a single EXIT successor, so 
I'm not sure what's wrong with the current code.  Can you describe the 
failure in more detail, or do I need to debug it myself.


r~

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

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

> It's only wrong if the RESX resolves within the function, i.e. the block
> has successors.  If there are no successors to the bb, then we're about
> to (effectively) perform a longjmp, and there's no point in keeping the
> stack restore.

You're right, and that the problem arises on x86-64 but not on i586 should 
have prompted me to further investigate.

It turns out that SP is set to a bogus value on entry to the cleanup because 
of a bogus DW_CFA_GNU_args_size entry in the FDE at -O.  Insns that adjust 
the stack but aren't FRAME_RELATED_P are processed by dwarf2out_frame_debug 
and discarded for ACCUMULATE_OUTGOING_ARGS targets:

  if (! RTX_FRAME_RELATED_P (insn))
    {
      if (!ACCUMULATE_OUTGOING_ARGS)
	dwarf2out_stack_adjust (insn, after_p);
      return;
    }

But if one of them is merged in a FRAME_RELATED_P insn by the csa pass, it is 
further processed by dwarf2out_frame_debug_expr:

      for (par_index = 0; par_index < limit; par_index++)
	{
	  elem = XVECEXP (expr, 0, par_index);
	  if (GET_CODE (elem) == SET
	      && (!MEM_P (SET_DEST (elem)) || GET_CODE (expr) == SEQUENCE)
	      && (RTX_FRAME_RELATED_P (elem) || par_index == 0))
	    dwarf2out_frame_debug_expr (elem, label);
	  else if (GET_CODE (elem) == SET
		   && par_index != 0
		   && !RTX_FRAME_RELATED_P (elem))
	    {
	      /* Stack adjustment combining might combine some post-prologue
		 stack adjustment into a prologue stack adjustment.  */
	      HOST_WIDE_INT offset = stack_adjust_offset (elem, args_size, 0);

	      if (offset != 0)
		dwarf2out_args_size_adjust (offset, label);
	    }
	}

Now the arg size adjustment is recorded whatever ACCUMULATE_OUTGOING_ARGS, 
thus leading to a bogus discrepancy.

Hence the attached patch which fixes the bug as well as move some related 
functions in dwarf2out.c around.  Tested on x86-64, OK for mainline?


2009-11-08  Eric Botcazou  <ebotcazou@adacore.com>

	* tree.h (dwarf2out_args_size): Delete.
	* dwarf2out.c (dwarf2out_args_size): Make static and move around.
	(dwarf2out_args_size_adjust): Delete prototype and move around.
	(dwarf2out_frame_debug_expr): Do not record arg size adjustments for
	ACCUMULATE_OUTGOING_ARGS targets.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 4177 bytes --]

Index: tree.h
===================================================================
--- tree.h	(revision 153889)
+++ tree.h	(working copy)
@@ -5161,11 +5161,6 @@ extern void dwarf2out_def_cfa (const cha
 
 extern void dwarf2out_window_save (const char *);
 
-/* Add a CFI to update the running total of the size of arguments pushed
-   onto the stack.  */
-
-extern void dwarf2out_args_size (const char *, HOST_WIDE_INT);
-
 /* Entry point for saving a register to the stack.  */
 
 extern void dwarf2out_reg_save (const char *, unsigned, HOST_WIDE_INT);
Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 153889)
+++ dwarf2out.c	(working copy)
@@ -470,8 +470,6 @@ static void output_cfi (dw_cfi_ref, dw_f
 static void output_cfi_directive (dw_cfi_ref);
 static void output_call_frame_info (int);
 static void dwarf2out_note_section_used (void);
-static void dwarf2out_stack_adjust (rtx, bool);
-static void dwarf2out_args_size_adjust (HOST_WIDE_INT, const char *);
 static void flush_queued_reg_saves (void);
 static bool clobbers_queued_reg_save (const_rtx);
 static void dwarf2out_frame_debug_expr (rtx, const char *);
@@ -1157,25 +1155,6 @@ dwarf2out_window_save (const char *label
   add_fde_cfi (label, cfi);
 }
 
-/* Add a CFI to update the running total of the size of arguments
-   pushed onto the stack.  */
-
-void
-dwarf2out_args_size (const char *label, HOST_WIDE_INT size)
-{
-  dw_cfi_ref cfi;
-
-  if (size == old_args_size)
-    return;
-
-  old_args_size = size;
-
-  cfi = new_cfi ();
-  cfi->dw_cfi_opc = DW_CFA_GNU_args_size;
-  cfi->dw_cfi_oprnd1.dw_cfi_offset = size;
-  add_fde_cfi (label, cfi);
-}
-
 /* Entry point for saving a register to the stack.  REG is the GCC register
    number.  LABEL and OFFSET are passed to reg_save.  */
 
@@ -1526,6 +1505,48 @@ compute_barrier_args_size (void)
   VEC_free (rtx, heap, next);
 }
 
+/* Add a CFI to update the running total of the size of arguments
+   pushed onto the stack.  */
+
+static void
+dwarf2out_args_size (const char *label, HOST_WIDE_INT size)
+{
+  dw_cfi_ref cfi;
+
+  if (size == old_args_size)
+    return;
+
+  old_args_size = size;
+
+  cfi = new_cfi ();
+  cfi->dw_cfi_opc = DW_CFA_GNU_args_size;
+  cfi->dw_cfi_oprnd1.dw_cfi_offset = size;
+  add_fde_cfi (label, cfi);
+}
+
+/* Adjust args_size based on stack adjustment OFFSET.  */
+
+static void
+dwarf2out_args_size_adjust (HOST_WIDE_INT offset, const char *label)
+{
+  if (cfa.reg == STACK_POINTER_REGNUM)
+    cfa.offset += offset;
+
+  if (cfa_store.reg == STACK_POINTER_REGNUM)
+    cfa_store.offset += offset;
+
+#ifndef STACK_GROWS_DOWNWARD
+  offset = -offset;
+#endif
+
+  args_size += offset;
+  if (args_size < 0)
+    args_size = 0;
+
+  def_cfa_1 (label, &cfa);
+  if (flag_asynchronous_unwind_tables)
+    dwarf2out_args_size (label, args_size);
+}
 
 /* Check INSN to see if it looks like a push or a stack adjustment, and
    make a note of it if it does.  EH uses this information to find out how
@@ -1619,30 +1640,6 @@ dwarf2out_stack_adjust (rtx insn, bool a
   dwarf2out_args_size_adjust (offset, label);
 }
 
-/* Adjust args_size based on stack adjustment OFFSET.  */
-
-static void
-dwarf2out_args_size_adjust (HOST_WIDE_INT offset, const char *label)
-{
-  if (cfa.reg == STACK_POINTER_REGNUM)
-    cfa.offset += offset;
-
-  if (cfa_store.reg == STACK_POINTER_REGNUM)
-    cfa_store.offset += offset;
-
-#ifndef STACK_GROWS_DOWNWARD
-  offset = -offset;
-#endif
-
-  args_size += offset;
-  if (args_size < 0)
-    args_size = 0;
-
-  def_cfa_1 (label, &cfa);
-  if (flag_asynchronous_unwind_tables)
-    dwarf2out_args_size (label, args_size);
-}
-
 #endif
 
 /* We delay emitting a register save until either (a) we reach the end
@@ -2209,7 +2206,8 @@ dwarf2out_frame_debug_expr (rtx expr, co
 	      && (!MEM_P (SET_DEST (elem)) || GET_CODE (expr) == SEQUENCE)
 	      && (RTX_FRAME_RELATED_P (elem) || par_index == 0))
 	    dwarf2out_frame_debug_expr (elem, label);
-	  else if (GET_CODE (elem) == SET
+	  else if (!ACCUMULATE_OUTGOING_ARGS
+		   && GET_CODE (elem) == SET
 		   && par_index != 0
 		   && !RTX_FRAME_RELATED_P (elem))
 	    {

Re: PR 41469 followup: improve __builtin_stack_restore removal

[H.J. Lu]

On Sun, Nov 8, 2009 at 3:53 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> It's only wrong if the RESX resolves within the function, i.e. the block
>> has successors.  If there are no successors to the bb, then we're about
>> to (effectively) perform a longjmp, and there's no point in keeping the
>> stack restore.
>
> You're right, and that the problem arises on x86-64 but not on i586 should
> have prompted me to further investigate.
>
> It turns out that SP is set to a bogus value on entry to the cleanup because
> of a bogus DW_CFA_GNU_args_size entry in the FDE at -O.  Insns that adjust
> the stack but aren't FRAME_RELATED_P are processed by dwarf2out_frame_debug
> and discarded for ACCUMULATE_OUTGOING_ARGS targets:
>
>  if (! RTX_FRAME_RELATED_P (insn))
>    {
>      if (!ACCUMULATE_OUTGOING_ARGS)
>        dwarf2out_stack_adjust (insn, after_p);
>      return;
>    }
>
> But if one of them is merged in a FRAME_RELATED_P insn by the csa pass, it is
> further processed by dwarf2out_frame_debug_expr:
>
>      for (par_index = 0; par_index < limit; par_index++)
>        {
>          elem = XVECEXP (expr, 0, par_index);
>          if (GET_CODE (elem) == SET
>              && (!MEM_P (SET_DEST (elem)) || GET_CODE (expr) == SEQUENCE)
>              && (RTX_FRAME_RELATED_P (elem) || par_index == 0))
>            dwarf2out_frame_debug_expr (elem, label);
>          else if (GET_CODE (elem) == SET
>                   && par_index != 0
>                   && !RTX_FRAME_RELATED_P (elem))
>            {
>              /* Stack adjustment combining might combine some post-prologue
>                 stack adjustment into a prologue stack adjustment.  */
>              HOST_WIDE_INT offset = stack_adjust_offset (elem, args_size, 0);
>
>              if (offset != 0)
>                dwarf2out_args_size_adjust (offset, label);
>            }
>        }
>
> Now the arg size adjustment is recorded whatever ACCUMULATE_OUTGOING_ARGS,
> thus leading to a bogus discrepancy.
>
> Hence the attached patch which fixes the bug as well as move some related
> functions in dwarf2out.c around.  Tested on x86-64, OK for mainline?
>
>
> 2009-11-08  Eric Botcazou  <ebotcazou@adacore.com>
>
>        * tree.h (dwarf2out_args_size): Delete.
>        * dwarf2out.c (dwarf2out_args_size): Make static and move around.
>        (dwarf2out_args_size_adjust): Delete prototype and move around.
>        (dwarf2out_frame_debug_expr): Do not record arg size adjustments for
>        ACCUMULATE_OUTGOING_ARGS targets.
>


Will it fix

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37022



-- 
H.J.

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> Will it fix
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=37022

AFAICS no, it won't, the PR pertains to !ACCUMULATE_OUTGOING_ARGS targets.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Ian Lance Taylor]

Eric Botcazou <ebotcazou@adacore.com> writes:

> 2009-11-08  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	* tree.h (dwarf2out_args_size): Delete.
> 	* dwarf2out.c (dwarf2out_args_size): Make static and move around.
> 	(dwarf2out_args_size_adjust): Delete prototype and move around.
> 	(dwarf2out_frame_debug_expr): Do not record arg size adjustments for
> 	ACCUMULATE_OUTGOING_ARGS targets.

This is OK.

Thanks.

Ian

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

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

> > 2009-11-08  Eric Botcazou  <ebotcazou@adacore.com>
> >
> > 	* tree.h (dwarf2out_args_size): Delete.
> > 	* dwarf2out.c (dwarf2out_args_size): Make static and move around.
> > 	(dwarf2out_args_size_adjust): Delete prototype and move around.
> > 	(dwarf2out_frame_debug_expr): Do not record arg size adjustments for
> > 	ACCUMULATE_OUTGOING_ARGS targets.
>
> This is OK.

Thanks.  The problem is, dwarf2out_args_size_adjust is a bit of a misnomer:

/* Adjust args_size based on stack adjustment OFFSET.  */

static void
dwarf2out_args_size_adjust (HOST_WIDE_INT offset, const char *label)
{

but it does more, in particular

  if (cfa_store.reg == STACK_POINTER_REGNUM)
    cfa_store.offset += offset;

So, even if the CFA register is not the stack pointer anymore, for example 
with an alloca in the function, the stack adjustment may need to be processed 
by dwarf2out_args_size_adjust for ACCUMULATE_OUTGOING_ARGS targets.  The 
problem was already latent since dwarf2out_stack_adjust is not invoked at all 
for ACCUMULATE_OUTGOING_ARGS targets but my patch exacerbated it and caused 
g++.dg/torture/stackalign/unwind-2.C to regress at -O on i686.

Hence the attached patch; it is only aimed at fixing the regression so I've 
added a ??? comment for the latent problem.

Tested on i686-suse-linux and x86_64-suse-linux, OK for mainline?


2009-11-11  Eric Botcazou  <ebotcazou@adacore.com>

	PR target/10127
	PR ada/20548
	PR middle-end/42004
	* dwarf2out.c (dwarf2out_args_size_adjust): Rename to...
	(dwarf2out_stack_adjust): ...this.  Do not adjust the arg size for
	ACCUMULATE_OUTGOING_ARGS targets.  Rename former version to...
	(dwarf2out_notice_stack_adjust): ...this.  Adjust for above renaming.
	(dwarf2out_frame_debug_expr): Revert previous change and adjust for
	above renaming.
	(dwarf2out_frame_debug): Add ??? comment.  Adjust for above renaming.


-- 
Eric Botcazou

[-- Attachment #2: p.diff --]
[-- Type: text/x-diff, Size: 2906 bytes --]

Index: dwarf2out.c
===================================================================
--- dwarf2out.c	(revision 154079)
+++ dwarf2out.c	(working copy)
@@ -1524,10 +1524,10 @@ dwarf2out_args_size (const char *label, 
   add_fde_cfi (label, cfi);
 }
 
-/* Adjust args_size based on stack adjustment OFFSET.  */
+/* Record a stack adjustment of OFFSET bytes.  */
 
 static void
-dwarf2out_args_size_adjust (HOST_WIDE_INT offset, const char *label)
+dwarf2out_stack_adjust (HOST_WIDE_INT offset, const char *label)
 {
   if (cfa.reg == STACK_POINTER_REGNUM)
     cfa.offset += offset;
@@ -1535,6 +1535,9 @@ dwarf2out_args_size_adjust (HOST_WIDE_IN
   if (cfa_store.reg == STACK_POINTER_REGNUM)
     cfa_store.offset += offset;
 
+  if (ACCUMULATE_OUTGOING_ARGS)
+    return;
+
 #ifndef STACK_GROWS_DOWNWARD
   offset = -offset;
 #endif
@@ -1549,11 +1552,11 @@ dwarf2out_args_size_adjust (HOST_WIDE_IN
 }
 
 /* Check INSN to see if it looks like a push or a stack adjustment, and
-   make a note of it if it does.  EH uses this information to find out how
-   much extra space it needs to pop off the stack.  */
+   make a note of it if it does.  EH uses this information to find out
+   how much extra space it needs to pop off the stack.  */
 
 static void
-dwarf2out_stack_adjust (rtx insn, bool after_p)
+dwarf2out_notice_stack_adjust (rtx insn, bool after_p)
 {
   HOST_WIDE_INT offset;
   const char *label;
@@ -1637,7 +1640,7 @@ dwarf2out_stack_adjust (rtx insn, bool a
     return;
 
   label = dwarf2out_cfi_label (false);
-  dwarf2out_args_size_adjust (offset, label);
+  dwarf2out_stack_adjust (offset, label);
 }
 
 #endif
@@ -2206,8 +2209,7 @@ dwarf2out_frame_debug_expr (rtx expr, co
 	      && (!MEM_P (SET_DEST (elem)) || GET_CODE (expr) == SEQUENCE)
 	      && (RTX_FRAME_RELATED_P (elem) || par_index == 0))
 	    dwarf2out_frame_debug_expr (elem, label);
-	  else if (!ACCUMULATE_OUTGOING_ARGS
-		   && GET_CODE (elem) == SET
+	  else if (GET_CODE (elem) == SET
 		   && par_index != 0
 		   && !RTX_FRAME_RELATED_P (elem))
 	    {
@@ -2216,7 +2218,7 @@ dwarf2out_frame_debug_expr (rtx expr, co
 	      HOST_WIDE_INT offset = stack_adjust_offset (elem, args_size, 0);
 
 	      if (offset != 0)
-		dwarf2out_args_size_adjust (offset, label);
+		dwarf2out_stack_adjust (offset, label);
 	    }
 	}
       return;
@@ -2709,10 +2711,13 @@ dwarf2out_frame_debug (rtx insn, bool af
   if (!NONJUMP_INSN_P (insn) || clobbers_queued_reg_save (insn))
     flush_queued_reg_saves ();
 
-  if (! RTX_FRAME_RELATED_P (insn))
+  if (!RTX_FRAME_RELATED_P (insn))
     {
+      /* ??? This should be done unconditionally since stack adjustments
+	 matter if the stack pointer is not the CFA register anymore but
+	 is still used to save registers.  */
       if (!ACCUMULATE_OUTGOING_ARGS)
-	dwarf2out_stack_adjust (insn, after_p);
+	dwarf2out_notice_stack_adjust (insn, after_p);
       return;
     }
 

Re: PR 41469 followup: improve __builtin_stack_restore removal

[H.J. Lu]

On Wed, Nov 11, 2009 at 8:26 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> > 2009-11-08  Eric Botcazou  <ebotcazou@adacore.com>
>> >
>> >     * tree.h (dwarf2out_args_size): Delete.
>> >     * dwarf2out.c (dwarf2out_args_size): Make static and move around.
>> >     (dwarf2out_args_size_adjust): Delete prototype and move around.
>> >     (dwarf2out_frame_debug_expr): Do not record arg size adjustments for
>> >     ACCUMULATE_OUTGOING_ARGS targets.
>>
>> This is OK.
>
> Thanks.  The problem is, dwarf2out_args_size_adjust is a bit of a misnomer:
>
> /* Adjust args_size based on stack adjustment OFFSET.  */
>
> static void
> dwarf2out_args_size_adjust (HOST_WIDE_INT offset, const char *label)
> {
>
> but it does more, in particular
>
>  if (cfa_store.reg == STACK_POINTER_REGNUM)
>    cfa_store.offset += offset;
>
> So, even if the CFA register is not the stack pointer anymore, for example
> with an alloca in the function, the stack adjustment may need to be processed
> by dwarf2out_args_size_adjust for ACCUMULATE_OUTGOING_ARGS targets.  The
> problem was already latent since dwarf2out_stack_adjust is not invoked at all
> for ACCUMULATE_OUTGOING_ARGS targets but my patch exacerbated it and caused
> g++.dg/torture/stackalign/unwind-2.C to regress at -O on i686.
>
> Hence the attached patch; it is only aimed at fixing the regression so I've
> added a ??? comment for the latent problem.
>
> Tested on i686-suse-linux and x86_64-suse-linux, OK for mainline?
>
>
> 2009-11-11  Eric Botcazou  <ebotcazou@adacore.com>
>
>        PR target/10127
>        PR ada/20548
>        PR middle-end/42004
>        * dwarf2out.c (dwarf2out_args_size_adjust): Rename to...
>        (dwarf2out_stack_adjust): ...this.  Do not adjust the arg size for
>        ACCUMULATE_OUTGOING_ARGS targets.  Rename former version to...
>        (dwarf2out_notice_stack_adjust): ...this.  Adjust for above renaming.
>        (dwarf2out_frame_debug_expr): Revert previous change and adjust for
>        above renaming.
>        (dwarf2out_frame_debug): Add ??? comment.  Adjust for above renaming.
>

Did you check in any testcases for PR target/10127?

Thanks.


-- 
H.J.

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> Did you check in any testcases for PR target/10127?

A couple of Ada testcases gnat.dg/stack_check[12].adb.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[H.J. Lu]

On Wed, Nov 11, 2009 at 8:51 AM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Did you check in any testcases for PR target/10127?
>
> A couple of Ada testcases gnat.dg/stack_check[12].adb.
>

Why not add the testcase in

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10127

-- 
H.J.

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> Why not add the testcase in
>
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=10127

Because it doesn't fail on the machines I use.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

> 2009-11-11  Eric Botcazou  <ebotcazou@adacore.com>
>
> 	PR target/10127
> 	PR ada/20548
> 	PR middle-end/42004
> 	* dwarf2out.c (dwarf2out_args_size_adjust): Rename to...
> 	(dwarf2out_stack_adjust): ...this.  Do not adjust the arg size for
> 	ACCUMULATE_OUTGOING_ARGS targets.  Rename former version to...
> 	(dwarf2out_notice_stack_adjust): ...this.  Adjust for above renaming.
> 	(dwarf2out_frame_debug_expr): Revert previous change and adjust for
> 	above renaming.
> 	(dwarf2out_frame_debug): Add ??? comment.  Adjust for above renaming.

I've applied it using the "revert your own change" rule for 40%, the "obvious 
change" rule for 20%, the remaining 40% coming from Ian's approval for the 
previous change that did essentially the same thing.

Retested on i586-suse-linux and x86_64-suse-linux.

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Dave Korn]

Eric Botcazou wrote:

> I've applied it using the "revert your own change" rule for 40%, the "obvious 
> change" rule for 20%, the remaining 40% coming from Ian's approval for the 
> previous change that did essentially the same thing.

  I don't see any smiley, so I'll just mention that I think the second two of
those three items are really stretching a point.  There's a discussion over on
the main list right now about exactly how far "obvious" goes, and I don't
think anyone's suggesting it covers serious functional changes; and since part
of the point of the approval process is for code review to be done, I'm not
sure that approval should be that easily transferred to code that has not been
explicitly reviewed and isn't even functionally identical (per your
description of it as only "essentially" the same thing).

  I'm not objecting to your patch, btw, just would like your input, since
there's a thread about this topic going on right now.

    cheers,
      DaveK

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Paolo Bonzini]

On 11/25/2009 11:02 PM, Dave Korn wrote:
>    I don't see any smiley, so I'll just mention that I think the second two of
> those three items are really stretching a point.  There's a discussion over on
> the main list right now about exactly how far "obvious" goes, and I don't
> think anyone's suggesting it covers serious functional changes; and since part
> of the point of the approval process is for code review to be done, I'm not
> sure that approval should be that easily transferred to code that has not been
> explicitly reviewed and isn't even functionally identical (per your
> description of it as only "essentially" the same thing).
>
>    I'm not objecting to your patch, btw, just would like your input, since
> there's a thread about this topic going on right now.

I think it goes very much with the person.  Eric is an RTL maintainer, 
and probably nobody knows whether dwarf2out or parts of it is included 
or not.  Besides, his record of patches and especially reviews is such 
that very few people if anyone would care.

GCC rarely had any problems with "not-so-obvious" stuff checked in as 
obvious and when there were problems they were promptly solved by 
reverting or approving the patch.  In no case luckily we had to recourse 
to any kind of disciplinary action.  We should be proud of that :-) and 
use our judgement so that it stays that way.

Paolo

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Eric Botcazou]

>   I'm not objecting to your patch, btw, just would like your input, since
> there's a thread about this topic going on right now.

I think people shouldn't waste so much time discussing, acting is better. :-)

-- 
Eric Botcazou

Re: PR 41469 followup: improve __builtin_stack_restore removal

[Dave Korn]

Eric Botcazou wrote:
>>   I'm not objecting to your patch, btw, just would like your input, since
>> there's a thread about this topic going on right now.
> 
> I think people shouldn't waste so much time discussing, acting is better. :-)
> 

  Heh, that puts you in the same boat as HJ I guess!

    cheers,
      DaveK