Patchwork Add unwind information to mips epilogues

login
register
mail settings
Submitter Bernd Schmidt
Date Sept. 7, 2011, 10:28 a.m.
Message ID <4E67474E.1070106@codesourcery.com>
Download mbox | patch
Permalink /patch/113734/
State New
Headers show

Comments

Bernd Schmidt - Sept. 7, 2011, 10:28 a.m.
Here's a new version, which adds support for mips16 and tries to avoid
the window with the frame pointer restore.

Testing mips16 is problematic, all the execute tests fail before and
after - I interpret one of your earlier mails to say that this is
expected. There are no new compilation failures with this patch, but
incorrect earlier versions triggered a few, indicating that the testing
is at least somewhat useful.

I get the following output for restore insns:

        move    $sp,$17
        restore 8,$16,$17
$LCFI2 = .
        .cfi_remember_state
        .cfi_restore 16
        .cfi_restore 17
        .cfi_def_cfa 29, 0

which I think is correct.

Question for Richard H.: What is this actually good for, other than
presenting consistent information to dwarf2cfi? Do we actually expect
code to unwind through the middle of an epilogue?


Bernd
* config/mips/mips.c (cfa_restores, cfa_sp_offset): New static
	variables.
	(mips16e_save_restore_reg): Add to cfa_restores.
	(mips_restore_reg): Add to cfa_restores, and add a REG_CFA_DEF_CFA
	note to insns restoring the frame pointer.
	(mips_expand_epilogue): Initialize the new variables.  Annotate RTL
	to ensure dwarf2cfi sees the effects of the epilogue.  Generate a
	simple_return only if the return address is in r31.
Richard Guenther - Sept. 7, 2011, 10:54 a.m.
On Wed, Sep 7, 2011 at 12:28 PM, Bernd Schmidt <bernds@codesourcery.com> wrote:
> Here's a new version, which adds support for mips16 and tries to avoid
> the window with the frame pointer restore.
>
> Testing mips16 is problematic, all the execute tests fail before and
> after - I interpret one of your earlier mails to say that this is
> expected. There are no new compilation failures with this patch, but
> incorrect earlier versions triggered a few, indicating that the testing
> is at least somewhat useful.
>
> I get the following output for restore insns:
>
>        move    $sp,$17
>        restore 8,$16,$17
> $LCFI2 = .
>        .cfi_remember_state
>        .cfi_restore 16
>        .cfi_restore 17
>        .cfi_def_cfa 29, 0
>
> which I think is correct.
>
> Question for Richard H.: What is this actually good for, other than
> presenting consistent information to dwarf2cfi? Do we actually expect
> code to unwind through the middle of an epilogue?

With async signals we can at least get interrupted in the middle of an
epilogue.  What you are allowed to do here (throw an exception?
perform manual unwinding? use gdb which unwinds?) is another
question.  ISTR customer requests for this feature at least (which
I think was doing profiling stuff).

Richard.

>
> Bernd
>
Joseph S. Myers - Sept. 7, 2011, 11:35 a.m.
On Wed, 7 Sep 2011, Richard Guenther wrote:

> With async signals we can at least get interrupted in the middle of an
> epilogue.  What you are allowed to do here (throw an exception?
> perform manual unwinding? use gdb which unwinds?) is another
> question.  ISTR customer requests for this feature at least (which
> I think was doing profiling stuff).

Profiling this way is discussed in Nathan Froyd et al's paper in the 2006 
Summit proceedings.
Jakub Jelinek - Sept. 7, 2011, 12:04 p.m.
On Wed, Sep 07, 2011 at 12:28:30PM +0200, Bernd Schmidt wrote:
> Question for Richard H.: What is this actually good for, other than
> presenting consistent information to dwarf2cfi? Do we actually expect
> code to unwind through the middle of an epilogue?

With -fasynchronous-unwind-tables and e.g. async cancellation enabled
pthread_signal may be handled anywhere.  There have been several bug reports
e.g. against glibc, which is why Richard implemented the epilogue unwinding
on various targets.  Additionally, if the debugger uses the unwind info,
when you step through the epilogue, you want precise unwind info too.

	Jakub
Richard Sandiford - Sept. 8, 2011, 2:08 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>      emit_insn (gen_blockage ());
>  }
>  
> -/* Emit instructions to restore register REG from slot MEM.  */
> +/* Emit instructions to restore register REG from slot MEM.  Also update
> +   the cfa_restores list.  */
>  
>  static void
>  mips_restore_reg (rtx reg, rtx mem)
>  {
> +  rtx orig_reg = reg;
> +  rtx last;
> +
>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>       $7 instead and adjust the return insn appropriately.  */
>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>  
>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
> +  if (reg == orig_reg)
> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
> +
> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
> +    return;
> +  last = get_last_insn ();
> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
> +						      cfa_sp_offset));
> +  RTX_FRAME_RELATED_P (last) = 1;

I suppose I still don't get why this is OK but this:

> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>        if (!TARGET_MIPS16)
>  	target = stack_pointer_rtx;
>  
> -      emit_insn (gen_add3_insn (target, base, adjust));
> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
> +	{
> +	  RTX_FRAME_RELATED_P (insn) = 1;
> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
> +			plus_constant (stack_pointer_rtx, step2));
> +	}

triggered ICEs without the !frame_pointer_required.  I think I need
to play around with it a bit before I understand enough to review.
I'll try to find time this weekend.

Also, this:

@@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
 	}
       else
 	{
-	  unsigned int regno;
+	  rtx pat;
 
 	  /* When generating MIPS16 code, the normal
 	     mips_for_each_saved_gpr_and_fpr path will restore the return
@@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
 	  if (TARGET_MIPS16
 	      && !GENERATE_MIPS16E_SAVE_RESTORE
 	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
-	    regno = GP_REG_FIRST + 7;
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
+	      pat = gen_return_internal (reg);
+	    }
 	  else
-	    regno = RETURN_ADDR_REGNUM;
-	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
-								   regno)));
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
+	      pat = gen_simple_return_internal (reg);
+	    }
+	  emit_jump_insn (pat);
 	}
     }
 
looks like a logically separate change.  I'm not sure I understand
why it's needed: both returns are simple returns in the rtx sense.

Sorry for being awkward here :-(

Richard
Bernd Schmidt - Sept. 8, 2011, 2:17 p.m.
On 09/08/11 16:08, Richard Sandiford wrote:
> Also, this:
> 
> @@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
>  	}
>        else
>  	{
> -	  unsigned int regno;
> +	  rtx pat;
>  
>  	  /* When generating MIPS16 code, the normal
>  	     mips_for_each_saved_gpr_and_fpr path will restore the return
> @@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
>  	  if (TARGET_MIPS16
>  	      && !GENERATE_MIPS16E_SAVE_RESTORE
>  	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
> -	    regno = GP_REG_FIRST + 7;
> +	    {
> +	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
> +	      pat = gen_return_internal (reg);
> +	    }
>  	  else
> -	    regno = RETURN_ADDR_REGNUM;
> -	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
> -								   regno)));
> +	    {
> +	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
> +	      pat = gen_simple_return_internal (reg);
> +	    }
> +	  emit_jump_insn (pat);
>  	}
>      }
>  
> looks like a logically separate change.  I'm not sure I understand
> why it's needed: both returns are simple returns in the rtx sense.

Should have explained that bit - it's needed only with shrink-wrapping.
If we find that the epilogue ends in a simple_return, and we need
conditional simple_returns elsewhere and don't have a pattern for them,
we try to reuse the epilogue's simple_return by placing a label in
front of it and redirecting these conditional simple_returns there. This
doesn't work if we need to use different return registers, and a simple
way to prevent it is to make the last insn look like a normal return
instead.


Bernd
Richard Sandiford - Sept. 8, 2011, 2:39 p.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/08/11 16:08, Richard Sandiford wrote:
>> Also, this:
>> 
>> @@ -10442,7 +10495,7 @@ mips_expand_epilogue (bool sibcall_p)
>>  	}
>>        else
>>  	{
>> -	  unsigned int regno;
>> +	  rtx pat;
>>  
>>  	  /* When generating MIPS16 code, the normal
>>  	     mips_for_each_saved_gpr_and_fpr path will restore the return
>> @@ -10450,11 +10503,16 @@ mips_expand_epilogue (bool sibcall_p)
>>  	  if (TARGET_MIPS16
>>  	      && !GENERATE_MIPS16E_SAVE_RESTORE
>>  	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
>> -	    regno = GP_REG_FIRST + 7;
>> +	    {
>> +	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
>> +	      pat = gen_return_internal (reg);
>> +	    }
>>  	  else
>> -	    regno = RETURN_ADDR_REGNUM;
>> -	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
>> -								   regno)));
>> +	    {
>> +	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
>> +	      pat = gen_simple_return_internal (reg);
>> +	    }
>> +	  emit_jump_insn (pat);
>>  	}
>>      }
>>  
>> looks like a logically separate change.  I'm not sure I understand
>> why it's needed: both returns are simple returns in the rtx sense.
>
> Should have explained that bit - it's needed only with shrink-wrapping.
> If we find that the epilogue ends in a simple_return, and we need
> conditional simple_returns elsewhere and don't have a pattern for them,
> we try to reuse the epilogue's simple_return by placing a label in
> front of it and redirecting these conditional simple_returns there. This
> doesn't work if we need to use different return registers, and a simple
> way to prevent it is to make the last insn look like a normal return
> instead.

Ah!  Yeah.  So this part is OK on its own with a comment like:

	      /* simple_returns cannot rely on values that are only available
		 on paths through the epilogue (because return paths that do
		 not pass through the epilogue may nevertheless reuse a
		 simple_return that occurs at the end of the epilogue).
		 Use a normal return here instead.  */
	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
	      pat = gen_return_internal (reg);

Thanks,
Richard
Richard Henderson - Sept. 8, 2011, 3:41 p.m.
On 09/08/2011 03:08 PM, Richard Sandiford wrote:
> Bernd Schmidt <bernds@codesourcery.com> writes:
>> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>>      emit_insn (gen_blockage ());
>>  }
>>  
>> -/* Emit instructions to restore register REG from slot MEM.  */
>> +/* Emit instructions to restore register REG from slot MEM.  Also update
>> +   the cfa_restores list.  */
>>  
>>  static void
>>  mips_restore_reg (rtx reg, rtx mem)
>>  {
>> +  rtx orig_reg = reg;
>> +  rtx last;
>> +
>>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>>       $7 instead and adjust the return insn appropriately.  */
>>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>>  
>>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
>> +  if (reg == orig_reg)
>> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
>> +
>> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
>> +    return;
>> +  last = get_last_insn ();
>> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
>> +						      cfa_sp_offset));
>> +  RTX_FRAME_RELATED_P (last) = 1;
> 
> I suppose I still don't get why this is OK but this:
> 
>> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>>        if (!TARGET_MIPS16)
>>  	target = stack_pointer_rtx;
>>  
>> -      emit_insn (gen_add3_insn (target, base, adjust));
>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>> +	{
>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>> +			plus_constant (stack_pointer_rtx, step2));
>> +	}
> 
> triggered ICEs without the !frame_pointer_required.  I think I need
> to play around with it a bit before I understand enough to review.
> I'll try to find time this weekend.

I'd be curious to understand in what situation the second hunk causes
failures without frame_pointer_required.  But as I'm supposed to be on
holiday atm I can't spend any real time on it til I get back.  ;-)

That said, the first hunk causes the CFA to get swapped back to the
stack pointer on the very insn that restores the frame pointer.

Which to me looks to do the right thing in all situations.  Is there
a different window you are worried about?


r~
Richard Sandiford - Sept. 8, 2011, 4:14 p.m.
Richard Henderson <rth@redhat.com> writes:
> On 09/08/2011 03:08 PM, Richard Sandiford wrote:
>> Bernd Schmidt <bernds@codesourcery.com> writes:
>>> @@ -10227,17 +10236,30 @@ mips_expand_prologue (void)
>>>      emit_insn (gen_blockage ());
>>>  }
>>>  
>>> -/* Emit instructions to restore register REG from slot MEM.  */
>>> +/* Emit instructions to restore register REG from slot MEM.  Also update
>>> +   the cfa_restores list.  */
>>>  
>>>  static void
>>>  mips_restore_reg (rtx reg, rtx mem)
>>>  {
>>> +  rtx orig_reg = reg;
>>> +  rtx last;
>>> +
>>>    /* There's no MIPS16 instruction to load $31 directly.  Load into
>>>       $7 instead and adjust the return insn appropriately.  */
>>>    if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
>>>      reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
>>>  
>>>    mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
>>> +  if (reg == orig_reg)
>>> +    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
>>> +
>>> +  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
>>> +    return;
>>> +  last = get_last_insn ();
>>> +  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
>>> +						      cfa_sp_offset));
>>> +  RTX_FRAME_RELATED_P (last) = 1;
>> 
>> I suppose I still don't get why this is OK but this:
>> 
>>> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>>>        if (!TARGET_MIPS16)
>>>  	target = stack_pointer_rtx;
>>>  
>>> -      emit_insn (gen_add3_insn (target, base, adjust));
>>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>>> +	{
>>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>>> +			plus_constant (stack_pointer_rtx, step2));
>>> +	}
>> 
>> triggered ICEs without the !frame_pointer_required.  I think I need
>> to play around with it a bit before I understand enough to review.
>> I'll try to find time this weekend.
>
> I'd be curious to understand in what situation the second hunk causes
> failures without frame_pointer_required.  But as I'm supposed to be on
> holiday atm I can't spend any real time on it til I get back.  ;-)
>
> That said, the first hunk causes the CFA to get swapped back to the
> stack pointer on the very insn that restores the frame pointer.
>
> Which to me looks to do the right thing in all situations.  Is there
> a different window you are worried about?

Nah, nothing like that.  I agree the new patch is setting the CFA at the
right time.  It's just that if those !frame_pointer_needed tests are
needed to avoid ICEs, then it feels like there's still some magic that
I don't understand.

(I'm not saying the checks should be removed from the new patch --
at best it would achieve nothing, and at worst it would bloat the
dwarf2 output.  I'm just curious.)

Richard
Bernd Schmidt - Sept. 9, 2011, 12:21 p.m.
On 09/08/11 16:08, Richard Sandiford wrote:
> I suppose I still don't get why this is OK but this:
> 
>> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>>        if (!TARGET_MIPS16)
>>  	target = stack_pointer_rtx;
>>  
>> -      emit_insn (gen_add3_insn (target, base, adjust));
>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>> +	{
>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>> +			plus_constant (stack_pointer_rtx, step2));
>> +	}
> 
> triggered ICEs without the !frame_pointer_required.  I think I need
> to play around with it a bit before I understand enough to review.
> I'll try to find time this weekend.

I've rebuilt and tested it again (shrink-wrapping included, not sure if
it makes a difference). What seems to happen is that we have

(insn/f 60 59 61 2 (set (reg/f:SI 30 $fp) (reg/f:SI 29 $sp))

(note 61 60 8 2 NOTE_INSN_PROLOGUE_END)

(jump_insn 8 61 49 2 (set (pc)
        (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
                (reg:SI 2 $2 [198]))
            (label_ref:SI 29) (pc)))

[...]

(jump_insn 10 9 48 3 (set (pc)
        (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
                (reg:SI 2 $2 [199]))
            (label_ref:SI 29) (pc)))

(note 48 10 62 4 [bb 4] NOTE_INSN_BASIC_BLOCK)

(note 62 48 63 4 NOTE_INSN_EPILOGUE_BEG)

(insn/f 63 62 64 4 (set (reg/f:SI 29 $sp) (reg/f:SI 30 $fp))
     (REG_CFA_DEF_CFA (plus:SI (reg/f:SI 29 $sp) (const_int 8))

and reorg.c manages to hoist insn 63 into a delay slot for jump_insn 10.
Presumably it saw insn 60 somehow? In any case, we can now arrive at
label 29 with either FP or SP as CFA reg.

This is gcc.dg/pr49994-1.c.


Bernd
Richard Sandiford - Sept. 10, 2011, 11:12 a.m.
Bernd Schmidt <bernds@codesourcery.com> writes:
> On 09/08/11 16:08, Richard Sandiford wrote:
>> I suppose I still don't get why this is OK but this:
>> 
>>> @@ -10324,12 +10350,26 @@ mips_expand_epilogue (bool sibcall_p)
>>>        if (!TARGET_MIPS16)
>>>  	target = stack_pointer_rtx;
>>>  
>>> -      emit_insn (gen_add3_insn (target, base, adjust));
>>> +      insn = emit_insn (gen_add3_insn (target, base, adjust));
>>> +      if (!frame_pointer_needed && target == stack_pointer_rtx)
>>> +	{
>>> +	  RTX_FRAME_RELATED_P (insn) = 1;
>>> +	  add_reg_note (insn, REG_CFA_DEF_CFA,
>>> +			plus_constant (stack_pointer_rtx, step2));
>>> +	}
>> 
>> triggered ICEs without the !frame_pointer_required.  I think I need
>> to play around with it a bit before I understand enough to review.
>> I'll try to find time this weekend.
>
> I've rebuilt and tested it again (shrink-wrapping included, not sure if
> it makes a difference). What seems to happen is that we have
>
> (insn/f 60 59 61 2 (set (reg/f:SI 30 $fp) (reg/f:SI 29 $sp))
>
> (note 61 60 8 2 NOTE_INSN_PROLOGUE_END)
>
> (jump_insn 8 61 49 2 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [198]))
>             (label_ref:SI 29) (pc)))
>
> [...]
>
> (jump_insn 10 9 48 3 (set (pc)
>         (if_then_else (eq (reg/v:SI 4 $4 [orig:196 b ] [196])
>                 (reg:SI 2 $2 [199]))
>             (label_ref:SI 29) (pc)))
>
> (note 48 10 62 4 [bb 4] NOTE_INSN_BASIC_BLOCK)
>
> (note 62 48 63 4 NOTE_INSN_EPILOGUE_BEG)
>
> (insn/f 63 62 64 4 (set (reg/f:SI 29 $sp) (reg/f:SI 30 $fp))
>      (REG_CFA_DEF_CFA (plus:SI (reg/f:SI 29 $sp) (const_int 8))
>
> and reorg.c manages to hoist insn 63 into a delay slot for jump_insn 10.
> Presumably it saw insn 60 somehow? In any case, we can now arrive at
> label 29 with either FP or SP as CFA reg.
>
> This is gcc.dg/pr49994-1.c.

Thanks.  I suppose the transformation would be valid if it really had
seen insn 60, but it looks like:

(insn 39 38 40 (set (reg/f:SI 29 $sp)
        (mem:SI (plus:SI (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
                (const_int 4 [0x4])) [0 S4 A32])) ../../../gcc/gcc/testsuite/gcc.dg/pr49994-1.c:14 278 {*movsi_internal}
     (expr_list:REG_DEAD (reg/f:SI 15 $15 [orig:197 CHAIN.1 ] [197])
        (nil)))

stops resource.c from considering the stack pointer to be live on
fallthrough, and so reorg.c is simply speculating a move to what
it thinks is a dead register.  Which seems like a bug.

(It's probably safe in this example, but we haven't done the work to
prove that.  In particular, we haven't proven that the MEM in insn 39
isn't an access to the current frame.)

But like you say, reorg.c does analyse register contents to some extent,
and it's hard to prove that it wouldn't be able to pull the same trick
for valid reasons.  Should we simply forbid speculation of frame-related
instructions?  E.g. by replacing the may_trap_or_fault_p calls with something
that also checks RTX_FRAME_RELATED_P?  (We'd still be able to use frame-
related insns for annulled and always-true branches.)  Or is that too
big a hammer?

Richard

Patch

Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c	(revision 178135)
+++ gcc/config/mips/mips.c	(working copy)
@@ -501,6 +501,11 @@  int sdb_label_count;
 int mips_dbx_regno[FIRST_PSEUDO_REGISTER];
 int mips_dwarf_regno[FIRST_PSEUDO_REGISTER];
 
+/* Used to collect REG_CFA notes during epilogue generation.  */
+static rtx cfa_restores = NULL_RTX;
+/* Used to produce REG_CFA_DEF_CFA notes during epilogue generation.  */
+static HOST_WIDE_INT cfa_sp_offset;
+
 /* The nesting depth of the PRINT_OPERAND '%(', '%<' and '%[' constructs.  */
 struct mips_asm_switch mips_noreorder = { "reorder", 0 };
 struct mips_asm_switch mips_nomacro = { "macro", 0 };
@@ -8371,8 +8376,8 @@  mips16e_collect_argument_saves (void)
 }
 
 /* Return a move between register REGNO and memory location SP + OFFSET.
-   Make the move a load if RESTORE_P, otherwise make it a frame-related
-   store.  */
+   Make the move a load and update cfa_restores if RESTORE_P, otherwise
+   make it a frame-related store.  */
 
 static rtx
 mips16e_save_restore_reg (bool restore_p, HOST_WIDE_INT offset,
@@ -8382,6 +8387,9 @@  mips16e_save_restore_reg (bool restore_p
 
   mem = gen_frame_mem (SImode, plus_constant (stack_pointer_rtx, offset));
   reg = gen_rtx_REG (SImode, regno);
+  if (restore_p)
+    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
+
   return (restore_p
 	  ? gen_rtx_SET (VOIDmode, reg, mem)
 	  : mips_frame_set (mem, reg));
@@ -10227,17 +10236,30 @@  mips_expand_prologue (void)
     emit_insn (gen_blockage ());
 }
 
-/* Emit instructions to restore register REG from slot MEM.  */
+/* Emit instructions to restore register REG from slot MEM.  Also update
+   the cfa_restores list.  */
 
 static void
 mips_restore_reg (rtx reg, rtx mem)
 {
+  rtx orig_reg = reg;
+  rtx last;
+
   /* There's no MIPS16 instruction to load $31 directly.  Load into
      $7 instead and adjust the return insn appropriately.  */
   if (TARGET_MIPS16 && REGNO (reg) == RETURN_ADDR_REGNUM)
     reg = gen_rtx_REG (GET_MODE (reg), GP_REG_FIRST + 7);
 
   mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
+  if (reg == orig_reg)
+    cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg, cfa_restores);
+
+  if (!frame_pointer_needed || REGNO (reg) != HARD_FRAME_POINTER_REGNUM)
+    return;
+  last = get_last_insn ();
+  add_reg_note (last, REG_CFA_DEF_CFA, plus_constant (stack_pointer_rtx,
+						      cfa_sp_offset));
+  RTX_FRAME_RELATED_P (last) = 1;
 }
 
 /* Emit any instructions needed before a return.  */
@@ -10268,6 +10290,8 @@  mips_expand_epilogue (bool sibcall_p)
   HOST_WIDE_INT step1, step2;
   rtx base, target, insn;
 
+  cfa_restores = NULL_RTX;
+
   if (!sibcall_p && mips_can_use_return_insn ())
     {
       emit_jump_insn (gen_return ());
@@ -10306,6 +10330,8 @@  mips_expand_epilogue (bool sibcall_p)
       step1 -= step2;
     }
 
+  cfa_sp_offset = step2;
+
   /* Set TARGET to BASE + STEP1.  */
   target = base;
   if (step1 > 0)
@@ -10324,12 +10350,26 @@  mips_expand_epilogue (bool sibcall_p)
       if (!TARGET_MIPS16)
 	target = stack_pointer_rtx;
 
-      emit_insn (gen_add3_insn (target, base, adjust));
+      insn = emit_insn (gen_add3_insn (target, base, adjust));
+      if (!frame_pointer_needed && target == stack_pointer_rtx)
+	{
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	  add_reg_note (insn, REG_CFA_DEF_CFA,
+			plus_constant (stack_pointer_rtx, step2));
+	}
     }
 
   /* Copy TARGET into the stack pointer.  */
   if (target != stack_pointer_rtx)
-    mips_emit_move (stack_pointer_rtx, target);
+    {
+      insn = mips_emit_move (stack_pointer_rtx, target);
+      if (!frame_pointer_needed)
+	{
+	  add_reg_note (insn, REG_CFA_DEF_CFA,
+			plus_constant (stack_pointer_rtx, step2));
+	  RTX_FRAME_RELATED_P (insn) = 1;
+	}
+    }
 
   /* If we're using addressing macros, $gp is implicitly used by all
      SYMBOL_REFs.  We must emit a blockage insn before restoring $gp
@@ -10357,7 +10397,10 @@  mips_expand_epilogue (bool sibcall_p)
 
       /* Restore the remaining registers and deallocate the final bit
 	 of the frame.  */
-      emit_insn (restore);
+      restore = emit_insn (restore);
+      REG_NOTES (restore) = cfa_restores;
+      RTX_FRAME_RELATED_P (restore) = 1;
+      add_reg_note (restore, REG_CFA_DEF_CFA, stack_pointer_rtx);
     }
   else
     {
@@ -10393,9 +10436,14 @@  mips_expand_epilogue (bool sibcall_p)
 
 	  /* If we don't use shoadow register set, we need to update SP.  */
 	  if (!cfun->machine->use_shadow_register_set_p && step2 > 0)
-	    emit_insn (gen_add3_insn (stack_pointer_rtx,
-				      stack_pointer_rtx,
-				      GEN_INT (step2)));
+	    {
+	      insn = emit_insn (gen_add3_insn (stack_pointer_rtx,
+					       stack_pointer_rtx,
+					       GEN_INT (step2)));
+	      REG_NOTES (insn) = cfa_restores;
+	      RTX_FRAME_RELATED_P (insn) = 1;
+	      add_reg_note (insn, REG_CFA_DEF_CFA, stack_pointer_rtx);
+	    }
 
 	  /* Move to COP0 Status.  */
 	  emit_insn (gen_cop0_move (gen_rtx_REG (SImode, COP0_STATUS_REG_NUM),
@@ -10405,9 +10453,14 @@  mips_expand_epilogue (bool sibcall_p)
 	{
 	  /* Deallocate the final bit of the frame.  */
 	  if (step2 > 0)
-	    emit_insn (gen_add3_insn (stack_pointer_rtx,
-				      stack_pointer_rtx,
-				      GEN_INT (step2)));
+	    {
+	      insn = emit_insn (gen_add3_insn (stack_pointer_rtx,
+					       stack_pointer_rtx,
+					       GEN_INT (step2)));
+	      REG_NOTES (insn) = cfa_restores;
+	      RTX_FRAME_RELATED_P (insn) = 1;	
+	      add_reg_note (insn, REG_CFA_DEF_CFA, stack_pointer_rtx);
+	    }
 	}
     }
 
@@ -10442,7 +10495,7 @@  mips_expand_epilogue (bool sibcall_p)
 	}
       else
 	{
-	  unsigned int regno;
+	  rtx pat;
 
 	  /* When generating MIPS16 code, the normal
 	     mips_for_each_saved_gpr_and_fpr path will restore the return
@@ -10450,11 +10503,16 @@  mips_expand_epilogue (bool sibcall_p)
 	  if (TARGET_MIPS16
 	      && !GENERATE_MIPS16E_SAVE_RESTORE
 	      && BITSET_P (frame->mask, RETURN_ADDR_REGNUM))
-	    regno = GP_REG_FIRST + 7;
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, GP_REG_FIRST + 7);
+	      pat = gen_return_internal (reg);
+	    }
 	  else
-	    regno = RETURN_ADDR_REGNUM;
-	  emit_jump_insn (gen_simple_return_internal (gen_rtx_REG (Pmode,
-								   regno)));
+	    {
+	      rtx reg = gen_rtx_REG (Pmode, RETURN_ADDR_REGNUM);
+	      pat = gen_simple_return_internal (reg);
+	    }
+	  emit_jump_insn (pat);
 	}
     }