From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:41716) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbwWt-000644-QS for qemu-devel@nongnu.org; Tue, 15 Sep 2015 16:08:40 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1ZbwWo-0003QN-MC for qemu-devel@nongnu.org; Tue, 15 Sep 2015 16:08:39 -0400 Received: from mail-qg0-x22e.google.com ([2607:f8b0:400d:c04::22e]:33080) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1ZbwWo-0003PZ-Ez for qemu-devel@nongnu.org; Tue, 15 Sep 2015 16:08:34 -0400 Received: by qgev79 with SMTP id v79so153339390qge.0 for ; Tue, 15 Sep 2015 13:08:33 -0700 (PDT) Sender: Richard Henderson References: <1441173123-25540-1-git-send-email-rth@twiddle.net> <1441173123-25540-19-git-send-email-rth@twiddle.net> From: Richard Henderson Message-ID: <55F87ABE.6060506@twiddle.net> Date: Tue, 15 Sep 2015 13:08:30 -0700 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PATCH 18/20] tcg: Save insn data and use it in cpu_restore_state_from_tb List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Peter Maydell Cc: dl.soluz@gmx.net, QEMU Developers , Aurelien Jarno , Artyom Tarasenko On 09/10/2015 06:49 AM, Peter Maydell wrote: >> + tcg_debug_assert(num_insns >= 0); > > This is claiming that every TB will have at least one insn_start, > right? I think that most targets will violate that in the breakpoint > case, because the "if we have a bp for this insn then generate a > debug insn and break out of the loop" code is before the call > to tcg_gen_insn_start(). > > We should probably assert that num_insns < TCG_MAX_INSNS while > we're here. True. I wonder if we shouldn't fix bp placement while I'm at it. And the assertion should really be num_insns == tb->icount. >> +static target_long decode_sleb128(uint8_t **pp) >> +{ >> + uint8_t *p = *pp; >> + target_long val = 0; >> + int byte, shift = 0; >> + >> + do { >> + byte = *p++; >> + val |= (target_ulong)(byte & 0x7f) << shift; >> + shift += 7; >> + } while (byte & 0x80); >> + if (shift < TARGET_LONG_BITS && (byte & 0x40)) { >> + val |= -(target_ulong)1 << shift; >> + } >> + >> + *pp = p; >> + return val; >> +} > > Are the encode/decode sleb128 functions known-good ones > borrowed from somewhere else? Yes, from libgcc. > (PS: checkpatch complains about missing braces.) Ho hum... >> +static int encode_search(TranslationBlock *tb, uint8_t *block) >> +{ > > I think this function would benefit from a brief comment > describing the compressed format we're creating here. Yes. >> gen_code_size = tcg_gen_code(&tcg_ctx, gen_code_buf); >> + search_size = encode_search(tb, (void *)gen_code_buf + gen_code_size); > > Now we're putting the encoded search info in the codegen buffer, > don't we need to adjust the calculation of code_gen_buffer_max_size > to avoid falling off the end if the last TB in the buffer has a very > large set of generated TCG code and also a big encoded search buffer? Dunno. It's not that we've ever checked for this before; I'm not sure what factor I would actually apply. > It would also be nice to assert if we do fall off the end of the > buffer somehow. Given that we generally use a very large mmap to allocate it, perhaps simply adding a guard page would be best. > How much extra space does the encoded search typically take (as a > % of the gen_code_size, say)? Dunno; I'll have to have a look at that. Probably easiest to just enhance info jit... r~