[RFC] Fix unwind info for sparc -mflat

[RFC] Fix unwind info for sparc -mflat

[Richard Henderson]

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

The current code generation for -mflat uses 3 insn patterns
which emit up two three insns (sort of) emulating the save
instruction.

The problem is that the unwind info is only produced at the
end of any pattern, leaving a 1 or 2 insn hole for which the
unwind info is not correct.

I tried to figure out why things had been done in this
slightly convoluted manner and failed.  It seems to me that
this is easily represented with the individual instructions.
A comment indicated that there had been problems with the
copy to %o7 being deleted.  Elsewhere we have successfully
used a naked USE pattern to keep such things from being
deleted.

Unfortunately, I can't seem to get -mflat to bootstrap on
gcc62 (as sparc-linux, not sparc64) either before or after
this patch, so I'm not sure what sort of pre-requisite I'm
missing in order to be able to test it...

Comments?


r~

[-- Attachment #2: commit-sparc-flat-cfi --]
[-- Type: text/plain, Size: 7658 bytes --]

commit 7c9f3f4fa56b6603007d254afad18c47009bc5af
Author: Richard Henderson <rth@twiddle.net>
Date:   Mon Jun 20 16:42:14 2011 -0700

    sparc: Fix -mflat unwind info.
    
    The old definition left a 2 instruction hole in which
    unwind info was out-of-date.

diff --git a/gcc/config/sparc/sparc.c b/gcc/config/sparc/sparc.c
index e50d2f1..7d83dd6 100644
--- a/gcc/config/sparc/sparc.c
+++ b/gcc/config/sparc/sparc.c
@@ -4617,39 +4617,6 @@ emit_save_register_window (rtx increment)
   return insn;
 }
 
-/* Generate a create_flat_frame_1 insn.  */
-
-static rtx
-gen_create_flat_frame_1 (rtx increment)
-{
-  if (TARGET_ARCH64)
-    return gen_create_flat_frame_1di (increment);
-  else
-    return gen_create_flat_frame_1si (increment);
-}
-
-/* Generate a create_flat_frame_2 insn.  */
-
-static rtx
-gen_create_flat_frame_2 (rtx increment)
-{
-  if (TARGET_ARCH64)
-    return gen_create_flat_frame_2di (increment);
-  else
-    return gen_create_flat_frame_2si (increment);
-}
-
-/* Generate a create_flat_frame_3 insn.  */
-
-static rtx
-gen_create_flat_frame_3 (rtx increment)
-{
-  if (TARGET_ARCH64)
-    return gen_create_flat_frame_3di (increment);
-  else
-    return gen_create_flat_frame_3si (increment);
-}
-
 /* Generate an increment for the stack pointer.  */
 
 static rtx
@@ -4793,7 +4760,6 @@ sparc_flat_expand_prologue (void)
 {
   HOST_WIDE_INT size;
   rtx insn;
-  int i;
 
   sparc_leaf_function_p = optimize > 0 && current_function_is_leaf;
 
@@ -4811,103 +4777,64 @@ sparc_flat_expand_prologue (void)
 
   if (size == 0)
     ; /* do nothing.  */
-  else if (frame_pointer_needed)
+  else
     {
-      if (size <= 4096)
-	{
-	  if (return_addr_reg_needed_p (sparc_leaf_function_p))
-	    insn = emit_insn (gen_create_flat_frame_1 (GEN_INT (-size)));
-	  else
-	    insn = emit_insn (gen_create_flat_frame_2 (GEN_INT (-size)));
-	  RTX_FRAME_RELATED_P (insn) = 1;
-	  for (i=0; i < XVECLEN (PATTERN (insn), 0); i++)
-	    RTX_FRAME_RELATED_P (XVECEXP (PATTERN (insn), 0, i)) = 1;
-	}
-      else
-	{
-	  rtx reg = gen_rtx_REG (Pmode, 1), note;
-	  emit_move_insn (reg, GEN_INT (-size));
-	  if (return_addr_reg_needed_p (sparc_leaf_function_p))
-	    {
-	      insn = emit_insn (gen_create_flat_frame_1 (reg));
-	      note
-		= gen_rtx_PARALLEL (VOIDmode,
-				    gen_rtvec
-				    (3, copy_rtx
-					(XVECEXP (PATTERN (insn), 0, 0)),
-					gen_stack_pointer_inc
-					(GEN_INT (-size)),
-					copy_rtx
-					(XVECEXP (PATTERN (insn), 0, 2))));
-	    }
-	  else
-	    {
-	      insn = emit_insn (gen_create_flat_frame_2 (reg));
-	      note
-		= gen_rtx_PARALLEL (VOIDmode,
-				    gen_rtvec
-				    (2, copy_rtx
-					(XVECEXP (PATTERN (insn), 0, 0)),
-					gen_stack_pointer_inc
-					(GEN_INT (-size))));
-	    }
+      rtx size_int_rtx, size_rtx;
+
+      size_rtx = size_int_rtx = GEN_INT (-size);
 
-	  RTX_FRAME_RELATED_P (insn) = 1;
-	  add_reg_note (insn, REG_FRAME_RELATED_EXPR, note);
-	  for (i=0; i < XVECLEN (note, 0); i++)
-	    RTX_FRAME_RELATED_P (XVECEXP (note, 0, i)) = 1;
-	}
-    }
-  else if (return_addr_reg_needed_p (sparc_leaf_function_p))
-    {
       if (size <= 4096)
+	insn = emit_insn (gen_stack_pointer_inc (size_int_rtx));
+      else if (size <= 8192 && !frame_pointer_needed)
 	{
-	  insn = emit_insn (gen_create_flat_frame_3 (GEN_INT (-size)));
+	  insn = emit_insn (gen_stack_pointer_inc (GEN_INT (-4096)));
 	  RTX_FRAME_RELATED_P (insn) = 1;
-	  for (i=0; i < XVECLEN (PATTERN (insn), 0); i++)
-	    RTX_FRAME_RELATED_P (XVECEXP (PATTERN (insn), 0, i)) = 1;
+	  insn = emit_insn (gen_stack_pointer_inc (GEN_INT (4096 - size)));
 	}
       else
 	{
-	  rtx reg = gen_rtx_REG (Pmode, 1), note;
-	  emit_move_insn (reg, GEN_INT (-size));
-	  insn = emit_insn (gen_create_flat_frame_3 (reg));
-	  note
-	    = gen_rtx_PARALLEL (VOIDmode,
-				gen_rtvec
-				(2, gen_stack_pointer_inc (GEN_INT (-size)),
-				    copy_rtx
-				    (XVECEXP (PATTERN (insn), 0, 1))));
-	  RTX_FRAME_RELATED_P (insn) = 1;
-	  add_reg_note (insn, REG_FRAME_RELATED_EXPR, note);
-	  for (i=0; i < XVECLEN (note, 0); i++)
-	    RTX_FRAME_RELATED_P (XVECEXP (note, 0, i)) = 1;
+	  size_rtx = gen_rtx_REG (Pmode, 1);
+	  emit_move_insn (size_rtx, size_int_rtx);
+	  insn = emit_insn (gen_stack_pointer_inc (size_rtx));
+	  add_reg_note (insn, REG_CFA_ADJUST_CFA,
+			gen_stack_pointer_inc (size_int_rtx));
 	}
-    }
-  else
-    {
-      if (size <= 4096)
-	insn = emit_insn (gen_stack_pointer_inc (GEN_INT (-size)));
-      else if (size <= 8192)
+      RTX_FRAME_RELATED_P (insn) = 1;
+
+      if (frame_pointer_needed)
 	{
-	  insn = emit_insn (gen_stack_pointer_inc (GEN_INT (-4096)));
+	  insn = emit_insn (gen_rtx_SET (VOIDmode, hard_frame_pointer_rtx,
+					 gen_rtx_MINUS (Pmode,
+							stack_pointer_rtx,
+							size_rtx)));
 	  RTX_FRAME_RELATED_P (insn) = 1;
-	  insn = emit_insn (gen_stack_pointer_inc (GEN_INT (4096 - size)));
+
+	  add_reg_note (insn, REG_CFA_ADJUST_CFA,
+			gen_rtx_SET (VOIDmode, hard_frame_pointer_rtx,
+				     plus_constant (stack_pointer_rtx,
+						    size)));
+
+	  /* Make sure nothing is scheduled until after the frame
+	     is established.  */
+	  emit_insn (gen_blockage ());
 	}
-      else
+
+      if (return_addr_reg_needed_p (sparc_leaf_function_p))
 	{
-	  rtx reg = gen_rtx_REG (Pmode, 1);
-	  emit_move_insn (reg, GEN_INT (-size));
-	  insn = emit_insn (gen_stack_pointer_inc (reg));
-	  add_reg_note (insn, REG_FRAME_RELATED_EXPR,
-			gen_stack_pointer_inc (GEN_INT (-size)));
-	}
+	  rtx i7 = gen_rtx_REG (Pmode, INCOMING_RETURN_ADDR_REGNUM);
+	  rtx o7 = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
 
-      RTX_FRAME_RELATED_P (insn) = 1;
-    }
+	  insn = emit_move_insn (o7, i7);
+	  RTX_FRAME_RELATED_P (insn) = 1;
 
-  /* Make sure nothing is scheduled until after the frame is established.  */
-  emit_insn (gen_blockage ());
+	  add_reg_note (insn, REG_CFA_REGISTER,
+			gen_rtx_SET (VOIDmode, o7, i7));
+
+	  /* Prevent this instruction from ever being considered dead,
+	     even if this function has no epilogue.  */
+	  emit_insn (gen_rtx_USE (VOIDmode, o7));
+	}
+    }
 
   if (frame_pointer_needed)
     {
diff --git a/gcc/config/sparc/sparc.md b/gcc/config/sparc/sparc.md
index 017b689..2c8d306 100644
--- a/gcc/config/sparc/sparc.md
+++ b/gcc/config/sparc/sparc.md
@@ -6287,39 +6287,6 @@
   "save\t%%sp, %0, %%sp"
   [(set_attr "type" "savew")])
 
-;; For the "create flat frame" insns, we need to use special insns
-;; because %fp cannot be clobbered until after the frame is established (so
-;; that it contains the live register window save area) and %i7 changed with
-;; a simple move as it is a fixed register and the move would be eliminated.
-
-(define_insn "create_flat_frame_1<P:mode>"
-  [(set (reg:P 30) (reg:P 14))
-   (set (reg:P 14) (plus:P (reg:P 14)
-			   (match_operand:P 0 "arith_operand" "rI")))
-   (set (reg:P 31) (reg:P 15))]
-  "TARGET_FLAT"
-  "add\t%%sp, %0, %%sp\n\tsub\t%%sp, %0, %%fp\n\tmov\t%%o7, %%i7"
-  [(set_attr "type" "multi")
-   (set_attr "length" "3")])
-
-(define_insn "create_flat_frame_2<P:mode>"
-  [(set (reg:P 30) (reg:P 14))
-   (set (reg:P 14) (plus:P (reg:P 14)
-		           (match_operand:P 0 "arith_operand" "rI")))]
-  "TARGET_FLAT"
-  "add\t%%sp, %0, %%sp\n\tsub\t%%sp, %0, %%fp"
-  [(set_attr "type" "multi")
-   (set_attr "length" "2")])
-
-(define_insn "create_flat_frame_3<P:mode>"
-  [(set (reg:P 14) (plus:P (reg:P 14)
-		           (match_operand:P 0 "arith_operand" "rI")))
-   (set (reg:P 31) (reg:P 15))]
-  "TARGET_FLAT"
-  "add\t%%sp, %0, %%sp\n\tmov\t%%o7, %%i7"
-  [(set_attr "type" "multi")
-   (set_attr "length" "2")])
-
 (define_expand "epilogue"
   [(return)]
   ""

Re: [RFC] Fix unwind info for sparc -mflat

[Richard Henderson]

On 06/20/2011 05:33 PM, Richard Henderson wrote:
> The current code generation for -mflat uses 3 insn patterns
> which emit up two three insns (sort of) emulating the save

Lest I get sent back for remedial English, that was supposed
to be "emit two or three" but fingers got ahead of brain.


r~

Re: [RFC] Fix unwind info for sparc -mflat

[Eric Botcazou]

> I tried to figure out why things had been done in this
> slightly convoluted manner and failed.  It seems to me that
> this is easily represented with the individual instructions.

I wanted to avoid the back-and-forth game on the CFA offset and emit the same 
CFIs as in the normal case.  Your solution is probably better in the end, but 
please add a comment in sparc_flat_expand_prologue explaining why we play this 
game to establish the frame.

> A comment indicated that there had been problems with the
> copy to %o7 being deleted.  Elsewhere we have successfully
> used a naked USE pattern to keep such things from being
> deleted.

Fine, thanks for cleaning up the whole thing.

> Unfortunately, I can't seem to get -mflat to bootstrap on
> gcc62 (as sparc-linux, not sparc64) either before or after
> this patch, so I'm not sure what sort of pre-requisite I'm
> missing in order to be able to test it...

Just run the C testsuite, bi-arch preferably, with -mflat; it should be clean.
It contains some unwinding tests.  You can also run the C++ testsuite, but 
there are about 20 (known) failures without -mflat multilibs.

I didn't try a bootstrap, but it probably doesn't work because the models are 
fully intermixable only if you insert flushw instructions at the boundaries.

-- 
Eric Botcazou

Re: [RFC] Fix unwind info for sparc -mflat

[Eric Botcazou]

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

> I tried to figure out why things had been done in this
> slightly convoluted manner and failed.  It seems to me that
> this is easily represented with the individual instructions.
> A comment indicated that there had been problems with the
> copy to %o7 being deleted.  Elsewhere we have successfully
> used a naked USE pattern to keep such things from being
> deleted.

I have 4 regressions in the C testsuite with -mflat on SPARC/Solaris:

FAIL: gcc.c-torture/execute/builtins/stpcpy-chk.c execution,  -O2 -flto
FAIL: gcc.c-torture/execute/nestfunc-4.c execution,  -Os
FAIL: gcc.c-torture/execute/pr19005.c execution,  -O2
FAIL: gcc.c-torture/execute/strncmp-1.c execution,  -O2 -flt

The problem is that the blockage isn't emitted if the frame pointer isn't used.
I have also fixed the swapping %o7/%i7 (despite the name, %o7 is the incoming 
return address register from GCC's viewpoint, and %i7 the register where you 
save the return address for the rest of the function).

Tested on SPARC/Solaris, applied on the mainline.


2011-06-26  Eric Botcazou  <ebotcazou@adacore.com>

	* config/sparc/sparc.c (save_local_or_in_reg_p): Adjust comment.
	(emit_save_register_window): Likewise.
	(sparc_expand_prologue): Use SIZE_INT_RTX and SIZE_RTX variables.
	(sparc_flat_expand_prologue): Add comment.  Always emit blockage.
	Swap back %o7/%i7 in register naming.


-- 
Eric Botcazou

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

Index: config/sparc/sparc.c
===================================================================
--- config/sparc/sparc.c	(revision 175398)
+++ config/sparc/sparc.c	(working copy)
@@ -4149,7 +4149,7 @@ save_local_or_in_reg_p (unsigned int reg
   if (regno == RETURN_ADDR_REGNUM && return_addr_reg_needed_p (leaf_function))
     return true;
 
-  /* PIC register (%l7) if needed.  */
+  /* GOT register (%l7) if needed.  */
   if (regno == PIC_OFFSET_TABLE_REGNUM && crtl->uses_pic_offset_table)
     return true;
 
@@ -4600,11 +4600,12 @@ emit_save_register_window (rtx increment
   insn = emit_insn (gen_save_register_window_1 (increment));
   RTX_FRAME_RELATED_P (insn) = 1;
 
-  /* The return address (%i7) is saved in %o7.  */
+  /* The incoming return address (%o7) is saved in %i7.  */
   add_reg_note (insn, REG_CFA_REGISTER,
 		gen_rtx_SET (VOIDmode,
 			     gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM),
-			     gen_rtx_REG (Pmode, INCOMING_RETURN_ADDR_REGNUM)));
+			     gen_rtx_REG (Pmode,
+					  INCOMING_RETURN_ADDR_REGNUM)));
 
   /* The window save event.  */
   add_reg_note (insn, REG_CFA_WINDOW_SAVE, const0_rtx);
@@ -4688,8 +4689,10 @@ sparc_expand_prologue (void)
     ; /* do nothing.  */
   else if (sparc_leaf_function_p)
     {
+      rtx size_int_rtx = GEN_INT (-size);
+
       if (size <= 4096)
-	insn = emit_insn (gen_stack_pointer_inc (GEN_INT (-size)));
+	insn = emit_insn (gen_stack_pointer_inc (size_int_rtx));
       else if (size <= 8192)
 	{
 	  insn = emit_insn (gen_stack_pointer_inc (GEN_INT (-4096)));
@@ -4699,19 +4702,21 @@ sparc_expand_prologue (void)
 	}
       else
 	{
-	  rtx reg = gen_rtx_REG (Pmode, 1);
-	  emit_move_insn (reg, GEN_INT (-size));
-	  insn = emit_insn (gen_stack_pointer_inc (reg));
+	  rtx size_rtx = gen_rtx_REG (Pmode, 1);
+	  emit_move_insn (size_rtx, size_int_rtx);
+	  insn = emit_insn (gen_stack_pointer_inc (size_rtx));
 	  add_reg_note (insn, REG_FRAME_RELATED_EXPR,
-			gen_stack_pointer_inc (GEN_INT (-size)));
+			gen_stack_pointer_inc (size_int_rtx));
 	}
 
       RTX_FRAME_RELATED_P (insn) = 1;
     }
   else
     {
+      rtx size_int_rtx = GEN_INT (-size);
+
       if (size <= 4096)
-	emit_save_register_window (GEN_INT (-size));
+	emit_save_register_window (size_int_rtx);
       else if (size <= 8192)
 	{
 	  emit_save_register_window (GEN_INT (-4096));
@@ -4720,9 +4725,9 @@ sparc_expand_prologue (void)
 	}
       else
 	{
-	  rtx reg = gen_rtx_REG (Pmode, 1);
-	  emit_move_insn (reg, GEN_INT (-size));
-	  emit_save_register_window (reg);
+	  rtx size_rtx = gen_rtx_REG (Pmode, 1);
+	  emit_move_insn (size_rtx, size_int_rtx);
+	  emit_save_register_window (size_rtx);
 	}
     }
 
@@ -4783,6 +4788,10 @@ sparc_flat_expand_prologue (void)
 
       size_rtx = size_int_rtx = GEN_INT (-size);
 
+      /* We establish the frame (i.e. decrement the stack pointer) first, even
+	 if we use a frame pointer, because we cannot clobber any call-saved
+	 registers, including the frame pointer, if we haven't created a new
+	 register save area, for the sake of compatibility with the ABI.  */
       if (size <= 4096)
 	insn = emit_insn (gen_stack_pointer_inc (size_int_rtx));
       else if (size <= 8192 && !frame_pointer_needed)
@@ -4801,6 +4810,9 @@ sparc_flat_expand_prologue (void)
 	}
       RTX_FRAME_RELATED_P (insn) = 1;
 
+      /* Ensure nothing is scheduled until after the frame is established.  */
+      emit_insn (gen_blockage ());
+
       if (frame_pointer_needed)
 	{
 	  insn = emit_insn (gen_rtx_SET (VOIDmode, hard_frame_pointer_rtx,
@@ -4813,26 +4825,22 @@ sparc_flat_expand_prologue (void)
 			gen_rtx_SET (VOIDmode, hard_frame_pointer_rtx,
 				     plus_constant (stack_pointer_rtx,
 						    size)));
-
-	  /* Make sure nothing is scheduled until after the frame
-	     is established.  */
-	  emit_insn (gen_blockage ());
 	}
 
       if (return_addr_reg_needed_p (sparc_leaf_function_p))
 	{
-	  rtx i7 = gen_rtx_REG (Pmode, INCOMING_RETURN_ADDR_REGNUM);
-	  rtx o7 = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
+	  rtx o7 = gen_rtx_REG (Pmode, INCOMING_RETURN_ADDR_REGNUM);
+	  rtx i7 = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
 
-	  insn = emit_move_insn (o7, i7);
+	  insn = emit_move_insn (i7, o7);
 	  RTX_FRAME_RELATED_P (insn) = 1;
 
 	  add_reg_note (insn, REG_CFA_REGISTER,
-			gen_rtx_SET (VOIDmode, o7, i7));
+			gen_rtx_SET (VOIDmode, i7, o7));
 
 	  /* Prevent this instruction from ever being considered dead,
 	     even if this function has no epilogue.  */
-	  emit_insn (gen_rtx_USE (VOIDmode, o7));
+	  emit_insn (gen_rtx_USE (VOIDmode, i7));
 	}
     }
 

Re: [RFC] Fix unwind info for sparc -mflat

[Eric Botcazou]

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

> I have 4 regressions in the C testsuite with -mflat on SPARC/Solaris:

And 2 regressions in 64-bit mode:

FAIL: gcc.c-torture/execute/nestfunc-6.c execution,  -O1
[...]

FAIL: gcc.dg/torture/stackalign/nested-6.c  -O1  execution test
[...]


Another latent problem exposed by the change.  Tested on SPARC/Solaris, applied 
on the mainline.


2011-06-27  Eric Botcazou  <ebotcazou@adacore.com>

        * config/sparc/sparc.c (sparc_frame_pointer_required): Return true if
	the function receives nonlocal gotos.


-- 
Eric Botcazou

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

Index: config/sparc/sparc.c
===================================================================
--- config/sparc/sparc.c	(revision 175408)
+++ config/sparc/sparc.c	(working copy)
@@ -10153,6 +10153,11 @@ sparc_frame_pointer_required (void)
   if (cfun->calls_alloca)
     return true;
 
+  /* If the function receives nonlocal gotos, it needs to save the frame
+     pointer in the nonlocal_goto_save_area object.  */
+  if (cfun->has_nonlocal_label)
+    return true;
+
   /* In flat mode, that's it.  */
   if (TARGET_FLAT)
     return false;