Re: [Qemu-devel] [PATCH v5 18/19] tcg-arm: Convert to CONFIG_QEMU_LDST_OPTIMIZATION

[Aurelien Jarno <aurelien@aurel32.net>]

Le 23/04/2013 10:13, Richard Henderson a écrit :
> On 2013-04-23 07:44, Aurelien Jarno wrote:
>> On Mon, Apr 22, 2013 at 03:39:42PM +0100, Richard Henderson wrote:
>>> On 2013-04-22 13:59, Aurelien Jarno wrote:
>>>>>> +    /* The code buffer is limited to 16MB, with the prologue located
>>>>>> +       at the end of it.  Therefore we needn't care for any out of
>>>>>> +       range branches.  */
>>>>>> +    assert(val - 8 < 0x01fffffd && val - 8 > -0x01fffffd);
>>>>>> +
>>>>>> +    tcg_out_b(s, cond, val);
>>>> While this is currently true, I am not sure we want to get rid of that
>>>> code, and I hope we'll be able to eventually get rid of the 16MB limit.
>>>>
>>>> For me this dramatically reduce the boot time of guests. That said it is
>>>> not a real benchmark, and it should theoretically reduce the
>>>> performances in some cases as doing so interleaves code and data.
>>>> Someone has to spend time doing benchmarks before we can progress on that.
>>>>
>>>
>>> Ok, sure, but how do we progress in the short term?
>>> Certainly rejecting this patch and its changes to tcg_out_goto
>>> is not compatible with approving  19/19, which relies on it.
>>
>> Oh, I haven't seen realized that the comment about goto not using
>> multi-insns ops was referring to patch 18.
>>
>>> IMO, tcg_out_goto should be the simple case of goto within a TB,
>>> with only the code for INDEX_goto_tb needing to handle the >16MB case.
>>>
>>
>> >From what I have seen in the code, we need to do jumps in the following
>> conditions:
>> - jumps within TB
>> - jumps between slow/fast path in the ld/st code
>> - jump to the epilogue
>> - jump between TB
>>
>> Currently the first three use tcg_goto_out, while only the third one
>> need to (eventually) jump above the 16MB limit.
> 
> The epilogue is exactly two insns.  When we eliminate the 16MB limit
> we should just fold the add sp + pop into the exit_tb.  We can't do
> better than that.
> 
>> The last one should also jump above the 16MB limit, but do not use
>> tcg_goto_out.
> 
> Agreed.
> 
>> For jumps within TB tcg_out_goto_label can use tcg_out_b directly,
>> as tcg_out_b_noaddr is already used in the other pass, and the only
>> benefit of tcg_goto_out over tcg_out_b is the asserts, which can not
>> really been triggered here.
>>
>> For jumps from the slow path to the fast path, I think tcg_out_b
>> can also be used, as anyway tcg_out_b_noaddr is already used from the
>> fast path to the slow path.
>>
>> This leaves out tcg_goto_out used only for the jump to the epilogue, and
>> it can be modified later when someone works on the 16MB limit issue. Of
>> course that means that patch 19 has to be dropped, but I don't think
>> it's a big issue.
> 
> Ok, I'll drop patch 19 for now so that we can make progress.
> 

If you plan to respin the series, do you want I start applying the first
patches, at least the "Fix local stack frame" one that we want to see in
1.5 (and stable)?

-- 
Aurelien Jarno                          GPG: 1024D/F1BCDB73
aurelien@aurel32.net                 http://www.aurel32.net