From patchwork Wed May 4 11:47:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Kaushik Phatak X-Patchwork-Id: 618375 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 3r0GWp37w2z9sXR for ; Wed, 4 May 2016 21:48:20 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=gcc.gnu.org header.i=@gcc.gnu.org header.b=j3kPocbH; 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:mime-version; q=dns; s=default; b=cKB/0jaibJbvL/Of o0sZbBigOFTWu8r7HSB5svOsri7R53GFUUVMOk7X1j6B2X5cWX3TA1NjzHgGA5Zg 6V8UJrPvoDxctaOPpc+AxJsiyNDhwsWGttF/UZrVBDxkliSMCCe9ds0UGzxGe9T/ lBAuGJpiHCV9RrYZPbVDFE3i4cs= 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:mime-version; s=default; bh=18FeOVNiFxm4nzNkrfXQTF q5t74=; b=j3kPocbH9o8a3vToE3OoXqWwgawf1UoQWPxgbqfotb7oFgFeKTE6KV Rnknb15Ud9xbfNDyd6KEOrxjMnzqvVRLFSfEGlqksnwhoifVedrSjb1Kbg2ToaCt EG2KK+zgmkohhIg3vm2DE790i1sxwMJQsDRustcvpKKZsyl5jRUBA= Received: (qmail 12906 invoked by alias); 4 May 2016 11:47:52 -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 12428 invoked by uid 89); 4 May 2016 11:47:51 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.6 required=5.0 tests=BAYES_50, KAM_ASCII_DIVIDERS, RCVD_IN_DNSWL_NONE, SPF_HELO_PASS, SPF_PASS autolearn=no version=3.3.2 spammy=H*M:PROD, H*M:OUTLOOK, H*M:INDPRD01, Stump X-HELO: IND01-MA1-obe.outbound.protection.outlook.com Received: from mail-ma1ind01on0060.outbound.protection.outlook.com (HELO IND01-MA1-obe.outbound.protection.outlook.com) (104.47.100.60) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-SHA256 encrypted) ESMTPS; Wed, 04 May 2016 11:47:41 +0000 Received: from MA1PR01MB0231.INDPRD01.PROD.OUTLOOK.COM (10.164.120.6) by MA1PR01MB0230.INDPRD01.PROD.OUTLOOK.COM (10.164.120.5) with Microsoft SMTP Server (TLS) id 15.1.485.9; Wed, 4 May 2016 11:47:33 +0000 Received: from MA1PR01MB0231.INDPRD01.PROD.OUTLOOK.COM ([10.164.120.6]) by MA1PR01MB0231.INDPRD01.PROD.OUTLOOK.COM ([10.164.120.6]) with mapi id 15.01.0485.011; Wed, 4 May 2016 11:47:32 +0000 From: Kaushik Phatak To: Nick Clifton , Mike Stump CC: "dj@redhat.com" , "gcc-patches@gcc.gnu.org" , Kaushik M Phatak Subject: RE: [PATCH : RL78] Disable interrupts during hardware multiplication routines Date: Wed, 4 May 2016 11:47:32 +0000 Message-ID: References: <06B89EF5-E403-4E5A-A356-A0EDA05246A6@comcast.net> In-Reply-To: <06B89EF5-E403-4E5A-A356-A0EDA05246A6@comcast.net> authentication-results: redhat.com; dkim=none (message not signed) header.d=none; redhat.com; dmarc=none action=none header.from=kpit.com; x-ms-office365-filtering-correlation-id: 2f7cd612-1f2e-4dce-c7cc-08d37411e11e x-microsoft-exchange-diagnostics: 1; MA1PR01MB0230; 5:i2kVScwFn8yn9R3QO6o6fLjA021Mt/nc78v5G+yHkikE8F7HR1KAAM7YnXLZArMTTaZAmi/XPXC9UVEziz8ea3GbjTKs0kqyrQveF9/yQ7hJIhzuidsttKYLMSzG36tkVTFLkNZVwk5/08riOnclAw==; 24:849OpfAhGTOvo15KMgrYvYMjCyf4rewQpo9R0pRweaqak8xUI9T78ZD3nGe6dSztGrLgSmT9GmbhCBzIPy6t5sDRdfkRmpkdMZG3GrXDaKY=; 7:xTSbviPxJQm8gJJlMHjPG44Z5PASq7LDIb/qQQC/MxvKgZb8F5QJZEgh66XxbZAC/SgCnFfc4nLySfXcruYG8E6mc5KAmFvjGNlAb1JlHeKClFRX6IBWPz0nmiecPiuYa5KNJVmX8QB80E05EkLQGT2XFeD9Vomb5SbU0QUlfe4= x-microsoft-antispam: UriScan:;BCL:0;PCL:0;RULEID:;SRVR:MA1PR01MB0230; x-microsoft-antispam-prvs: x-exchange-antispam-report-test: UriScan:; x-exchange-antispam-report-cfa-test: BCL:0; PCL:0; RULEID:(102415293)(102615271)(9101521098)(9101528026)(2401047)(8121501046)(5005006)(10201501046)(3002001)(6055026); SRVR:MA1PR01MB0230; BCL:0; PCL:0; RULEID:; SRVR:MA1PR01MB0230; x-forefront-prvs: 093290AD39 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(6009001)(24454002)(13464003)(377454003)(377424004)(10400500002)(2950100001)(81166005)(2900100001)(189998001)(5004730100002)(9686002)(87936001)(107886002)(3280700002)(1220700001)(6116002)(102836003)(3846002)(586003)(86362001)(3660700001)(8936002)(15975445007)(99936001)(4001450100002)(54356999)(77096005)(76176999)(50986999)(122556002)(66066001)(5002640100001)(4326007)(19580395003)(19580405001)(33656002)(92566002)(5008740100001)(2906002)(5001770100001)(4001430100002)(106116001)(5003600100002)(450100001); DIR:OUT; SFP:1101; SCL:1; SRVR:MA1PR01MB0230; H:MA1PR01MB0231.INDPRD01.PROD.OUTLOOK.COM; FPR:; SPF:None; MLV:sfv; LANG:en; MIME-Version: 1.0 X-OriginatorOrg: kpit.com X-MS-Exchange-CrossTenant-originalarrivaltime: 04 May 2016 11:47:32.2184 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: 3539451e-b46e-4a26-a242-ff61502855c7 X-MS-Exchange-Transport-CrossTenantHeadersStamped: MA1PR01MB0230 Hi Nick, I have modified and updated this patch as per your comments. Apologies, as it has taken me awhile for me to get back to this. https://gcc.gnu.org/ml/gcc-patches/2016-01/msg00702.html >> +/* 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}, > If the is_volatile field is true for all members of this array, why bother having it at all ? I have got rid of the unnecessary volatile field here. >> +check_mduc_usage () >Add a void type to the declaration. Done. > You should have a blank line between the end of the variable > declarations and the start of the code. Done. >> > + if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES) > I am not sure - but it might be safer to check INSN_P(insn) first Added an INSN_P check here. >> > +if (mduc_regs[i].mode == QImode) >> +{ > Indentation. Hopefully this is fixed. Some issue in editor when moving from tabs to spaces. >> + emit_insn (gen_movqi (gen_rtx_REG (HImode, AX_REG), mem_mduc)); >> +} >In the else case you are using gen_movqi to move an HImode value... >Also you could simplify the above code like this: Fixed this, also used a simpler logic as suggested. >> > +fs = fs + NUM_OF_MDUC_REGS * 2; >> if (fs > 254 * 3) > No - this is wrong. "fs" is the amount of extra space needed in the Sorry, I think I misread this. I have added code at top or prologue which will update cfun->machine->framesize. >> +#define NUM_OF_MDUC_REGS 6 > Why define this here ? It is only ever used in rl78,c and it can be > computed automatically by applying the ARRAY_SIZE macro I have removed this marco and used ARRAY_SIZE to compute the size instead. >> +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. >This looks wrong. Surely you only need the msave-mduc-in-interrupts >definition. That will automatically allow -mno-save-mduc-in-interrupts, >since it does not have the RejectNegative attribute. Also these is no >need to have two separate target mask bits. Just SAVE_MDUC_REGISTERS >will do. Well, the earlier idea was to save the MDUC registers by default for G13 targets. Hence the '-mno-' was introduced, but I can go with your suggestion if it reduces any confusion. >> +++ gcc/doc/invoke.texi(working copy) > You should also add the name of the new option to the Machine Dependent Added the new option to the list. >> +@item -msave-mduc-in-interrupts >Still not quite right. The last sentence should be: > The MDUC registers will only be saved Update the last line of the manual as per your suggestion. >> My review comment is still outstanding. - from Mike Stump The current RL78 ABI does not contain specific information about these registers from the G13 variant of the RL78 target. We can try and request Renesas to add information about the same along with the option required for this. Nick, do you have any thoughts on this? (assuming this version of patch is closer to acceptance) The patch is regression tested for "-msim -mg13 -msave-mduc-in-interrupts". Best Regards, Kaushik gcc/ChangeLog 2016-05-04 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_REGISTERS): 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. * doc/invoke.texi (RL78 Options): Add -msave-mduc-in-interrupts. ===== END of PATCH ====== -----Original Message----- From: Mike Stump [mailto:mikestump@comcast.net] Sent: Tuesday, January 12, 2016 9:22 PM To: Kaushik M Phatak Cc: Nick Clifton ; dj@redhat.com; gcc-patches@gcc.gnu.org; Kaushik Phatak Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines On Jan 11, 2016, at 11:20 PM, Kaushik M Phatak wrote: > Kindly review the updated patch and let me know if it is OK. My review comment is still outstanding. Index: gcc/config/rl78/rl78.c =================================================================== --- gcc/config/rl78/rl78.c (revision 235865) +++ gcc/config/rl78/rl78.c (working copy) @@ -76,6 +76,21 @@ "sp", "ap", "psw", "es", "cs" }; +/* Structure for G13 MDUC registers. */ +struct mduc_reg_type +{ + unsigned int address; + enum machine_mode mode; +}; + +struct mduc_reg_type mduc_regs[] = + {{0xf00e8, QImode}, + {0xffff0, HImode}, + {0xffff2, HImode}, + {0xf2224, HImode}, + {0xf00e0, HImode}, + {0xf00e2, HImode}}; + struct GTY(()) machine_function { /* If set, the rest of the fields have been computed. */ @@ -317,6 +332,10 @@ #undef TARGET_OPTION_OVERRIDE #define TARGET_OPTION_OVERRIDE rl78_option_override +#define MUST_SAVE_MDUC_REGISTERS \ + (TARGET_SAVE_MDUC_REGISTERS \ + && (is_interrupt_func (NULL_TREE)) && RL78_MUL_G13) + static void rl78_option_override (void) { @@ -344,6 +363,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: @@ -1263,6 +1285,25 @@ 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 (void) +{ + rtx insn; + basic_block bb; + + FOR_EACH_BB_FN (bb, cfun) + { + FOR_BB_INSNS (bb, insn) + { + if (INSN_P (insn) + && (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) @@ -1278,6 +1319,9 @@ /* Always re-compute the frame info - the register usage may have changed. */ rl78_compute_frame_info (); + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + cfun->machine->framesize += ARRAY_SIZE (mduc_regs) * 2; + if (flag_stack_usage_info) current_function_static_stack_size = cfun->machine->framesize; @@ -1327,6 +1371,24 @@ F (emit_insn (gen_push (ax))); } + /* Save MDUC registers inside interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + for (int i = 0; i < ARRAY_SIZE (mduc_regs); i++) + { + mduc_reg_type *reg = mduc_regs + i; + rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address)); + + MEM_VOLATILE_P (mem_mduc) = 1; + if (reg->mode == QImode) + emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc)); + else + emit_insn (gen_movhi (gen_rtx_REG (HImode, AX_REG), mem_mduc)); + + emit_insn (gen_push (gen_rtx_REG (HImode, AX_REG))); + } + } + if (frame_pointer_needed) { F (emit_move_insn (ax, sp)); @@ -1400,6 +1462,23 @@ } } + /* Restore MDUC registers from interrupt routine. */ + if (MUST_SAVE_MDUC_REGISTERS && (!crtl->is_leaf || check_mduc_usage ())) + { + for (int i = ARRAY_SIZE (mduc_regs) - 1; i >= 0; i--) + { + mduc_reg_type *reg = mduc_regs + i; + rtx mem_mduc = gen_rtx_MEM (reg->mode, GEN_INT (reg->address)); + + emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG))); + MEM_VOLATILE_P (mem_mduc) = 1; + if (reg->mode == QImode) + emit_insn (gen_movqi (mem_mduc, gen_rtx_REG (QImode, A_REG))); + else + emit_insn (gen_movhi (mem_mduc, gen_rtx_REG (HImode, AX_REG))); + } + } + if (is_interrupt_func (cfun->decl) && cfun->machine->uses_es) { emit_insn (gen_pop (gen_rtx_REG (HImode, AX_REG))); @@ -1495,6 +1574,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.md =================================================================== --- gcc/config/rl78/rl78.md (revision 235865) +++ gcc/config/rl78/rl78.md (working copy) @@ -67,6 +67,7 @@ (include "rl78-virt.md") (include "rl78-real.md") +(define_attr "is_g13_muldiv_insn" "yes,no" (const_string "no")) ;; Function Prologue/Epilogue Instructions @@ -379,7 +380,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 @@ -459,7 +461,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")] ) (define_expand "udivmodhi4" @@ -692,5 +695,6 @@ movw %H3, ax \n\ ; end of udivmodsi macro"; } - [(set_attr "valloc" "macax")] + [(set_attr "valloc" "macax") + (set_attr "is_g13_muldiv_insn" "yes")] ) Index: gcc/config/rl78/rl78.opt =================================================================== --- gcc/config/rl78/rl78.opt (revision 235865) +++ gcc/config/rl78/rl78.opt (working copy) @@ -91,3 +91,7 @@ mes0 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. Index: gcc/doc/invoke.texi =================================================================== --- gcc/doc/invoke.texi (revision 235865) +++ gcc/doc/invoke.texi (working copy) @@ -946,7 +946,7 @@ @emph{RL78 Options} @gccoptlist{-msim -mmul=none -mmul=g13 -mmul=g14 -mallregs @gol -mcpu=g10 -mcpu=g13 -mcpu=g14 -mg10 -mg13 -mg14 @gol --m64bit-doubles -m32bit-doubles} +-m64bit-doubles -m32bit-doubles -msave-mduc-in-interrupts} @emph{RS/6000 and PowerPC Options} @gccoptlist{-mcpu=@var{cpu-type} @gol @@ -19777,6 +19777,20 @@ or 32 bits (@option{-m32bit-doubles}) in size. The default is @option{-m32bit-doubles}. +@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 MDUC registers will only be saved +if the interrupt handler performs a multiplication or division +operation or it calls another function. + @end table @node RS/6000 and PowerPC Options