From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 71828 invoked by alias); 12 Jan 2016 12:20:19 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 71816 invoked by uid 89); 12 Jan 2016 12:20:19 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=2.6 required=5.0 tests=BAYES_50,KAM_ASCII_DIVIDERS,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,SPF_HELO_PASS autolearn=no version=3.3.2 spammy=Structure, 254, 286, gen_rtx_reg X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Tue, 12 Jan 2016 12:20:18 +0000 Received: from int-mx14.intmail.prod.int.phx2.redhat.com (int-mx14.intmail.prod.int.phx2.redhat.com [10.5.11.27]) by mx1.redhat.com (Postfix) with ESMTPS id 89E63C09FAAB; Tue, 12 Jan 2016 12:20:16 +0000 (UTC) Received: from [10.36.4.36] (vpn1-4-36.ams2.redhat.com [10.36.4.36]) by int-mx14.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id u0CCKDL4029338 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Tue, 12 Jan 2016 07:20:15 -0500 Subject: Re: [PATCH : RL78] Disable interrupts during hardware multiplication routines To: Kaushik M Phatak References: Cc: dj@redhat.com, Mike Stump , gcc-patches@gcc.gnu.org, "kaushik.phatak@kpit.com" From: Nick Clifton Message-ID: <5694EF7D.4090204@redhat.com> Date: Tue, 12 Jan 2016 12:20:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.5.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2016-01/txt/msg00702.txt.bz2 Hi Kaushik, > +/* 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}}; If the is_volatile field is true for all members of this array, why bother having it at all ? (If I remember correctly in your previous patch only some of these addresses were being treated as volatile registers, not all of them). > +/* Check if the block uses mul/div insns for G13 target. */ > +static bool > +check_mduc_usage () Add a void type to the declaration. Ie: check mduc_usage (void) > +{ > + rtx insn; > + basic_block bb; > + FOR_EACH_BB_FN (bb, cfun) > + { You should have a blank line between the end of the variable declarations and the start of the code. > + FOR_BB_INSNS (bb, insn) > + { > + if (get_attr_is_g13_muldiv_insn (insn) == IS_G13_MULDIV_INSN_YES) > + return true; I am not sure - but it might be safer to check INSN_P(insn) first before checking for the muldiv attribute. > + for (int i = 0; i + { > +if (mduc_regs[i].mode == QImode) > +{ Indentation. > + mem_mduc = gen_rtx_MEM (QImode, GEN_INT (mduc_regs[i].address)); > + MEM_VOLATILE_P (mem_mduc) = 1; > + emit_insn (gen_movqi (gen_rtx_REG (QImode, A_REG), mem_mduc)); > +} > +else > +{ > + mem_mduc = gen_rtx_MEM (HImode, GEN_INT (mduc_regs[i].address)); > + MEM_VOLATILE_P (mem_mduc) = 1; > + 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: for (int i = 0; i < NUM_OF_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) = reg->is_volatile; 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))); } > 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 > 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) No - this is wrong. "fs" is the amount of extra space needed in the stack frame to hold local variables and outgoing variables. It should not include the stack space used for already pushed registers. > 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 Why define this here ? It is only ever used in rl78,c and it can be computed automatically by applying the ARRAY_SIZE macro to the mduc_regs array. > 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. 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. > Index: gcc/doc/invoke.texi > =================================================================== > --- gcc/doc/invoke.texi(revision 2871) > +++ gcc/doc/invoke.texi(working copy) You should also add the name of the new option to the Machine Dependent Options section of the manual. (Approx line 896 in invoke.texi) > +@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. Still not quite right. The last sentence should be: The MDUC registers will only be saved if the interrupt handler performs a multiplication or division operation or it calls another function. Cheers Nick