qemu-devel.nongnu.org archive mirror
 help / color / mirror / Atom feed
From: Aurelien Jarno <aurelien@aurel32.net>
To: Blue Swirl <blauwirbel@gmail.com>
Cc: qemu-devel <qemu-devel@nongnu.org>
Subject: Re: [Qemu-devel] [PATCH RFC 00/11] AREG0 elimination
Date: Sun, 15 May 2011 01:31:47 +0200	[thread overview]
Message-ID: <20110514233147.GC30615@hall.aurel32.net> (raw)
In-Reply-To: <20110514211616.GA13600@volta.aurel32.net>

On Sat, May 14, 2011 at 11:16:16PM +0200, Aurelien Jarno wrote:
> On Sat, May 14, 2011 at 10:35:20PM +0300, Blue Swirl wrote:
> > Here's a RFC series for eliminating AREG0.
> > 
> > Blue Swirl (11):
> >   Move user emulator stuff from cpu-exec.c to user-exec.c
> >   Delete unused tb_invalidate_page_range
> > 
> > The above should be OK to commit.
> > 
> >   cpu_loop_exit: avoid using AREG0
> >   Delegate setup of TCG temporaries to targets
> > 
> > These two are not, unless the overall plan is OK.
> > 
> >   TCG: fix negative frame offset calculations
> >   TCG/x86: use stack for TCG temps
> >   TCG/Sparc64: use stack for TCG temps
> > 
> > But these three should be OK. I've tested lightly x86_64 and Sparc64 hosts.
> > 
> >   Add CONFIG_TARGET_NEEDS_AREG0
> >   Don't compile legacy qemu_ld/st functions if target doesn't need them
> > 
> > Should be OK, though the latter patch only touches x86.
> > 
> >   Add new qemu_ld and qemu_st functions
> >   sparc: use new qemu_ld and qemu_st functions
> > 
> > The last two compile but QEMU segfaults. I just made a naive
> > conversion for getting comments.
> > 
> 
> What is the goal behing removing TCG_AREG0? If it is speed improvement,
> can you please provide some benchmarks?
> 
> The env register is used very often (basically for every load/store, but
> also a lot of helpers), so it makes sense to reserve a register for it.
> 
> For what I understand from your patch series, you prefer to pass this
> register explicitly to TCG functions. This basically means this TCG
> global will be loaded to host register as soon as it is used, but also
> regularly, as globals are saved back to their canonical location before
> an helper or a load/store.
> 
> So it seems that this patch series will just allowing the "env register"
> to change over time, though it will not spare one more register for the 
> TCG code, and it will emit longer TCG code to regularly reload the env
> global into a host register.
> 

One way to solve that would be to use a env register only at the TCG
level, but not at the GCC level. That means loading the value of env
into TCG_AREG0 in the prologue.

That way, all the TCG code will have direct access to env, and the GCC
generated code (which includes the helper) don't need to have this
register reserved. I doubt the latter will increase the performance, but
I understand the cleanliness argument.

This also means minimal code changes, especially as most (if not all) 
TCG targets seems to have TCG_AREG0 as a callee saved arguments.
Basically the path to do the changes could be:
- Make sure all targets have TCG_AREG0 as a callee saved register
- Load env into TCG_AREG0 (and rename it as a TCG_ENV?) in the prologue 
  (could be by passing it as argument to the prologue code)
- Change all helpers to not use env directly
- Change softmmu code to not use env directly
- Remove GCC hack to reserve a register for env.

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

      parent reply	other threads:[~2011-05-14 23:31 UTC|newest]

Thread overview: 31+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-05-14 19:35 [Qemu-devel] [PATCH RFC 00/11] AREG0 elimination Blue Swirl
2011-05-14 21:16 ` Aurelien Jarno
2011-05-14 21:52   ` Blue Swirl
2011-05-14 22:04     ` Aurelien Jarno
2011-05-15  7:15       ` Blue Swirl
2011-05-15  9:24         ` Aurelien Jarno
2011-05-15  9:58           ` Blue Swirl
2011-05-15 10:19             ` Aurelien Jarno
2011-05-15 11:12               ` Blue Swirl
2011-05-15 11:36                 ` Aurelien Jarno
2011-05-15 12:30                   ` Blue Swirl
2011-05-15 12:49                     ` Aurelien Jarno
2011-05-15 13:16                       ` Blue Swirl
2011-05-15 13:43                         ` Aurelien Jarno
2011-05-15 14:02                           ` Blue Swirl
2011-05-15 14:06                             ` Aurelien Jarno
2011-05-15 14:10                               ` Blue Swirl
2011-05-15  9:27         ` Laurent Desnogues
2011-05-15  9:49           ` Aurelien Jarno
2011-05-15 11:03           ` Blue Swirl
2011-05-15 11:14             ` Aurelien Jarno
2011-05-15 11:33               ` Blue Swirl
2011-05-15 11:37                 ` Aurelien Jarno
2011-05-15 12:33                   ` Blue Swirl
2011-05-15 12:14                 ` Laurent Desnogues
2011-05-15 12:37                   ` Blue Swirl
2011-05-15 13:02                     ` Aurelien Jarno
2011-05-15 13:42                       ` Blue Swirl
2011-05-15 14:03                         ` Aurelien Jarno
2011-05-15 14:06                           ` Blue Swirl
2011-05-14 23:31   ` Aurelien Jarno [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20110514233147.GC30615@hall.aurel32.net \
    --to=aurelien@aurel32.net \
    --cc=blauwirbel@gmail.com \
    --cc=qemu-devel@nongnu.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).