Re: [Qemu-devel] [PATCH 3/3] tcg/aarch64: implement new TCG target for aarch64

[Laurent Desnogues]

On Tue, May 14, 2013 at 2:01 PM, Claudio Fontana
<address@hidden> wrote:
[...]
>>> +static void tcg_target_qemu_prologue(TCGContext *s)
>>> +{
>>> +    int r;
>>> +    int frame_size; /* number of 16 byte items */
>>> +
>>> +    /* we need to save (FP, LR) and X19 to X28 */
>>> +    frame_size = (1) + (TCG_REG_X27 - TCG_REG_X19) / 2 + 1;
>>
>> The comment says "X19 to X28" and the code does X27 - X19:
>> which is right?
>
> The comment is right, and the code is technically working but it shows as 
> misleading for the reader.
> I will rewrite the callee-saved registers' contribution to the frame size (in 
> 16 byte elements) as:
>
> (TCG_REG_X28 - TCG_REG_X19) / 2 + 1;

Shouldn't that be like this?

((TCG_REG_X28 - TCG_REG_X19 + 1) + 1) / 2 + 1;

The last +1 is for FP,LR as you explained.
The first +1 is needed to count the number of regs in the
interval [x19,x28].
The second +1 is needed because if the number of regs
is odd, you want to round up and not down.

Here that'd give us (9+1+1)/2+1 = 6.

Of course that's nitpicking because the callee-saved regs
shouldn't change :-)

>>
>> Why the brackets round the first '1' ?
>
> It represents the (FP, LR) pair, so the () should help the reader notice that 
> it refers
> to the couple (FP, LR) mentioned in the comment just above.
> It clearly failed, so I will try to align the comment and the code better.
>
>>
>>> +
>>> +    /* push (fp, lr) and update sp to final frame size */
>>> +    tcg_out_push_p(s, TCG_REG_FP, TCG_REG_LR, frame_size);
>>> +
>>> +    /* FP -> frame chain */
>>> +    tcg_out_movr_sp(s, 1, TCG_REG_FP, TCG_REG_SP);
>>> +
>>> +    /* store callee-preserved regs x19..x28 */
>>> +    for (r = TCG_REG_X19; r <= TCG_REG_X27; r += 2) {

Shouldn't the comparison be against TCG_REG_X28?  It'd
be more readable.

>>> +        int idx; idx = (r - TCG_REG_X19) / 2 + 1;
>>> +        tcg_out_store_p(s, r, r + 1, idx);
>>> +    }
>>> +
>>> +    tcg_out_mov(s, TCG_TYPE_PTR, TCG_AREG0, tcg_target_call_iarg_regs[0]);
>>> +    tcg_out_gotor(s, tcg_target_call_iarg_regs[1]);
>>> +
>>> +    tb_ret_addr = s->code_ptr;
>>> +
>>> +    /* restore registers x19..x28 */
>>> +    for (r = TCG_REG_X19; r <= TCG_REG_X27; r += 2) {

Ditto.

Thanks,

Laurent

>>> +        int idx; idx = (r - TCG_REG_X19) / 2 + 1;
>>> +        tcg_out_load_p(s, r, r + 1, idx);
>>> +    }
>>> +
>>> +    /* pop (fp, lr), restore sp to previous frame, return */
>>> +    tcg_out_pop_p(s, TCG_REG_FP, TCG_REG_LR, frame_size);
>>> +    tcg_out_ret(s);
>>> +}