From patchwork Thu Jul 18 12:33:05 2019 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Jozef Lawrynowicz X-Patchwork-Id: 1133716 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Authentication-Results: ozlabs.org; spf=pass (mailfrom) smtp.mailfrom=gcc.gnu.org (client-ip=209.132.180.131; helo=sourceware.org; envelope-from=gcc-patches-return-505272-incoming=patchwork.ozlabs.org@gcc.gnu.org; receiver=) Authentication-Results: ozlabs.org; dmarc=none (p=none dis=none) header.from=mittosystems.com Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b="pqDWzvbD"; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=mittosystems.com header.i=@mittosystems.com header.b="jDP4BiAp"; dkim-atps=neutral Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id 45qD7n5zD3z9s4Y for ; Thu, 18 Jul 2019 22:33:21 +1000 (AEST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; q=dns; s=default; b=r1/0S07TzaIBZHyPAXLeM+LFZjZZiHvGecw/HRL7tPol/UOGL7 xL6tF6jTPJgcfrTQA8Yl2mNg/v0UzZw/UyoOML+LPMNCsYeZxYkJliofLREGfLhU nmlaJKF6g/a90Dx5J75ChN30fHdH6VJU4g7W1Ht1Hq3YFR8AnJvsY0iJc= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:message-id:mime-version:content-type; s= default; bh=XCFiTqzD/hfEGotGBMxRjOLjAvY=; b=pqDWzvbDr7HPG5Eojrul t69lhVGCqPuFr84zEE5zkIB3ktku8Pf45fwlF1wERCGgmfj9OzJiyzzUodJUMS3I /x+oZ54jZVEZ3k17wJEyayE621UQnNxeQmtEmAR/Wi77e6TOkWdVTf8iImoIU+U5 XJTUNbMCZiGF7ZipG4Hyzbw= Received: (qmail 52970 invoked by alias); 18 Jul 2019 12:33:13 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 52650 invoked by uid 89); 18 Jul 2019 12:33:13 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-26.9 required=5.0 tests=BAYES_00, GIT_PATCH_0, GIT_PATCH_1, GIT_PATCH_2, GIT_PATCH_3, RCVD_IN_DNSWL_NONE, SPF_PASS autolearn=ham version=3.3.1 spammy=surprise, restored X-HELO: mail-wr1-f48.google.com Received: from mail-wr1-f48.google.com (HELO mail-wr1-f48.google.com) (209.85.221.48) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 18 Jul 2019 12:33:10 +0000 Received: by mail-wr1-f48.google.com with SMTP id x1so13507666wrr.9 for ; Thu, 18 Jul 2019 05:33:10 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=mittosystems.com; s=google; h=date:from:to:cc:subject:message-id:mime-version; bh=iiwTHMqGQCLza4AsJV6KvFbcGQ6/i0my7+Ld003hzrg=; b=jDP4BiApJUlVlE50N8ndTOV5EeivczxG03HMFJ/l5QIvFoiw5Dg0cZbFbgd4EsRfeY Y3iuRz67vF4fAFsfKeFo96f8e3RsGZRQDd1F8t3+Sq4H3VNJn4gwd1/EJJoZaH7JA3zE DjzJ9Q2ym3InLOYMyRmD2rNE4kpzIEu9wi0ahQFL4M3vYAsqSbEX8CkuJlC7Wu551O11 ijx1HViU+JrKVYJbZwjZlzKTNxIiHNanXJBd+u22xojHzi2UJwZTvzsuZZm3gQfypSlG 2Puni6fcKCa8KngJSpcoia80WzwDmDDFrQxy+OjITLJEI8nvdsNQqVFrY/U8O7RqzqwG jabg== Received: from jozef-kubuntu ([2a01:4b00:87fd:900:7d5b:b0c5:f732:e63e]) by smtp.gmail.com with ESMTPSA id v4sm22017072wmg.22.2019.07.18.05.33.07 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Thu, 18 Jul 2019 05:33:07 -0700 (PDT) Date: Thu, 18 Jul 2019 13:33:05 +0100 From: Jozef Lawrynowicz To: "gcc-patches@gcc.gnu.org" Cc: law@redhat.com Subject: [PATCH][MSP430] Fix unnecessary saving of all callee-saved regs in an interrupt function that calls another function Message-ID: <20190718133305.5fc2c57e@jozef-kubuntu> MIME-Version: 1.0 X-IsSubscribed: yes 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 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 * 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 * 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