From patchwork Tue Jan 12 07:20:56 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kaushik M Phatak X-Patchwork-Id: 566362 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 2DBB31402F0 for ; Tue, 12 Jan 2016 18:21:11 +1100 (AEDT) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=A8xWdZye; 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 :mime-version:date:message-id:subject:from:to:cc:content-type; q=dns; s=default; b=A2BFth5oS5u+FtyhdnePYwW13AsurG6YPY4OKBkAWy/ rYMqKnx7PKkDwC14MLTSbZohIyqtyD1DapMO8WB5O29MgO6W2QuWkFBsYMTHPikd 7BKFRm0P+qodUwE+yb8UALEHe68qyp/d7kgPnVv6rlm5wPTMjFa0Kv4mTS1TGlJM = 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 :mime-version:date:message-id:subject:from:to:cc:content-type; s=default; bh=tTyS9FhoE9eNHlnbZolwSStGE5k=; b=A8xWdZyeKXDh+xQDG YCoMEiJ07ZcPQyY6nOxA95QFL02zY7GSlLvCJ2+A4nVUUdKWGNuHcw90q4CqsQ/5 QBCcCa/t9C3rrCNjuBOalyiFAdPjJeTCbaVto1vOUVEmTcVYq4EgpBK1ZgRrVW0+ r8hn4hse11kVc8ymB/uCfNurVI= Received: (qmail 99359 invoked by alias); 12 Jan 2016 07:21:02 -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 99338 invoked by uid 89); 12 Jan 2016 07:21:01 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=0.2 required=5.0 tests=AWL, BAYES_50, FREEMAIL_FROM, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=no version=3.3.2 spammy=35810, sk:machine, Structure, naked X-HELO: mail-oi0-f51.google.com Received: from mail-oi0-f51.google.com (HELO mail-oi0-f51.google.com) (209.85.218.51) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-GCM-SHA256 encrypted) ESMTPS; Tue, 12 Jan 2016 07:20:58 +0000 Received: by mail-oi0-f51.google.com with SMTP id o124so58419829oia.3 for ; Mon, 11 Jan 2016 23:20:58 -0800 (PST) MIME-Version: 1.0 X-Received: by 10.202.210.151 with SMTP id j145mr91426843oig.114.1452583256708; Mon, 11 Jan 2016 23:20:56 -0800 (PST) Received: by 10.202.97.133 with HTTP; Mon, 11 Jan 2016 23:20:56 -0800 (PST) Date: Tue, 12 Jan 2016 12:50:56 +0530 Message-ID: Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines From: Kaushik M Phatak To: Nick Clifton Cc: dj@redhat.com, Mike Stump , gcc-patches@gcc.gnu.org, "kaushik.phatak@kpit.com" Hi Nick, Thanks for your detailed review. Please find an updated version of this patch here. I have tried to modify it as per your suggestions. > I would suggest: > static bool Done. > + if (recog_memoized (insn) == CODE_FOR_udivmodsi4_g13 > have an attribute on these insns, and then test for that here. Done. Added attribute 'is_g13_muldiv_insn' which gets tested in this function. > + return 1; > If you change the function to "bool" then return "true" here. > + return 0; Done. > if (rl78_is_naked_func ()) > return; > - > + > Why are you adding a extraneous space here ? Done. This was a typo while extracting the patch. > + if (MUST_SAVE_MDUC_REGISTER && (!crtl->is_leaf || check_mduc_usage ())) > It would be nice to update the stack size computation performed I have tried to add code to update 'fs' while checking for 'framesize_locals'. Please let me know if this is OK. > + emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG))); > it would be cleaner to use a for loop and a small structure containing > the address, mode and volatility to be saved. I have created simple structure for this, though the volatility is always set and may not need to be a separate member. Please let me know if this is OK. > Hence I would suggest: > fprintf (file, "\t; preserves MDUC registers\n"); Done. > +This is the default. > It is *not* the default! Updated this. However, the registers are saved when -mmul=g13 and -mg13 is passed. > +@item -msave-mduc-in-interrupts > +@item -mno-save-mduc-in-interrupts > +@opindex msave-mduc-in-interrupts > You should also mention that even if this option is enabled Added information regarding registers being saved based on certain conditions being met. Kindly review the updated patch and let me know if it is OK. This is regression tested for rl78 -msim and -mg13 + -msave-mduc-in-interrupts Best Regards, Kaushik gcc/ChangeLog 2016-01-12 Kaushik Phatak * config/rl78/rl78.c (rl78_expand_prologue): Save the MDUC related registers in all interrupt handlers if necessary. (rl78_option_override): Add warning. (MUST_SAVE_MDUC_REGISTER): New macro. (rl78_expand_epilogue): Restore the MDUC registers if necessary. * config/rl78/rl78.c (check_mduc_usage): New function. * config/rl78/rl78.c (mduc_regs): New structure to hold MDUC register data. * config/rl78/rl78.md (is_g13_muldiv_insn): New attribute. * config/rl78/rl78.md (mulsi3_g13): Add is_g13_muldiv_insn attribute. * config/rl78/rl78.md (udivmodsi4_g13): Add is_g13_muldiv_insn attribute. * config/rl78/rl78.md (mulhi3_g13): Add is_g13_muldiv_insn attribute. * config/rl78/rl78.opt (msave-mduc-in-interrupts): New option. (mno-save-mduc-in-interrupts): New option. * doc/invoke.texi (@item -msave-mduc-in-interrupts): New item. (@item -mno-save-mduc-in-interrupts): New item ================== END of Patch ============== -----Original Message----- From: Nick Clifton [mailto:nickc@redhat.com] Sent: Thursday, January 07, 2016 5:33 PM To: Kaushik M Phatak ; dj@redhat.com Cc: Mike Stump ; gcc-patches@gcc.gnu.org; Kaushik Phatak Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines Hi Kaushik, Just a few comments on the patch itself: Index: gcc/config/rl78/rl78.c =================================================================== --- gcc/config/rl78/rl78.c (revision 2871) +++ gcc/config/rl78/rl78.c (working copy) @@ -83,6 +83,22 @@ "sp", "ap", "psw", "es", "cs" }; +/* Structure for G13 MDUC registers. */ +struct mduc_reg_type +{ + unsigned int address; + enum machine_mode mode; + bool is_volatile; +}; + +struct mduc_reg_type mduc_regs[NUM_OF_MDUC_REGS] = + {{0xf00e8, QImode, true}, + {0xffff0, HImode, true}, + {0xffff2, HImode, true}, + {0xf2224, HImode, true}, + {0xf00e0, HImode, true}, + {0xf00e2, HImode, true}}; + struct GTY(()) machine_function { /* If set, the rest of the fields have been computed. */ @@ -342,6 +358,10 @@ #undef TARGET_OPTION_OVERRIDE #define TARGET_OPTION_OVERRIDE rl78_option_override +#define MUST_SAVE_MDUC_REGISTERS \ + (!TARGET_NO_SAVE_MDUC_REGISTERS \ + && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13) + static void rl78_option_override (void) { @@ -366,6 +386,9 @@ /* Address spaces are currently only supported by C. */ error ("-mes0 can only be used with C"); + if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13)) + warning (0, "mduc registers only saved for G13 target"); + switch (rl78_cpu_type) { case CPU_UNINIT: @@ -1307,6 +1330,23 @@ return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE); } +/* Check if the block uses mul/div insns for G13 target. */ +static bool +check_mduc_usage () +{ + rtx insn; + basic_block bb; + FOR_EACH_BB_FN (bb, cfun) + { + FOR_BB_INSNS (bb, insn) + { + if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES) + return true; + } + } + return false; +} + /* Expand the function prologue (from the prologue pattern). */ void rl78_expand_prologue (void) @@ -1371,6 +1411,28 @@ F (emit_insn (gen_push (ax))); } + /* Save MDUC registers inside interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + rtx mem_mduc; + + for (int i = 0; i machine->framesize_locals + cfun->machine->framesize_outgoing; + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + fs = fs + NUM_OF_MDUC_REGS * 2; if (fs > 0) { /* If we need to subtract more than 254*3 then it is faster and @@ -1426,6 +1490,8 @@ else { fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing; + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + fs = fs + NUM_OF_MDUC_REGS * 2; if (fs > 254 * 3) { emit_move_insn (ax, sp); @@ -1444,6 +1510,29 @@ } } + /* Restore MDUC registers from interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + rtx mem_mduc; + + for (int i = NUM_OF_MDUC_REGS-1; i >= 0; i--) + { + emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG))); + if (mduc_regs[i].mode == QImode) + { + mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address)); + MEM_VOLATILE_P (mem_mduc) = 1; + emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG))); + } + else + { + mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address)); + MEM_VOLATILE_P (mem_mduc) = 1; + emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG))); + } + } + } + /* Save ES register inside interrupt functions. */ if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es) { @@ -1599,6 +1688,9 @@ if (cfun->machine->uses_es) fprintf (file, "\t; uses ES register\n"); + + if (MUST_SAVE_MDUC_REGISTERS) + fprintf (file, "\t; preserves MDUC registers\n"); } /* Return an RTL describing where a function return value of type RET_TYPE Index: gcc/config/rl78/rl78.h =================================================================== --- gcc/config/rl78/rl78.h (revision 2871) +++ gcc/config/rl78/rl78.h (working copy) @@ -28,6 +28,7 @@ #define TARGET_G14 (rl78_cpu_type == CPU_G14) +#define NUM_OF_MDUC_REGS 6 #define TARGET_CPU_CPP_BUILTINS() \ do \ Index: gcc/config/rl78/rl78.md =================================================================== --- gcc/config/rl78/rl78.md (revision 2871) +++ gcc/config/rl78/rl78.md (working copy) @@ -78,6 +78,7 @@ (include "rl78-virt.md") (include "rl78-real.md") +(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no")) ;; Function Prologue/Epilogue Instructions @@ -416,7 +417,8 @@ movw ax, 0xffff6 ; MDBL movw %h0, ax ; end of mulhi macro" - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) ;; 0xFFFF0 is MACR(L). 0xFFFF2 is MACR(H) but we don't care about it @@ -496,7 +498,8 @@ movw ax, !0xf00e0 ; MDCL movw %H0, ax ; end of mulsi macro" - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) ;; start-sanitize-rl78 @@ -731,7 +734,8 @@ movw %H3, ax \n\ ; end of udivmodsi macro"; } - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) (define_insn "mov1" [(set (match_operand:QI 0 "rl78_nonimmediate_operand" "=vU") Index: gcc/config/rl78/rl78.opt =================================================================== --- gcc/config/rl78/rl78.opt (revision 2871) +++ gcc/config/rl78/rl78.opt (working copy) @@ -103,4 +103,10 @@ Target Mask(ES0) Assume ES is zero throughout program execution, use ES: for read-only data. +msave-mduc-in-interrupts +Target Mask(SAVE_MDUC_REGISTERS) +Stores the MDUC registers in interrupt handlers for G13 target. +mno-save-mduc-in-interrupts +Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS) +Does not save the MDUC registers in interrupt handlers for G13 target. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 2871) +++ gcc/doc/invoke.texi (working copy) @@ -18901,6 +18901,20 @@ registers @code{r24..r31} are reserved for use in interrupt handlers. With this option enabled these registers can be used in ordinary functions as well. + +@item -msave-mduc-in-interrupts +@item -mno-save-mduc-in-interrupts +@opindex msave-mduc-in-interrupts +@opindex mno-save-mduc-in-interrupts +Specifies that interrupt handler functions should preserve the +MDUC registers. This is only necessary if normal code might use +the MDUC registers, for example because it performs multiplication +and division operations. The default is to ignore the MDUC registers +as this makes the interrupt handlers faster. The target option -mg13 +needs to be passed for this to work as this feature is only available +on the G13 target (S2 core). The option will not have any effect if +the target does not have multiply hardware, or if the interrupt +function does not call any other function. @end table @node RS/6000 and PowerPC Options Index: gcc/config/rl78/rl78.c =================================================================== --- gcc/config/rl78/rl78.c(revision 2871) +++ gcc/config/rl78/rl78.c(working copy) @@ -83,6 +83,22 @@ "sp", "ap", "psw", "es", "cs" }; +/* Structure for G13 MDUC registers. */ +struct mduc_reg_type +{ + unsigned int address; + enum machine_mode mode; + bool is_volatile; +}; + +struct mduc_reg_type mduc_regs[NUM_OF_MDUC_REGS] = + {{0xf00e8, QImode, true}, + {0xffff0, HImode, true}, + {0xffff2, HImode, true}, + {0xf2224, HImode, true}, + {0xf00e0, HImode, true}, + {0xf00e2, HImode, true}}; + struct GTY(()) machine_function { /* If set, the rest of the fields have been computed. */ @@ -342,6 +358,10 @@ #undef TARGET_OPTION_OVERRIDE #define TARGET_OPTION_OVERRIDErl78_option_override +#define MUST_SAVE_MDUC_REGISTERS \ + (!TARGET_NO_SAVE_MDUC_REGISTERS \ + && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13) + static void rl78_option_override (void) { @@ -366,6 +386,9 @@ /* Address spaces are currently only supported by C. */ error ("-mes0 can only be used with C"); + if (TARGET_SAVE_MDUC_REGISTERS && !(TARGET_G13 || RL78_MUL_G13)) + warning (0, "mduc registers only saved for G13 target"); + switch (rl78_cpu_type) { case CPU_UNINIT: @@ -1307,6 +1330,23 @@ return (lookup_attribute ("naked", DECL_ATTRIBUTES (current_function_decl)) != NULL_TREE); } +/* Check if the block uses mul/div insns for G13 target. */ +static bool +check_mduc_usage () +{ + rtx insn; + basic_block bb; + FOR_EACH_BB_FN (bb, cfun) + { + FOR_BB_INSNS (bb, insn) + { + if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES) + return true; + } + } + return false; +} + /* Expand the function prologue (from the prologue pattern). */ void rl78_expand_prologue (void) @@ -1371,6 +1411,28 @@ F (emit_insn (gen_push (ax))); } + /* Save MDUC registers inside interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + rtx mem_mduc; + + for (int i = 0; i machine->framesize_locals + cfun->machine->framesize_outgoing; + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + fs = fs + NUM_OF_MDUC_REGS * 2; if (fs > 0) { /* If we need to subtract more than 254*3 then it is faster and @@ -1426,6 +1490,8 @@ else { fs = cfun->machine->framesize_locals + cfun->machine->framesize_outgoing; + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) +fs = fs + NUM_OF_MDUC_REGS * 2; if (fs > 254 * 3) { emit_move_insn (ax, sp); @@ -1444,6 +1510,29 @@ } } + /* Restore MDUC registers from interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + rtx mem_mduc; + + for (int i = NUM_OF_MDUC_REGS-1; i >= 0; i--) + { +emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG))); + if (mduc_regs[i].mode == QImode) + { + mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address)); + MEM_VOLATILE_P (mem_mduc) = 1; + emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG))); + } + else + { + mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address)); + MEM_VOLATILE_P (mem_mduc) = 1; + emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG))); + } + } + } + /* Save ES register inside interrupt functions. */ if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es) { @@ -1599,6 +1688,9 @@ if (cfun->machine->uses_es) fprintf (file, "\t; uses ES register\n"); + + if (MUST_SAVE_MDUC_REGISTERS) + fprintf (file, "\t; preserves MDUC registers\n"); } /* Return an RTL describing where a function return value of type RET_TYPE Index: gcc/config/rl78/rl78.h =================================================================== --- gcc/config/rl78/rl78.h(revision 2871) +++ gcc/config/rl78/rl78.h(working copy) @@ -28,6 +28,7 @@ #define TARGET_G14(rl78_cpu_type == CPU_G14) +#define NUM_OF_MDUC_REGS 6 #define TARGET_CPU_CPP_BUILTINS() \ do \ Index: gcc/config/rl78/rl78.md =================================================================== --- gcc/config/rl78/rl78.md(revision 2871) +++ gcc/config/rl78/rl78.md(working copy) @@ -78,6 +78,7 @@ (include "rl78-virt.md") (include "rl78-real.md") +(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no")) ;; Function Prologue/Epilogue Instructions @@ -416,7 +417,8 @@ movw ax, 0xffff6 ; MDBL movw %h0, ax ; end of mulhi macro" - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) ;; 0xFFFF0 is MACR(L). 0xFFFF2 is MACR(H) but we don't care about it @@ -496,7 +498,8 @@ movwax, !0xf00e0; MDCL movw%H0, ax ; end of mulsi macro" - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) ;; start-sanitize-rl78 @@ -731,7 +734,8 @@ movw%H3, ax\n\ ; end of udivmodsi macro"; } - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) (define_insn "mov1" [(set (match_operand:QI 0 "rl78_nonimmediate_operand" "=vU") Index: gcc/config/rl78/rl78.opt =================================================================== --- gcc/config/rl78/rl78.opt(revision 2871) +++ gcc/config/rl78/rl78.opt(working copy) @@ -103,4 +103,10 @@ Target Mask(ES0) Assume ES is zero throughout program execution, use ES: for read-only data. +msave-mduc-in-interrupts +Target Mask(SAVE_MDUC_REGISTERS) +Stores the MDUC registers in interrupt handlers for G13 target. +mno-save-mduc-in-interrupts +Target RejectNegative Mask(NO_SAVE_MDUC_REGISTERS) +Does not save the MDUC registers in interrupt handlers for G13 target. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi(revision 2871) +++ gcc/doc/invoke.texi(working copy) @@ -18901,6 +18901,20 @@ registers @code{r24..r31} are reserved for use in interrupt handlers. With this option enabled these registers can be used in ordinary functions as well. + +@item -msave-mduc-in-interrupts +@item -mno-save-mduc-in-interrupts +@opindex msave-mduc-in-interrupts +@opindex mno-save-mduc-in-interrupts +Specifies that interrupt handler functions should preserve the +MDUC registers. This is only necessary if normal code might use +the MDUC registers, for example because it performs multiplication +and division operations. The default is to ignore the MDUC registers +as this makes the interrupt handlers faster. The target option -mg13 +needs to be passed for this to work as this feature is only available +on the G13 target (S2 core). The option will not have any effect if +the target does not have multiply hardware, or if the interrupt +function does not call any other function. @end table @node RS/6000 and PowerPC Options