From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([208.118.235.92]:57094) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn6b5-0005K6-4i for qemu-devel@nongnu.org; Fri, 06 Jul 2012 07:21:21 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Sn6ay-0002Mf-KD for qemu-devel@nongnu.org; Fri, 06 Jul 2012 07:21:14 -0400 Received: from mailout4.samsung.com ([203.254.224.34]:24894) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Sn6ay-0002M9-5D for qemu-devel@nongnu.org; Fri, 06 Jul 2012 07:21:08 -0400 Received: from epcpsbgm1.samsung.com (mailout4.samsung.com [203.254.224.34]) by mailout4.samsung.com (Oracle Communications Messaging Server 7u4-24.01(7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTP id <0M6Q00JR0KUMD801@mailout4.samsung.com> for qemu-devel@nongnu.org; Fri, 06 Jul 2012 20:20:51 +0900 (KST) Received: from [172.21.111.108] ([182.198.1.3]) by mmp1.samsung.com (Oracle Communications Messaging Server 7u4-24.01 (7.0.4.24.0) 64bit (built Nov 17 2011)) with ESMTPA id <0M6Q00MBLKURYQ40@mmp1.samsung.com> for qemu-devel@nongnu.org; Fri, 06 Jul 2012 20:20:51 +0900 (KST) Message-id: <4FF6CA14.6020506@samsung.com> Date: Fri, 06 Jul 2012 20:20:52 +0900 From: Yeongkyoon Lee MIME-version: 1.0 References: <1341494619-4714-1-git-send-email-yeongkyoon.lee@samsung.com> <1341494619-4714-4-git-send-email-yeongkyoon.lee@samsung.com> In-reply-to: Content-type: text/plain; charset=UTF-8; format=flowed Content-transfer-encoding: 7bit Subject: Re: [Qemu-devel] [RFC][PATCH v2 3/4] tcg: add optimized TCG qemu_ld/st generation List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: laurent.desnogues@gmail.com, e.voevodin@samsung.com, qemu-devel@nongnu.org, chenwj@iis.sinica.edu.tw > Is it really worth having this as a CONFIG_ switch? If we think > it's better to do this out of line we should just switch to > always generating the out of line code, I think. There's not much > point in retaining the old code path if it's disabled -- it will > just bitrot. I agree. However, it is just a safe guard because I have not test all the targets of qemu. I've only tested x86 and ARM targets on x86 and x86-64 hosts. If agreed to remove conditional macro, then I'll fix it. >> +#ifdef CONFIG_QEMU_LDST_OPTIMIZATION >> + /* jne slow_path */ >> + tcg_out_opc(s, OPC_JCC_long + JCC_JNE, 0, 0, 0); >> + if (!label_ptr) { >> + tcg_abort(); >> + } > There's no point in this check and abort -- label_ptr will always be > non-NULL (it would be an internal error if it wasn't), and if it is by > some future bug NULL, we'll just crash on the next line, which is just > as good. The existing code didn't feel the need to make this check, we > don't need to do it in the new code. It cannot be happened now as you said. It is just for a possible future bug. But I cannot understand "we'll just crash on the next line" you mentioned above. >> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION) >> + /* helper stub will be jumped back here */ > "will jump back here". Ok. > >> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION) >> + /* helper stub will be jumped back here */ > ditto. Ok. >> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION) >> +/* optimization to reduce jump overheads for qemu_ld/st IRs */ >> + >> +/* >> + * qemu_ld/st code generator call add_qemu_ldst_label, >> + * so that slow case(TLB miss or I/O rw) is handled at the end of TB >> + */ > This comment isn't really describing the purpose of this function, > which is something more along the lines of "Record the context of > a call to the out of line helper code for the slow path for a > load or store, so that we can later generate the correct helper > code". I agree. Your description looks better. > >> + if (s->nb_qemu_ldst_labels >= TCG_MAX_QEMU_LDST) >> + tcg_abort(); > QEMU coding style requires braces. Please use checkpatch.pl. Ok. > >> + >> + idx = s->nb_qemu_ldst_labels++; >> + label = (TCGLabelQemuLdst *)&s->qemu_ldst_labels[idx]; >> + label->opc_ext = opc_ext; >> + label->datalo_reg = data_reg; >> + label->datahi_reg = data_reg2; >> + label->addrlo_reg = addrlo_reg; >> + label->addrhi_reg = addrhi_reg; >> + label->mem_index = mem_index; >> + label->raddr = raddr; >> + if (!label_ptr) { >> + tcg_abort(); >> + } > Another pointless abort. ditto. > >> diff --git a/tcg/tcg.c b/tcg/tcg.c >> index 8386b70..8009069 100644 >> --- a/tcg/tcg.c >> +++ b/tcg/tcg.c >> @@ -301,6 +301,14 @@ void tcg_func_start(TCGContext *s) >> >> gen_opc_ptr = gen_opc_buf; >> gen_opparam_ptr = gen_opparam_buf; >> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION) >> + /* initialize qemu_ld/st labels which help to generate TLB miss case codes at the end of TB */ >> + s->qemu_ldst_labels = tcg_malloc(sizeof(TCGLabelQemuLdst) * TCG_MAX_QEMU_LDST); >> + if (!s->qemu_ldst_labels) { >> + tcg_abort(); >> + } > Unnecessary check -- tcg_malloc() can never return 0. Ok. >> +#if defined(CONFIG_QEMU_LDST_OPTIMIZATION) >> +/* Macros and structures for qemu_ld/st IR code optimization: >> + It looks good for TCG_MAX_HELPER_LABELS to be half of OPC_BUF_SIZE in exec-all.h. */ >> +#define TCG_MAX_QEMU_LDST 320 > Is that true even if you have a huge block with nothing but simple > guest load instructions in it? I agree. It needs to be set as same size with OPC_BUF_SIZE for covering extreme cases.