Re: [Qemu-devel] [PATCH 11/13] tcg: sync globals for pure helpers instead of saving them

[Aurelien Jarno <aurelien@aurel32.net>]

On Thu, Sep 27, 2012 at 12:39:10PM -0700, Richard Henderson wrote:
> On 09/27/2012 10:15 AM, Aurelien Jarno wrote:
> > Note: this is a change of behavior, but the new one is still compliant
> > with the documentation. Currently only cris, i386 and s390x have PURE
> > only helpers. I have checked they are really PURE, and also did some 
> > tests on cris and i386.
> > 
> > diff --git a/tcg/README b/tcg/README
> > index 27846f1..d61daa1 100644
> > --- a/tcg/README
> > +++ b/tcg/README
> > @@ -77,11 +77,16 @@ destroyed, but local temporaries and globals are preserved.
> >  Using the tcg_gen_helper_x_y it is possible to call any function
> >  taking i32, i64 or pointer types. By default, before calling a helper,
> >  all globals are stored at their canonical location and it is assumed
> > -that the function can modify them. This can be overridden by the
> > -TCG_CALL_CONST function modifier. By default, the helper is allowed to
> > -modify the CPU state or raise an exception. This can be overridden by
> > -the TCG_CALL_PURE function modifier, in which case the call to the
> > -function is removed if the return value is not used.
> > +that the function can modify them. The helper is allowed to modify
> > +the CPU state or raise an exception.
> > +
> > +This can be overridden by the TCG_CALL_CONST and TCG_CALL_PURE function
> > +modifiers. A PURE helper can read globals but cannot modify them nor the
> > +CPU state and cannot raise an exception. It can be removed if the return
> > +value is not used. For that the globals are synchronized with their canonical
> > +location, but the associated registers are not freed nor reloaded afterwise.
> > +A CONST helper is a PURE helper which in addition cannot read globals, they
> > +are not synchronized nor stored. Note that CONST implies PURE.
> 
> If we're going to re-org these flags, lets please do it right.

Well it was not a re-org, but more an improvement of the documentation.

> In particular the terms "const" and "pure" were clearly stolen from gcc,
> but then given different meanings (!).  This, to me at least, is incredibly
> confusing.  I have to go back and re-read the docs every time I touch
> one of the helper declarations.
> 
> There are really three flags we care about:
> 
> (1) Helper may read globals.  Sometimes indirectly via exception.
>     Implies that globals must be synced, but may remain in REG/CONST state.

I actually do wonder if we shouldn't use a different flag for "may
trigger exception", even if we end-up handling it the same way as "may
read globals". I am not sure it would be clear without knowledge of the
QEMU internals that a function triggering exceptions should be flagged
as "read globals".

> (2) Helper may write globals.  Implies globals must be synced, and
>     returned to MEM state.
> 
> (3) Helper has no side effects at all.  Implies that it can be deleted if
>     its outputs are dead.

This is basically the flags I have used for the TCG op. That said the
problem with this way of doing for helpers is that most helpers will now
need flags, while it was more or less the contrary up to now.

It's a bit like in GCC, a default function implies nothing, but you can
add more flags latter.

> For the sake of discussion, lets call these READG, WRITEG, NOSIDE.
> 
> Our current default is READG | WRITEG.
> 
> Our current definition of PURE is READG | NOSIDE.  Note that the gcc
> definition of pure actually talks about main memory.
> 
> Our current definition of CONST is none of these bits.  Note that the
> gcc definition of const is a superset of pure.
> 
> What we'd like for all fp helpers is READG only.  Something that we
> cannot get at the moment.

Agreed, as long as the fp env is not mapped as a global.

> There are several cases in which the helper needs to return more than
> 64 bits in which we use a "temp" allocated in the env struct.  Sparc
> does this for some of its 128-bit arithmetic; I'm planning to do just
> the same in my s390x rewrite.  For this, the helper neither reads nor
> writes globals, but it has non-global-register side effects.  There
> will generally be a post-helper load from env to get the "extra" bits.
> While the "true" output of the helper may be dead, the side load from
> env may not be.  So we want none of READG | WRITEG | NOSIDE.
> 
> If we do reorg, we can certainly add compatibility defines so that we
> need not update all translators at once.
> 

Given the default is READG | WRITEG, it would be difficult to do that
unless we negate the flags. Actually NOWRITEG, NOREADG, NOSIDE might be
a good way to go, and is more consistent that by default nothing should
be implied.

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