From patchwork Fri Aug 14 12:00:51 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Robert Suchanek X-Patchwork-Id: 507360 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org 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 BC842140291 for ; Fri, 14 Aug 2015 22:01:14 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=vzIleVc2; dkim-atps=neutral DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :to:cc:subject:date:message-id:references:in-reply-to :content-type:content-transfer-encoding:mime-version; q=dns; s= default; b=buz02gNp9Wcxqahm6q6gG3qiNfv3VD3QFHpuTofSza9nS6EahF4P5 k6r2M0D0gXHvVnVazqWIVmxS0fyFDXTtmn2CGX0eitx+/xJIgSPEhUy7nfSIDqrq 08j8Z1jtCvyVNExayxOeh6VX+S/cd3OD8X56c1AnKw4lF5FVfVVOTY= 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:from :to:cc:subject:date:message-id:references:in-reply-to :content-type:content-transfer-encoding:mime-version; s=default; bh=HIRRwUMnQKqRwYLJhq589JtTWcw=; b=vzIleVc2D3EqFZ/TChooqt4WDPCU fdEv50SS5niwDqpQEjghCmuECtPzmxNTyzLL6+YQYiZC3RdXPLQzx+2EK+DyNXMB GLgt95b9r4Qr2grtdKu/dx6ZU03nBCiUeuNSMFdIl8Q6kPReAtl+g/qFlU8HJsEw WhzBdDhArY+ECeQ= Received: (qmail 64461 invoked by alias); 14 Aug 2015 12:01:06 -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 64448 invoked by uid 89); 14 Aug 2015 12:01:06 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.2 required=5.0 tests=AWL, BAYES_00, RCVD_IN_DNSWL_LOW, RP_MATCHES_RCVD, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mailapp01.imgtec.com Received: from mailapp01.imgtec.com (HELO mailapp01.imgtec.com) (195.59.15.196) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Fri, 14 Aug 2015 12:00:55 +0000 Received: from KLMAIL01.kl.imgtec.org (unknown [192.168.5.35]) by Websense Email Security Gateway with ESMTPS id 77CF34337F91E; Fri, 14 Aug 2015 13:00:50 +0100 (IST) Received: from hhmail02.hh.imgtec.org (10.100.10.20) by KLMAIL01.kl.imgtec.org (192.168.5.35) with Microsoft SMTP Server (TLS) id 14.3.195.1; Fri, 14 Aug 2015 13:00:52 +0100 Received: from hhmail02.hh.imgtec.org ([::1]) by hhmail02.hh.imgtec.org ([::1]) with mapi id 14.03.0235.001; Fri, 14 Aug 2015 13:00:52 +0100 From: Robert Suchanek To: Mike Stump , Richard Sandiford CC: "Catherine_Moore@mentor.com" , Matthew Fortune , "gcc-patches@gcc.gnu.org" Subject: RE: [PATCH][MIPS] Fix register renaming in the interrupt handlers Date: Fri, 14 Aug 2015 12:00:51 +0000 Message-ID: References: <87k2szc6b9.fsf@googlemail.com> <4438A5B8-6266-4E4F-A43C-8BA503A08900@comcast.net> In-Reply-To: <4438A5B8-6266-4E4F-A43C-8BA503A08900@comcast.net> MIME-Version: 1.0 X-IsSubscribed: yes Hi, > > You also need to do the same thing for TARGET_HARD_REGNO_SCRATCH_OK, > > to stop peephole2 from using unsaved registers as scratch registers. > > > > I should dig out my patches to clean up this interface. It's just > > too brittle to have two macros that say what registers can be used > > after reload. Indeed. I naively thought that there would be a single place that needed an update and overlooked this hook. > Gosh, I don't like the interface of them at all. I have interrupt functions > and I want all the goodness out of gcc and I want gcc to just know that it > can't use registers it doesn't want to save for any reason that the port > marked as saved by the calling api of the function. :-( Very few ports (3) > define TARGET_HARD_REGNO_SCRATCH_OK and many port (19 or so) don't define > this but do seem to have interrupt functions. The existing documentation > seems not give be enough information to let me decide if I need to define it > or not. I read through all the ports that do define it, and none of them > shed any light on to allow me to decide if my port needs to or not. > > So, first question is, are the 16 other ports that have interrupt functions > and don't define this just buggy (or non-optimal)? How can I tell if I need > to or not? A single example of that does cause a port to see it and a single > example of why a port would not need to define it would be instructive. > > Along the lines of TARGET_CALL_FUSAGE_CONTAINS_NON_CALLEE_CLOBBERS, I'd be > willing to explain the required abi of each function, and then I just want > gcc to just work. All functions are normal, except for interrupt functions, > all allocatable register need to be saved. I think this is a universally > true thing, and it just happens to be true on my port as well. > > Anyway, I'd love to see gcc improved in this area. > > This one is a pet peeve of mine as I've seen what happens to software when a > single register that should have been saved, wasn't. It was slightly > annoying to track down and find. Fixing was trivial. The cleanup as Richard suggested would probably be a good start. It's interesting that this kind of bug wasn't discovered earlier and likely existing for several years. The below is an updated version of the patch to include the other hook. Regards, Robert gcc/ * config/mips/mips-protos.h (mips_hard_regno_rename_ok): New prototype. * config/mips/mips.c (mips_hard_regno_rename_ok): New function. (mips_hard_regno_scratch_ok): Likewise. (TARGET_HARD_REGNO_SCRATCH_OK): Define macro. * config/mips/mips.h (HARD_REGNO_RENAME_OK): New. gcc/testsuite/ * gcc.target/mips/interrupt_handler-bug-1.c: New test. --- gcc/config/mips/mips-protos.h | 1 + gcc/config/mips/mips.c | 30 ++++++++++++++++++++++ gcc/config/mips/mips.h | 3 +++ .../gcc.target/mips/interrupt_handler-bug-1.c | 11 ++++++++ 4 files changed, 45 insertions(+) create mode 100644 gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c diff --git a/gcc/config/mips/mips-protos.h b/gcc/config/mips/mips-protos.h index 6ce3d70..e455553 100644 --- a/gcc/config/mips/mips-protos.h +++ b/gcc/config/mips/mips-protos.h @@ -317,6 +317,7 @@ extern unsigned int mips_sync_loop_insns (rtx, rtx *); extern const char *mips_output_division (const char *, rtx *); extern const char *mips_msa_output_division (const char *, rtx *); extern const char *mips_output_probe_stack_range (rtx, rtx); +extern bool mips_hard_regno_rename_ok (unsigned int, unsigned int); extern unsigned int mips_hard_regno_nregs (int, machine_mode); extern bool mips_linked_madd_p (rtx, rtx); extern bool mips_store_data_bypass_p (rtx, rtx); diff --git a/gcc/config/mips/mips.c b/gcc/config/mips/mips.c index a9829bd..d0851e9 100644 --- a/gcc/config/mips/mips.c +++ b/gcc/config/mips/mips.c @@ -12959,6 +12959,33 @@ mips_hard_regno_mode_ok_p (unsigned int regno, machine_mode mode) return false; } +/* Return nonzero if register OLD_REG can be renamed to register NEW_REG. */ + +bool +mips_hard_regno_rename_ok (unsigned int old_reg ATTRIBUTE_UNUSED, + unsigned int new_reg) +{ + /* Interrupt functions can only use registers that have already been + saved by the prologue, even if they would normally be call-clobbered. */ + if (cfun->machine->interrupt_handler_p && !df_regs_ever_live_p (new_reg)) + return false; + + return true; +} + +/* Return nonzero if register REGNO can be used as a scratch register + in peephole2. */ + +bool +mips_hard_regno_scratch_ok (unsigned int regno) +{ + /* See mips_hard_regno_rename_ok. */ + if (cfun->machine->interrupt_handler_p && !df_regs_ever_live_p (regno)) + return false; + + return true; +} + /* Implement HARD_REGNO_NREGS. */ unsigned int @@ -22428,6 +22455,9 @@ mips_lra_p (void) #undef TARGET_SCHED_SET_SCHED_FLAGS #define TARGET_SCHED_SET_SCHED_FLAGS mips_set_sched_flags +#undef TARGET_HARD_REGNO_SCRATCH_OK +#define TARGET_HARD_REGNO_SCRATCH_OK mips_hard_regno_scratch_ok + struct gcc_target targetm = TARGET_INITIALIZER; #include "gt-mips.h" diff --git a/gcc/config/mips/mips.h b/gcc/config/mips/mips.h index 6578ae5..3fe690a 100644 --- a/gcc/config/mips/mips.h +++ b/gcc/config/mips/mips.h @@ -1932,6 +1932,9 @@ struct mips_cpu_info { #define HARD_REGNO_MODE_OK(REGNO, MODE) \ mips_hard_regno_mode_ok[ (int)(MODE) ][ (REGNO) ] +#define HARD_REGNO_RENAME_OK(OLD_REG, NEW_REG) \ + mips_hard_regno_rename_ok (OLD_REG, NEW_REG) + /* Select a register mode required for caller save of hard regno REGNO. */ #define HARD_REGNO_CALLER_SAVE_MODE(REGNO, NREGS, MODE) \ mips_hard_regno_caller_save_mode (REGNO, NREGS, MODE) diff --git a/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c new file mode 100644 index 0000000..128ba90 --- /dev/null +++ b/gcc/testsuite/gcc.target/mips/interrupt_handler-bug-1.c @@ -0,0 +1,11 @@ +/* { dg-options "-funroll-loops" } */ +int foo; +int bar; + +void __attribute__ ((interrupt)) +isr (void) +{ + if (!foo) + while (bar & 0xFF30); +} +/* { dg-final { scan-assembler-not "\\\$8" } } */