The attached patch fixes an issue for msp430 where the logic to decide which
registers need to be saved in an interrupt function was unnecessarily
choosing to save all callee-saved registers regardless of whether they were
used or not. This came at a code size and performance penalty for the 430 ISA,
and a performance penalty for the 430X ISA.
Interrupt functions require special conventions for saving registers which
would normally be caller-saved. Since the interrupt happens without warning,
registers that would normally have been preserved by the caller of a function
cannot be preserved when an interrupt is triggered. This means interrupts must
save and restore the used caller-saved registers, in addition to the used
callee-saved registers that a regular function would save.
If an interrupt is not a leaf function, all caller-saved registers must be
saved/restored in the prologue/epilogue of the interrupt function, since it
is unknown which of these will be modified in later functions.
We can rely on the function called by an interrupt to save and restore
callee-saved registers, so it is unnecessary to save all callee-saved regs
in the ISR. This is what this patch changes.
Successfully regtested for msp430-elf on trunk for C/C++.
Ok for trunk?
Thanks,
Jozef
>From 1e151dac2be34ae50bea8b4b37bd2d78c5f7ddd6 Mon Sep 17 00:00:00 2001
From: Jozef Lawrynowicz <joze...@mittosystems.com>
Date: Thu, 18 Jul 2019 09:25:52 +0100
Subject: [PATCH] MSP430: Fix unnecessary saving of all callee-saved regs in an
ISR which calls another function
gcc/ChangeLog:
2019-07-18 Jozef Lawrynowicz <joze...@mittosystems.com>
* config/msp430/msp430.c (msp430_preserve_reg_p): Don't save
callee-saved regs R4->R10 in an interrupt function that calls another
function.
gcc/testsuite/ChangeLog:
2019-07-18 Jozef Lawrynowicz <joze...@mittosystems.com>
* gcc.target/msp430/isr-push-pop-main.c: New test.
* gcc.target/msp430/isr-push-pop-isr-430.c: Likewise.
* gcc.target/msp430/isr-push-pop-isr-430x.c: Likewise.
* gcc.target/msp430/isr-push-pop-leaf-isr-430.c: Likewise.
* gcc.target/msp430/isr-push-pop-leaf-isr-430x.c: Likewise.
---
gcc/config/msp430/msp430.c | 18 ++-
.../gcc.target/msp430/isr-push-pop-isr-430.c | 13 ++
.../gcc.target/msp430/isr-push-pop-isr-430x.c | 12 ++
.../msp430/isr-push-pop-leaf-isr-430.c | 27 ++++
.../msp430/isr-push-pop-leaf-isr-430x.c | 24 ++++
.../gcc.target/msp430/isr-push-pop-main.c | 120 ++++++++++++++++++
6 files changed, 209 insertions(+), 5 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430.c
create mode 100644 gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430x.c
create mode 100644 gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430.c
create mode 100644 gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430x.c
create mode 100644 gcc/testsuite/gcc.target/msp430/isr-push-pop-main.c
diff --git a/gcc/config/msp430/msp430.c b/gcc/config/msp430/msp430.c
index 365e9eba747..265c2f642d8 100644
--- a/gcc/config/msp430/msp430.c
+++ b/gcc/config/msp430/msp430.c
@@ -1755,11 +1755,19 @@ msp430_preserve_reg_p (int regno)
if (fixed_regs [regno])
return false;
- /* Interrupt handlers save all registers they use, even
- ones which are call saved. If they call other functions
- then *every* register is saved. */
- if (msp430_is_interrupt_func ())
- return ! crtl->is_leaf || df_regs_ever_live_p (regno);
+ /* For interrupt functions we must save and restore the used regs that
+ would normally be caller-saved (R11->R15). */
+ if (msp430_is_interrupt_func () && regno >= 11 && regno <= 15)
+ {
+ if (crtl->is_leaf && df_regs_ever_live_p (regno))
+ /* If the interrupt func is a leaf then we only need to restore the
+ caller-saved regs that are used. */
+ return true;
+ else if (!crtl->is_leaf)
+ /* If the interrupt function is not a leaf we must save all
+ caller-saved regs in case the callee modifies them. */
+ return true;
+ }
if (!call_used_regs [regno]
&& df_regs_ever_live_p (regno))
diff --git a/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430.c b/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430.c
new file mode 100644
index 00000000000..a2bf8433ebd
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430.c
@@ -0,0 +1,13 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x*" "-mlarge" } { "" } } */
+/* { dg-options "-mcpu=msp430" } */
+/* { dg-final { scan-assembler "PUSH\tR11" } } */
+/* { dg-final { scan-assembler-not "PUSH\tR10" } } */
+
+void __attribute__((noinline)) callee (void);
+
+void __attribute__((interrupt))
+isr (void)
+{
+ callee();
+}
diff --git a/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430x.c b/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430x.c
new file mode 100644
index 00000000000..2d65186bdf9
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/isr-push-pop-isr-430x.c
@@ -0,0 +1,12 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
+/* { dg-final { scan-assembler "PUSHM.*#5" } } */
+/* { dg-final { scan-assembler-not "PUSHM.*#12" } } */
+
+void __attribute__((noinline)) callee (void);
+
+void __attribute__((interrupt))
+isr (void)
+{
+ callee();
+}
diff --git a/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430.c b/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430.c
new file mode 100644
index 00000000000..cbb45974c4a
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430.c
@@ -0,0 +1,27 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430x*" "-mlarge" } { "" } } */
+/* { dg-options "-mcpu=msp430" } */
+/* { dg-final { scan-assembler "PUSH\tR5" } } */
+/* { dg-final { scan-assembler "PUSH\tR12" } } */
+/* { dg-final { scan-assembler-not "PUSH\tR4" } } */
+/* { dg-final { scan-assembler-not "PUSH\tR11" } } */
+
+/* To check that the compiler doesn't blindly save all regs, we omit R4 and R11
+ from the trashing. */
+#define TRASH_REGS_LITE \
+ __asm__ ("mov #0xFFFF, r5" : : : "R5"); \
+ __asm__ ("mov #0xFFFF, r6" : : : "R6"); \
+ __asm__ ("mov #0xFFFF, r7" : : : "R7"); \
+ __asm__ ("mov #0xFFFF, r8" : : : "R8"); \
+ __asm__ ("mov #0xFFFF, r9" : : : "R9"); \
+ __asm__ ("mov #0xFFFF, r10" : : : "R10"); \
+ __asm__ ("mov #0xFFFF, r12" : : : "R12"); \
+ __asm__ ("mov #0xFFFF, r13" : : : "R13"); \
+ __asm__ ("mov #0xFFFF, r14" : : : "R14"); \
+ __asm__ ("mov #0xFFFF, r15" : : : "R15");
+
+void __attribute__((interrupt))
+isr_leaf (void)
+{
+ TRASH_REGS_LITE
+}
diff --git a/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430x.c b/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430x.c
new file mode 100644
index 00000000000..872a40ef755
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/isr-push-pop-leaf-isr-430x.c
@@ -0,0 +1,24 @@
+/* { dg-do compile } */
+/* { dg-skip-if "" { *-*-* } { "-mcpu=msp430" } { "" } } */
+/* { dg-final { scan-assembler "PUSHM.*#4.*R15" } } */
+/* { dg-final { scan-assembler "PUSHM.*#6.*R10" } } */
+
+/* To check that the compiler doesn't blindly save all regs, we omit R4 and R11
+ from the trashing. */
+#define TRASH_REGS_LITE \
+ __asm__ ("mov #0xFFFF, r5" : : : "R5"); \
+ __asm__ ("mov #0xFFFF, r6" : : : "R6"); \
+ __asm__ ("mov #0xFFFF, r7" : : : "R7"); \
+ __asm__ ("mov #0xFFFF, r8" : : : "R8"); \
+ __asm__ ("mov #0xFFFF, r9" : : : "R9"); \
+ __asm__ ("mov #0xFFFF, r10" : : : "R10"); \
+ __asm__ ("mov #0xFFFF, r12" : : : "R12"); \
+ __asm__ ("mov #0xFFFF, r13" : : : "R13"); \
+ __asm__ ("mov #0xFFFF, r14" : : : "R14"); \
+ __asm__ ("mov #0xFFFF, r15" : : : "R15");
+
+void __attribute__((interrupt))
+isr_leaf (void)
+{
+ TRASH_REGS_LITE
+}
diff --git a/gcc/testsuite/gcc.target/msp430/isr-push-pop-main.c b/gcc/testsuite/gcc.target/msp430/isr-push-pop-main.c
new file mode 100644
index 00000000000..5c7b594b50b
--- /dev/null
+++ b/gcc/testsuite/gcc.target/msp430/isr-push-pop-main.c
@@ -0,0 +1,120 @@
+/* { dg-do run } */
+
+#ifdef __MSP430X__
+#include "isr-push-pop-isr-430x.c"
+#include "isr-push-pop-leaf-isr-430x.c"
+#else
+#include "isr-push-pop-isr-430.c"
+#include "isr-push-pop-leaf-isr-430.c"
+#endif
+
+/* Test that ISRs which call other functions do not save extraneous registers.
+ They only need to save the caller-saved regs R11->R15.
+ We use a lot of asm statements to hide what is going on from the compiler to
+ more accurately simulate an interrupt. */
+
+/* Store the register number in each general register R4->R15, so they can be
+ later checked their value has been kept. */
+#define SETUP_REGS \
+ __asm__ ("mov #4, r4"); \
+ __asm__ ("mov #5, r5"); \
+ __asm__ ("mov #6, r6"); \
+ __asm__ ("mov #7, r7"); \
+ __asm__ ("mov #8, r8"); \
+ __asm__ ("mov #9, r9"); \
+ __asm__ ("mov #10, r10"); \
+ __asm__ ("mov #11, r11"); \
+ __asm__ ("mov #12, r12"); \
+ __asm__ ("mov #13, r13"); \
+ __asm__ ("mov #14, r14"); \
+ __asm__ ("mov #15, r15");
+
+/* Write an arbitrary value to all general regs. */
+#define TRASH_REGS \
+ __asm__ ("mov #0xFFFF, r4" : : : "R4"); \
+ __asm__ ("mov #0xFFFF, r5" : : : "R5"); \
+ __asm__ ("mov #0xFFFF, r6" : : : "R6"); \
+ __asm__ ("mov #0xFFFF, r7" : : : "R7"); \
+ __asm__ ("mov #0xFFFF, r8" : : : "R8"); \
+ __asm__ ("mov #0xFFFF, r9" : : : "R9"); \
+ __asm__ ("mov #0xFFFF, r10" : : : "R10"); \
+ __asm__ ("mov #0xFFFF, r11" : : : "R11"); \
+ __asm__ ("mov #0xFFFF, r12" : : : "R12"); \
+ __asm__ ("mov #0xFFFF, r13" : : : "R13"); \
+ __asm__ ("mov #0xFFFF, r14" : : : "R14"); \
+ __asm__ ("mov #0xFFFF, r15" : : : "R15");
+
+/* Check the value in all general registers is the same as that set in
+ SETUP_REGS. */
+#define CHECK_REGS \
+ __asm__ ("cmp #4, r4 { jne ABORT"); \
+ __asm__ ("cmp #5, r5 { jne ABORT"); \
+ __asm__ ("cmp #6, r6 { jne ABORT"); \
+ __asm__ ("cmp #7, r7 { jne ABORT"); \
+ __asm__ ("cmp #8, r8 { jne ABORT"); \
+ __asm__ ("cmp #9, r9 { jne ABORT"); \
+ __asm__ ("cmp #10, r10 { jne ABORT"); \
+ __asm__ ("cmp #11, r11 { jne ABORT"); \
+ __asm__ ("cmp #12, r12 { jne ABORT"); \
+ __asm__ ("cmp #13, r13 { jne ABORT"); \
+ __asm__ ("cmp #14, r14 { jne ABORT"); \
+ __asm__ ("cmp #15, r15 { jne ABORT");
+
+void __attribute__((noinline))
+callee (void)
+{
+ /* Here were modify all the regs, but tell the compiler that we are since
+ this is just a way to simulate a function that happens to modify all the
+ registers. */
+ TRASH_REGS
+}
+int
+#ifdef __MSP430X_LARGE__
+__attribute__((lower))
+#endif
+main (void)
+{
+ SETUP_REGS
+
+ /* A surprise branch to the ISR that the compiler cannot prepare for.
+ We must first simulate the interrupt acceptance procedure that the
+ hardware would normally take care of.
+ So push the desired PC return address, and then the SR (R2).
+ MSP430X expects the high bits 19:16 of the PC return address to be stored
+ in bits 12:15 of the SR stack slot. This is hard to handle in hand-rolled
+ assembly code, so we always place main() in lower memory so the return
+ address is 16-bits. */
+ __asm__ ("push #CHECK1");
+ __asm__ ("push r2");
+ __asm__ ("br #isr");
+
+ __asm__ ("CHECK1:");
+ /* If any of the regs R4->R15 don't match their original value, this will
+ jump to ABORT. */
+ CHECK_REGS
+
+ /* Now test that an interrupt function that is a leaf also works
+ correctly. */
+ __asm__ ("push #CHECK2");
+ __asm__ ("push r2");
+ __asm__ ("br #isr_leaf");
+
+ __asm__ ("CHECK2:");
+ CHECK_REGS
+
+ /* The values in R4->R15 were successfully checked, now jump to FINISH to run
+ the prologue generated by the compiler. */
+ __asm__ ("jmp FINISH");
+
+ /* CHECK_REGS will branch here if a register holds the wrong value. */
+ __asm__ ("ABORT:");
+#ifdef __MSP430X_LARGE__
+ __asm__ ("calla #abort");
+#else
+ __asm__ ("call #abort");
+#endif
+
+ __asm__ ("FINISH:");
+ return 0;
+}
+
--
2.17.1
