qemu-devel
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup


From: Laurent Desnogues
Subject: Re: [Qemu-devel] [PATCH 00/18] target-arm cleanup
Date: Thu, 15 Oct 2009 19:49:14 +0200

On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <address@hidden> wrote:
> On Sun, Jul 19, 2009 at 05:43:03PM +0200, Filip Navara wrote:
>> Hi,
> Hi,
>
>> the set of patches I just sent are intended to improve the TCG
>> register allocation in target-arm code. The goals are:
>>
>> - Use TCG memory-backed variables for the CPU registers
>> - Get rid of cpu_T variables
>> - Remove unused code
>>
>> Several emulation correctness bugs are also fixed by the patches, but
>> I couldn't verify if the fixes actually work (VUZP and RFE
>> instructions).
>>
>
> I have just reviewed this series of patches, you've done a good job! I
> have only a few minor comments.
>
> First of all on the code style. It's nice for patches that only affect
> one target to mention that in the title of the patch, for example by
> prefixing it with "target-arm". Also a few patches (06, 08, 11, 12, 13
> and 15) have indentation issues (tabs instead of spaces) and/or dead
> spaces at the end of lines. It's something I have fixed on my local tree,
> no need to resend the patches for that.
>
> On the code itself, I don't really like the remaining, new_tmp(),
> dead_tmp(), and even more the fact that some functions can allocate
> (e.g load_reg) or free (e.g. store_reg) some TCG variables implicitely.
> This is a way to make errors by reallocating or forgetting to free some
> variables, and that leads to strange code like:
>
> |    if (rn == 15) {
> |        tmp = new_tmp();
> |        tcg_gen_movi_i32(tmp, 0);
> |    } else {
> |        tmp = load_reg(s, rn);
> |    }
>
> ...
>
> |    if (rd != 15) {
> |        store_reg(s, rd, tmp);
> |    } else {
> |        dead_tmp(tmp);
> |    }
>
> Also I am not sure that counting the number of allocated temps has its
> place in target-arm/translate.c. It should probably be moved to
> tcg/tcg.c and enabled only when CONFIG_DEBUG_TCG is defined, so that it
> can benefits all targets.
>
> On the other hand, I fully understand that this code results from
> incremental changes from the current code. It should probably be fixed
> by an additional patch to the whole series.
>
> Otherwise, I have no specific comments to the individual patches.
>
> As the minor issue concerning TCG temp variables can be fixed later by
> a separate patch, and that it is not a regression from the current code,
> I would like, if nobody opposes, to apply this whole series (including
> my local white spaces fixes).

I had promised to take a look at the patches weeks ago, sorry
for the delay.  But talk about coincidence, I looked at them
yesterday :-)

I fully agree with Aurélien on everything he wrote (especially
the functions that implicitly allocate and free temps, that makes
the code hard to follow and potentially fragile).

I would go one step further by saying that the temps allocated
when entering disas_insn should be allocated on demand
where needed.  The impact on the speed should be measured,
but I think it would be cleaner.

And one last thing:  enable TCG_DEBUG and fix the two
problems that remain ;-)


Laurent




reply via email to

[Prev in Thread] Current Thread [Next in Thread]