[Qemu-devel] dyngen-exec.h cleanup

[Qemu-devel] dyngen-exec.h cleanup

[Blue Swirl]

[-- Attachment #1: Type: text/plain, Size: 155 bytes --]

Hi,

This patch cleans up dyngen-exec.h a bit. It works for me with Linux
and OpenBSD hosts, but I wonder if some other hosts will be broken.
Please test.

[-- Attachment #2: dyngen_exec_cleanup.diff --]
[-- Type: plain/text, Size: 2617 bytes --]

[Qemu-devel] Re: dyngen-exec.h cleanup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 649 bytes --]

Blue Swirl wrote:
> Hi,
> 
> This patch cleans up dyngen-exec.h a bit. It works for me with Linux
> and OpenBSD hosts, but I wonder if some other hosts will be broken.
> Please test.

Mmmh, I played with such removals as well but always felt highly
uncomfortable due to the hard-to-assess side effects of this old code.
Also, some discussion on this list suggested that it's more efficient to
look into converting the remaining AREGS to TCG and finally do the
ultimative "rm dyngen-exec.h". Don't you want to spend some time on this
already?

Jan

PS: Please try to post such patches inlined or at least without base64
encoding.


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: dyngen-exec.h cleanup

[Laurent Desnogues]

On Sun, Mar 8, 2009 at 3:41 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Also, some discussion on this list suggested that it's more efficient to
> look into converting the remaining AREGS to TCG and finally do the
> ultimative "rm dyngen-exec.h". Don't you want to spend some time on this
> already?

This requires modifying ARM translator which is the last one to use
AREGn with n>0. And don't all targets use AREG0 as a pointer to the
CPU state?


Laurent

[Qemu-devel] Re: dyngen-exec.h cleanup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 978 bytes --]

Laurent Desnogues wrote:
> On Sun, Mar 8, 2009 at 3:41 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Also, some discussion on this list suggested that it's more efficient to
>> look into converting the remaining AREGS to TCG and finally do the
>> ultimative "rm dyngen-exec.h". Don't you want to spend some time on this
>> already?
> 
> This requires modifying ARM translator which is the last one to use
> AREGn with n>0. And don't all targets use AREG0 as a pointer to the
> CPU state?

Yes, but wasn't it you who suggested that all those users should be
converted over to the tcg_global_reg API?

There is surely some work to do, and that probably across all archs. But
the sooner we should start. dyngen-exec.h is a constant source of pain
when you try to introduce new headers or refactor existing ones. /me was
so far lacking the time to finally attack this (and instead had to add
yet another hack with the recent monitor series, see disas.h).

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: dyngen-exec.h cleanup

[Laurent Desnogues]

On Sun, Mar 8, 2009 at 4:36 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Laurent Desnogues wrote:
>> On Sun, Mar 8, 2009 at 3:41 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>> Also, some discussion on this list suggested that it's more efficient to
>>> look into converting the remaining AREGS to TCG and finally do the
>>> ultimative "rm dyngen-exec.h". Don't you want to spend some time on this
>>> already?
>>
>> This requires modifying ARM translator which is the last one to use
>> AREGn with n>0. And don't all targets use AREG0 as a pointer to the
>> CPU state?
>
> Yes, but wasn't it you who suggested that all those users should be
> converted over to the tcg_global_reg API?

Yes, and I did the work for ARM.  However when considering the
removal of AREG0, and after looking at generated code, I came
to the perhaps premature conclusion that removing it would not
bring me any speedup (at least for a not so register starved
target as x86_64).

> There is surely some work to do, and that probably across all archs. But
> the sooner we should start. dyngen-exec.h is a constant source of pain
> when you try to introduce new headers or refactor existing ones.

Well dyngen-exec.h is long gone in my sources even though
AREG0 is still used.  I would have to backtrack my changes to
see how I arrived to that, but for sure the first thing to do is to
remove cpu_T from ARM target.


Laurent

[Qemu-devel] Re: dyngen-exec.h cleanup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1671 bytes --]

Laurent Desnogues wrote:
> On Sun, Mar 8, 2009 at 4:36 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Laurent Desnogues wrote:
>>> On Sun, Mar 8, 2009 at 3:41 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>> Also, some discussion on this list suggested that it's more efficient to
>>>> look into converting the remaining AREGS to TCG and finally do the
>>>> ultimative "rm dyngen-exec.h". Don't you want to spend some time on this
>>>> already?
>>> This requires modifying ARM translator which is the last one to use
>>> AREGn with n>0. And don't all targets use AREG0 as a pointer to the
>>> CPU state?
>> Yes, but wasn't it you who suggested that all those users should be
>> converted over to the tcg_global_reg API?
> 
> Yes, and I did the work for ARM.  However when considering the
> removal of AREG0, and after looking at generated code, I came
> to the perhaps premature conclusion that removing it would not
> bring me any speedup (at least for a not so register starved
> target as x86_64).

I don't think we are looking for speedup here, just for cleanup. Status
quo regarding performance after a conversion would be more than fine IMHO.

> 
>> There is surely some work to do, and that probably across all archs. But
>> the sooner we should start. dyngen-exec.h is a constant source of pain
>> when you try to introduce new headers or refactor existing ones.
> 
> Well dyngen-exec.h is long gone in my sources even though
> AREG0 is still used.  I would have to backtrack my changes to
> see how I arrived to that, but for sure the first thing to do is to
> remove cpu_T from ARM target.

Yes, please share your wisdom!

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] Re: dyngen-exec.h cleanup

[Laurent Desnogues]

On Sun, Mar 8, 2009 at 5:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
> Laurent Desnogues wrote:
[...]
>> Yes, and I did the work for ARM.  However when considering the
>> removal of AREG0, and after looking at generated code, I came
>> to the perhaps premature conclusion that removing it would not
>> bring me any speedup (at least for a not so register starved
>> target as x86_64).
>
> I don't think we are looking for speedup here, just for cleanup. Status
> quo regarding performance after a conversion would be more than fine IMHO.

I was ambiguous, I meant I expect slowdowns...  But given how many
times I was wrong in the past when I thought some tricks would
speedup things, I should probably not trust my a priori.

>> Well dyngen-exec.h is long gone in my sources even though
>> AREG0 is still used.  I would have to backtrack my changes to
>> see how I arrived to that, but for sure the first thing to do is to
>> remove cpu_T from ARM target.
>
> Yes, please share your wisdom!

Sorry if I sounded arrogant, I just wanted to highlight the fact one
can achieve your aim.  As far as sharing goes, that's another
story.


Laurent

[Qemu-devel] Re: dyngen-exec.h cleanup

[Jan Kiszka]

[-- Attachment #1: Type: text/plain, Size: 1448 bytes --]

Laurent Desnogues wrote:
> On Sun, Mar 8, 2009 at 5:00 PM, Jan Kiszka <jan.kiszka@web.de> wrote:
>> Laurent Desnogues wrote:
> [...]
>>> Yes, and I did the work for ARM.  However when considering the
>>> removal of AREG0, and after looking at generated code, I came
>>> to the perhaps premature conclusion that removing it would not
>>> bring me any speedup (at least for a not so register starved
>>> target as x86_64).
>> I don't think we are looking for speedup here, just for cleanup. Status
>> quo regarding performance after a conversion would be more than fine IMHO.
> 
> I was ambiguous, I meant I expect slowdowns...  But given how many
> times I was wrong in the past when I thought some tricks would
> speedup things, I should probably not trust my a priori.
> 
>>> Well dyngen-exec.h is long gone in my sources even though
>>> AREG0 is still used.  I would have to backtrack my changes to
>>> see how I arrived to that, but for sure the first thing to do is to
>>> remove cpu_T from ARM target.
>> Yes, please share your wisdom!
> 
> Sorry if I sounded arrogant, I just wanted to highlight the fact one
> can achieve your aim.  As far as sharing goes, that's another
> story.

No, no, you didn't sound arrogant. I just forgot your boundary conditions.

But it's good to know that there might be a way to overcome
dyngen-exec.h without dropping AREGs altogether in case performance
requires this.

Jan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 257 bytes --]

Re: [Qemu-devel] dyngen-exec.h cleanup

[Hasso Tepper]

Blue Swirl wrote:
> This patch cleans up dyngen-exec.h a bit. It works for me with Linux
> and OpenBSD hosts, but I wonder if some other hosts will be broken.
> Please test.

I had to modify target-arm/exec.h and target-m68k/exec.h to include 
sys/types.h on DragonFly to make it compile. Otherwise it complained 
about uint32_t.


regards,

-- 
Hasso Tepper

Re: [Qemu-devel] dyngen-exec.h cleanup

[Anthony Liguori]

Blue Swirl wrote:
> Hi,
>
> This patch cleans up dyngen-exec.h a bit. It works for me with Linux
> and OpenBSD hosts, but I wonder if some other hosts will be broken.
> Please test.
>   

The windows build still works.

Regards,

Anthony Liguori