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

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

[Filip Navara]

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).

Best regards,
Filip Navara

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

[Aurelien Jarno]

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).

Regards,
Aurelien

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

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

[Laurent Desnogues]

On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <aurelien@aurel32.net> 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

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

[Aurelien Jarno]

On Thu, Oct 15, 2009 at 07:49:14PM +0200, Laurent Desnogues wrote:
> On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <aurelien@aurel32.net> 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 ;-)
> 

Good catch. It concerns patches 01 and 13, I have just send comments to
these patches.

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

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

[Filip Navara]

Hi!

On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
[snip]
> 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.

I'll make sure not to mess it up next time.

> 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.

Neither do I, but removing that altogether would make the patch series
way too big.

> 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.

Fully agreed.

> 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.

Yep.

> Otherwise, I have no specific comments to the individual patches.

Apart from the points you have raised about specific patches there
were few more minor bugs in the series spotted by Juha Riihimäki
<juha.riihimaki@nokia.com>. A fix is available at
http://repo.or.cz/w/qemu/navara.git?a=commit;h=2ce696baa6fc5d99522cf387b6a4913807fd43ed

> 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'd be glad if the series gets applied, provided that your fixes and
the one referenced above is included. If necessary I can resend the
whole series with the fixes included, but I'd rather avoid it.

Best regards,
Filip Navara

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

[Aurelien Jarno]

On Fri, Oct 16, 2009 at 05:41:14PM +0200, Filip Navara wrote:
> Hi!
> 
> On Thu, Oct 15, 2009 at 7:40 PM, Aurelien Jarno <aurelien@aurel32.net> wrote:
> [snip]
> > 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.
> 
> I'll make sure not to mess it up next time.
> 
> > 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.
> 
> Neither do I, but removing that altogether would make the patch series
> way too big.
> 
> > 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.
> 
> Fully agreed.
> 
> > 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.
> 
> Yep.
> 
> > Otherwise, I have no specific comments to the individual patches.
> 
> Apart from the points you have raised about specific patches there
> were few more minor bugs in the series spotted by Juha Riihimäki
> <juha.riihimaki@nokia.com>. A fix is available at
> http://repo.or.cz/w/qemu/navara.git?a=commit;h=2ce696baa6fc5d99522cf387b6a4913807fd43ed

One of the fix was already in my branch (catched by Laurent Desnogues).
I have added the other fixes in my branch. The last to hunks are good
example why temp new/free should be done explicitly ;)

> > 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'd be glad if the series gets applied, provided that your fixes and
> the one referenced above is included. If necessary I can resend the
> whole series with the fixes included, but I'd rather avoid it.
> 

I'll merge that over the week-end if there are no more comments.

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

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

[Juha.Riihimaki]

>> Apart from the points you have raised about specific patches there
>> were few more minor bugs in the series spotted by Juha Riihimäki
>> <juha.riihimaki@nokia.com>. A fix is available at
>> http://repo.or.cz/w/qemu/navara.git?a=commit;h=2ce696baa6fc5d99522cf387b6a4913807fd43ed
>
> One of the fix was already in my branch (catched by Laurent  
> Desnogues).
> I have added the other fixes in my branch. The last to hunks are good
> example why temp new/free should be done explicitly ;)

I think I have a couple of other fixes and patches on top of that as  
well, but I'd rather wait until you get this bunch committed and then  
format the patches against the new mainline so that they apply.

On the subject of the new_tmp/dead_tmp, can you elaborate how critical  
it is if there are resource leaks in the translator code, i.e. if some  
of the temporary variables don't get marked dead? I suppose the  
leakage would only affect the translation of the code block where it  
appears? I found more leaks by introducing a new_tmp64/dead_tmp64 and  
some other checkups to catch programming errors.

Some of the generated tcg code is not very optimal, for example a  
single vld4.8 instruction can generate over 250 tcg ops. I did some  
optimizations and got it under 200 but do you think it could be an  
issue that a single instruction can expand to so many tcg ops? I mean  
besides the fact that it causes TBs for only one or two guest  
instructions to be generated.

Perhaps this would also be a good place and time to also ask whether  
you would be interested in integrating support for OMAP3 in the QEMU  
mainline? We have been developing and using it for several months now,  
it's based on the work done by Yajin <yajin@vm-kernel.org> to support  
the OMAP3 BeagleBoard hardware. We have added support for the Nokia  
N900 system emulation as well. In my personal opinion the OMAP3  
emulation is in functionality on par with the existing OMAP2  
emulation, if not even more complete.


Cheers,
Juha

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

[Laurent Desnogues]

On Mon, Oct 19, 2009 at 8:23 AM,  <Juha.Riihimaki@nokia.com> wrote:
>
> I think I have a couple of other fixes and patches on top of that as
> well, but I'd rather wait until you get this bunch committed and then
> format the patches against the new mainline so that they apply.

It's already been committed (the commit notification process is
again dead).

> On the subject of the new_tmp/dead_tmp, can you elaborate how critical
> it is if there are resource leaks in the translator code, i.e. if some
> of the temporary variables don't get marked dead? I suppose the
> leakage would only affect the translation of the code block where it
> appears? I found more leaks by introducing a new_tmp64/dead_tmp64 and
> some other checkups to catch programming errors.

Yes it would only affect one TB.  The price would be a higher time
spent in the TCG -> host code generation pass, since some parts
are linear in the number of live temps.

> Some of the generated tcg code is not very optimal, for example a
> single vld4.8 instruction can generate over 250 tcg ops. I did some
> optimizations and got it under 200 but do you think it could be an
> issue that a single instruction can expand to so many tcg ops? I mean
> besides the fact that it causes TBs for only one or two guest
> instructions to be generated.

Fabrice wrote this (tcg/README):

  Don't hesitate to use helpers for complicated or seldom used target
  intructions. There is little performance advantage in using TCG to
  implement target instructions taking more than about twenty TCG
  instructions.

How applicable is it, I can't say.  It'd probably be a good thing
to benchmark the two versions, TCG vs helper.

> Perhaps this would also be a good place and time to also ask whether
> you would be interested in integrating support for OMAP3 in the QEMU
> mainline? We have been developing and using it for several months now,
> it's based on the work done by Yajin <yajin@vm-kernel.org> to support
> the OMAP3 BeagleBoard hardware. We have added support for the Nokia
> N900 system emulation as well. In my personal opinion the OMAP3
> emulation is in functionality on par with the existing OMAP2
> emulation, if not even more complete.

I would certainly like to see that in mainline!


Laurent

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

[Aurelien Jarno]

On Mon, Oct 19, 2009 at 08:23:11AM +0200, Juha.Riihimaki@nokia.com wrote:
> >> Apart from the points you have raised about specific patches there
> >> were few more minor bugs in the series spotted by Juha Riihimäki
> >> <juha.riihimaki@nokia.com>. A fix is available at
> >> http://repo.or.cz/w/qemu/navara.git?a=commit;h=2ce696baa6fc5d99522cf387b6a4913807fd43ed
> >
> > One of the fix was already in my branch (catched by Laurent  
> > Desnogues).
> > I have added the other fixes in my branch. The last to hunks are good
> > example why temp new/free should be done explicitly ;)
> 
> I think I have a couple of other fixes and patches on top of that as  
> well, but I'd rather wait until you get this bunch committed and then  
> format the patches against the new mainline so that they apply.

Thanks I have seen your patch, I'll have a closer look later today.

> On the subject of the new_tmp/dead_tmp, can you elaborate how critical  
> it is if there are resource leaks in the translator code, i.e. if some  
> of the temporary variables don't get marked dead? I suppose the  
> leakage would only affect the translation of the code block where it  
> appears? I found more leaks by introducing a new_tmp64/dead_tmp64 and  
> some other checkups to catch programming errors.

As long as they are not to many leaks by TB, it should not be a problem.
If there is a leak on a single instruction, and this instruction is used
a lot of times, the maximum number of temps (512) can be reached, which
causes qemu to stop.

> Some of the generated tcg code is not very optimal, for example a  
> single vld4.8 instruction can generate over 250 tcg ops. I did some  
> optimizations and got it under 200 but do you think it could be an  
> issue that a single instruction can expand to so many tcg ops? I mean  
> besides the fact that it causes TBs for only one or two guest  
> instructions to be generated.

According to Fabrice, an helper starts to be faster starting to 20 ops.
I think however it depends a lot on the host architecture, and therefore
it's difficult to give a limit. 200 looks high though.

> Perhaps this would also be a good place and time to also ask whether  
> you would be interested in integrating support for OMAP3 in the QEMU  
> mainline? We have been developing and using it for several months now,  
> it's based on the work done by Yajin <yajin@vm-kernel.org> to support  
> the OMAP3 BeagleBoard hardware. We have added support for the Nokia  
> N900 system emulation as well. In my personal opinion the OMAP3  
> emulation is in functionality on par with the existing OMAP2  
> emulation, if not even more complete.

That would be very nice to have such an emulated board in mainstream
QEMU. I would be happy to review your patches.

Cheers,
Aurelien

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

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

[Juha.Riihimaki]

On Oct 19, 2009, at 16:23, ext Aurelien Jarno wrote:

>> I think I have a couple of other fixes and patches on top of that as
>> well, but I'd rather wait until you get this bunch committed and then
>> format the patches against the new mainline so that they apply.
>
> Thanks I have seen your patch, I'll have a closer look later today.

Ok, that was "just" the resource leak patch. I have some others on top  
of that, I'll post them as soon as I get them (re)formatted.

>> Perhaps this would also be a good place and time to also ask whether
>> you would be interested in integrating support for OMAP3 in the QEMU
>> mainline? We have been developing and using it for several months  
>> now,
>> it's based on the work done by Yajin <yajin@vm-kernel.org> to support
>> the OMAP3 BeagleBoard hardware. We have added support for the Nokia
>> N900 system emulation as well. In my personal opinion the OMAP3
>> emulation is in functionality on par with the existing OMAP2
>> emulation, if not even more complete.
>
> That would be very nice to have such an emulated board in mainstream
> QEMU. I would be happy to review your patches.

Alright I'll look into generating a patch set for that as well but it  
might take a bit longer time to do it since it is a fairly large chunk  
of code with several modifications to the existing OMAP1/2 (and some  
others) code as well to accommodate for the OMAP3 features. The OMAP3  
support also calls for some further changes in the ARM core emulation  
to make the guest Linux kernel happy.


Regards,
Juha

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

[Paul Brook]

> > Some of the generated tcg code is not very optimal, for example a
> > single vld4.8 instruction can generate over 250 tcg ops. I did some
> > optimizations and got it under 200 but do you think it could be an
> > issue that a single instruction can expand to so many tcg ops? I mean
> > besides the fact that it causes TBs for only one or two guest
> > instructions to be generated.
> 
> Fabrice wrote this (tcg/README):
> 
>   Don't hesitate to use helpers for complicated or seldom used target
>   intructions. There is little performance advantage in using TCG to
>   implement target instructions taking more than about twenty TCG
>   instructions.
> 
> How applicable is it, I can't say.  It'd probably be a good thing
> to benchmark the two versions, TCG vs helper.

The problem is that you can not do memory accesses from within a helper 
function.

Paul

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

[Paul Brook]

> 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);
> |    }

There is actually logic behind this
Consider the obvious implementation of the "neg" instruction:

val = load_reg(rn);
tcg_gen_neg_i32(val, val);
store_reg(rd, val);

With the current code this is safe. However if load_reg returns cpu_R[n] 
instead of a temporary then the above code will incorrectly clobber the source 
register.

My theory was that tracking down an accidental write to a source register is 
much harder than tracking down a mismatched temporary.

Paul

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

[Aurelien Jarno]

On Tue, Nov 10, 2009 at 11:38:49PM +0000, Paul Brook wrote:
> > > Some of the generated tcg code is not very optimal, for example a
> > > single vld4.8 instruction can generate over 250 tcg ops. I did some
> > > optimizations and got it under 200 but do you think it could be an
> > > issue that a single instruction can expand to so many tcg ops? I mean
> > > besides the fact that it causes TBs for only one or two guest
> > > instructions to be generated.
> > 
> > Fabrice wrote this (tcg/README):
> > 
> >   Don't hesitate to use helpers for complicated or seldom used target
> >   intructions. There is little performance advantage in using TCG to
> >   implement target instructions taking more than about twenty TCG
> >   instructions.
> > 
> > How applicable is it, I can't say.  It'd probably be a good thing
> > to benchmark the two versions, TCG vs helper.
> 
> The problem is that you can not do memory accesses from within a helper 
> function.
> 

It is something possible, it's done for example for unaligned memory
access on MIPS (see for example helper_ldr).

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