This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: Add unwind information to mips epilogues
Richard Sandiford <richard.sandiford@linaro.org> writes:
> 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.
Does the attached patch look OK? It should fix a couple of things.
First, on MIPS16, this code:
/* Set TARGET to BASE + STEP1. */
target = base;
if (step1 > 0)
{
rtx adjust;
/* Get an rtx for STEP1 that we can add to BASE. */
adjust = GEN_INT (step1);
if (!SMALL_OPERAND (step1))
{
mips_emit_move (MIPS_EPILOGUE_TEMP (Pmode), adjust);
adjust = MIPS_EPILOGUE_TEMP (Pmode);
}
/* Normal mode code can copy the result straight into $sp. */
if (!TARGET_MIPS16)
target = stack_pointer_rtx;
emit_insn (gen_add3_insn (target, base, adjust));
}
adds STEP1 to the frame pointer, then:
/* Copy TARGET into the stack pointer. */
if (target != stack_pointer_rtx)
mips_emit_move (stack_pointer_rtx, target);
moves it to the stack pointer. So there's a window in which the frame
pointer would have been adjusted, but the CFA is still defined in terms
of the old frame pointer. I think:
emit_insn (gen_add3_insn (target, base, adjust));
always needs a DEF_CFA note for TARGET + STEP2. For MIPS16, this will
redefine the CFA in terms of the new frame pointer, otherwise it will
define the CFA in terms of the stack pointer. It's not necessary to
do this immediately for frame_pointer_needed && !TARGET_MIPS16, but we'd
eventually emit the same thing when restoring the frame pointer, so we
might as well do it here.
With that change, we can drop the DEF_CFA for:
/* Copy TARGET into the stack pointer. */
if (target != stack_pointer_rtx)
mips_emit_move (stack_pointer_rtx, target);
and simply set it when restoring the frame pointer, just as for
non-MIPS16 code.
Second thing is that we never emitted .cfi_restores for interrupt
handlers that use the shadow register set (which don't need to
deallocate the stack).
I tested this with the other shrink-wrapping patches on mips64-linux-gnu
using --with-arch=mips64 (-mabi=32, -mabi=64, -mabi=n32 and -mips16/-mabi=32,
which tests SAVE and RESTORE). The results were good apart from:
FAIL: gcc.c-torture/compile/iftrap-1.c -O3 -g (internal compiler error)
FAIL: gcc.c-torture/compile/iftrap-1.c -O3 -g (test for excess errors)
when testing with -mips16/-mabi=32. Not sure if you've already
seen/mentioned this or not, but just in case: the ICE is coming from:
void f8(int p)
{
if (p)
__builtin_trap();
else
{
bar();
__builtin_trap();
}
}
When generating prologues and epilogues, there are still two trap
instructions (each with only fake edges to the exit block, since
the trap is unconditional). We decide to shrink-wrap the "else",
meaning that the two traps have different CFAs. Then the cfg cleanups
in csa notice that we have two basic blocks that end in traps,
and decides to redirect the "then" branch to the end of the "else".
That's correct execution-wise, but it means we have two incoming edges
with different CFAs.
Richard
gcc/
2011-09-11 Bernd Schmidt <bernds@codesourcery.com>
Richard Sandiford <rdsandiford@googlemail.com>
* config/mips/mips.c (mips_epilogue): New structure.
(mips16e_save_restore_reg): Queue REG_CFA_RESTORE notes when
restoring registers.
(mips_epilogue_emit_cfa_restores): New function.
(mips_epilogue_set_cfa): Likewise.
(mips_restore_reg): Queue REG_CFA_RESTORE notes. When restoring
the current CFA register from the stack, redefine the CFA in terms
of the stack pointer.
(mips_expand_epilogue): Set up mips_epilogue. Attach CFA information
to the epilogue instructions.
gcc/testsuite/
* gcc.target/mips/mips.exp (mips_option_groups): Add debug options.
* gcc.target/mips/interrupt_handler-2.c: New test.
* gcc.target/mips/interrupt_handler-3.c: Likewise.
Index: gcc/config/mips/mips.c
===================================================================
--- gcc/config/mips/mips.c 2011-09-10 09:30:00.000000000 +0100
+++ gcc/config/mips/mips.c 2011-09-11 09:07:47.000000000 +0100
@@ -501,6 +501,21 @@ const char *current_function_file = "";
int mips_dbx_regno[FIRST_PSEUDO_REGISTER];
int mips_dwarf_regno[FIRST_PSEUDO_REGISTER];
+/* Information about the current function's epilogue, used only while
+ expanding it. */
+static struct {
+ /* A list of queued REG_CFA_RESTORE notes. */
+ rtx cfa_restores;
+
+ /* The CFA is currently defined as CFA_REG + CFA_OFFSET. */
+ rtx cfa_reg;
+ HOST_WIDE_INT cfa_offset;
+
+ /* The offset of the CFA from the stack pointer while restoring
+ registers. */
+ HOST_WIDE_INT cfa_restore_sp_offset;
+} mips_epilogue;
+
/* 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 };
@@ -8377,7 +8392,11 @@ 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)
- return gen_rtx_SET (VOIDmode, reg, mem);
+ {
+ mips_epilogue.cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg,
+ mips_epilogue.cfa_restores);
+ return gen_rtx_SET (VOIDmode, reg, mem);
+ }
if (reg_parm_p)
return gen_rtx_SET (VOIDmode, mem, reg);
return mips_frame_set (mem, reg);
@@ -10222,7 +10241,47 @@ mips_expand_prologue (void)
emit_insn (gen_blockage ());
}
-/* Emit instructions to restore register REG from slot MEM. */
+/* Attach all pending register saves to the previous instruction.
+ Return that instruction. */
+
+static rtx
+mips_epilogue_emit_cfa_restores (void)
+{
+ rtx insn;
+
+ insn = get_last_insn ();
+ gcc_assert (insn && !REG_NOTES (insn));
+ if (mips_epilogue.cfa_restores)
+ {
+ RTX_FRAME_RELATED_P (insn) = 1;
+ REG_NOTES (insn) = mips_epilogue.cfa_restores;
+ mips_epilogue.cfa_restores = 0;
+ }
+ return insn;
+}
+
+/* Like mips_epilogue_emit_cfa_restores, but also record that the CFA is
+ now at REG + OFFSET. */
+
+static void
+mips_epilogue_set_cfa (rtx reg, HOST_WIDE_INT offset)
+{
+ rtx insn;
+
+ insn = mips_epilogue_emit_cfa_restores ();
+ if (reg != mips_epilogue.cfa_reg || offset != mips_epilogue.cfa_offset)
+ {
+ RTX_FRAME_RELATED_P (insn) = 1;
+ REG_NOTES (insn) = alloc_reg_note (REG_CFA_DEF_CFA,
+ plus_constant (reg, offset),
+ REG_NOTES (insn));
+ mips_epilogue.cfa_reg = reg;
+ mips_epilogue.cfa_offset = offset;
+ }
+}
+
+/* Emit instructions to restore register REG from slot MEM. Also update
+ the cfa_restores list. */
static void
mips_restore_reg (rtx reg, rtx mem)
@@ -10231,8 +10290,17 @@ mips_restore_reg (rtx reg, rtx mem)
$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);
+ else
+ mips_epilogue.cfa_restores = alloc_reg_note (REG_CFA_RESTORE, reg,
+ mips_epilogue.cfa_restores);
mips_emit_save_slot_move (reg, mem, MIPS_EPILOGUE_TEMP (GET_MODE (reg)));
+ if (REGNO (reg) == REGNO (mips_epilogue.cfa_reg))
+ /* The CFA is currently defined in terms of the register whose
+ value we have just restored. Redefine the CFA in terms of
+ the stack pointer. */
+ mips_epilogue_set_cfa (stack_pointer_rtx,
+ mips_epilogue.cfa_restore_sp_offset);
}
/* Emit any instructions needed before a return. */
@@ -10291,6 +10359,9 @@ mips_expand_epilogue (bool sibcall_p)
base = hard_frame_pointer_rtx;
step1 -= frame->hard_frame_pointer_offset;
}
+ mips_epilogue.cfa_reg = base;
+ mips_epilogue.cfa_offset = step1;
+ mips_epilogue.cfa_restores = NULL_RTX;
/* If we need to restore registers, deallocate as much stack as
possible in the second step without going out of range. */
@@ -10320,6 +10391,7 @@ mips_expand_epilogue (bool sibcall_p)
target = stack_pointer_rtx;
emit_insn (gen_add3_insn (target, base, adjust));
+ mips_epilogue_set_cfa (target, step2);
}
/* Copy TARGET into the stack pointer. */
@@ -10332,6 +10404,7 @@ mips_expand_epilogue (bool sibcall_p)
if (TARGET_CALL_SAVED_GP && !TARGET_EXPLICIT_RELOCS)
emit_insn (gen_blockage ());
+ mips_epilogue.cfa_restore_sp_offset = step2;
if (GENERATE_MIPS16E_SAVE_RESTORE && frame->mask != 0)
{
unsigned int regno, mask;
@@ -10353,6 +10426,7 @@ mips_expand_epilogue (bool sibcall_p)
/* Restore the remaining registers and deallocate the final bit
of the frame. */
emit_insn (restore);
+ mips_epilogue_set_cfa (stack_pointer_rtx, 0);
}
else
{
@@ -10388,9 +10462,15 @@ 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)));
+ {
+ emit_insn (gen_add3_insn (stack_pointer_rtx,
+ stack_pointer_rtx,
+ GEN_INT (step2)));
+ mips_epilogue_set_cfa (stack_pointer_rtx, 0);
+ }
+ else
+ /* The choice of position is somewhat arbitrary in this case. */
+ mips_epilogue_emit_cfa_restores ();
/* Move to COP0 Status. */
emit_insn (gen_cop0_move (gen_rtx_REG (SImode, COP0_STATUS_REG_NUM),
@@ -10400,11 +10480,15 @@ 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)));
+ {
+ emit_insn (gen_add3_insn (stack_pointer_rtx,
+ stack_pointer_rtx,
+ GEN_INT (step2)));
+ mips_epilogue_set_cfa (stack_pointer_rtx, 0);
+ }
}
}
+ gcc_assert (!mips_epilogue.cfa_restores);
/* Add in the __builtin_eh_return stack adjustment. We need to
use a temporary in MIPS16 code. */
Index: gcc/testsuite/gcc.target/mips/mips.exp
===================================================================
--- gcc/testsuite/gcc.target/mips/mips.exp 2011-09-11 09:19:47.000000000 +0100
+++ gcc/testsuite/gcc.target/mips/mips.exp 2011-09-11 09:19:53.000000000 +0100
@@ -226,6 +226,7 @@ set mips_option_groups {
abi "-mabi=.*"
addressing "addressing=.*"
arch "-mips([1-5]|32.*|64.*)|-march=.*|isa(|_rev)(=|<=|>=).*"
+ debug "-g*"
dump_pattern "-dp"
endianness "-E(L|B)|-me(l|b)"
float "-m(hard|soft)-float"
Index: gcc/testsuite/gcc.target/mips/interrupt_handler-2.c
===================================================================
--- /dev/null 2011-09-11 09:48:11.061662158 +0100
+++ gcc/testsuite/gcc.target/mips/interrupt_handler-2.c 2011-09-11 09:27:18.000000000 +0100
@@ -0,0 +1,14 @@
+/* Make sure that we emit .cfa_restore notes for LO and HI. */
+/* { dg-options "-mips32r2 -msoft-float -O -g" } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 64\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 65\n" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa( |\t)" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa_register( |\t)" } } */
+
+extern void f (void);
+
+NOMIPS16 void __attribute__ ((interrupt, use_shadow_register_set))
+v1 (void)
+{
+ f ();
+}
Index: gcc/testsuite/gcc.target/mips/interrupt_handler-3.c
===================================================================
--- /dev/null 2011-09-11 09:48:11.061662158 +0100
+++ gcc/testsuite/gcc.target/mips/interrupt_handler-3.c 2011-09-11 09:25:54.000000000 +0100
@@ -0,0 +1,33 @@
+/* Make sure that we emit .cfa_restore notes for LO, HI and GPRs. */
+/* { dg-options "-mips32r2 -msoft-float -O -g" } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 1\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 2\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 3\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 4\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 5\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 6\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 7\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 8\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 9\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 10\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 11\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 12\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 13\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 14\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 15\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 24\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 25\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 31\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 64\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_restore 65\n" } } */
+/* { dg-final { scan-assembler "\t\\\.cfi_def_cfa_offset 0\n" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa( |\t)" } } */
+/* { dg-final { scan-assembler-not "\\\.cfi_def_cfa_register( |\t)" } } */
+
+extern void f (void);
+
+NOMIPS16 void __attribute__ ((interrupt))
+v1 (void)
+{
+ f ();
+}