Re: [Qemu-devel] [PATCH 25/26] target/s390x: implement TRANSLATE ONE/TWO TO ONE/TWO

[Aurelien Jarno]

On 2017-05-30 09:45, Richard Henderson wrote:
> On 05/29/2017 04:17 AM, Aurelien Jarno wrote:
> > On 2017-05-26 10:10, Richard Henderson wrote:
> > > On 05/25/2017 02:05 PM, Aurelien Jarno wrote:
> > > > +uint32_t HELPER(trXX)(CPUS390XState *env, uint32_t r1, uint32_t r2,
> > > > +                      uint32_t sizes)
> > > > +{
> > > > +    uintptr_t ra = GETPC();
> > > > +    int dsize = (sizes & 1) ? 1 : 2;
> > > > +    int ssize = (sizes & 2) ? 1 : 2;
> > > > +    uint16_t tst = env->regs[0] & ((1 << (8 * dsize)) - 1);
> > > 
> > > I think you should pass in tst as an argument.  That way you can pass in 
> > > an
> > > out-of-band value when we implement ETF2 and test field M3 bit 3.
> > 
> > I don't mind passing r0 as an argument. That said if we want to pass tst
> > or bundle the M3 field, it means we need to use TCG instructions to do
> > so. I am not sure it brings a lot compare to doing so in the helper
> > side.
> 
> Not at all -- the M3 bit test would be a translation-time check.

I still don't really see the point. On the TCG side it means we need
something like that:

    if (m3 & 1) {
       tcg_gen_movi_tl(r0, -1);
    } else if (dsize == 1) {
       tcg_gen_ext8u(r0, regs[0]);
    } else if (dsize == 2) 
       tcg_gen_ext16u(r0, regs[0]);
    }

On the helper side we then need to check if the value passed equals -1
or not to get m3 instead of directly accessing the value in register 0.
If we want to bundle m3 with something else, it might be better to bundle
it with the "sizes" argument.

If what worries you is the cast of tst to uint8_t / uint16_t as function
of dsize in the helper, we can rename trXX into an inline function
do_trXX and use one helper per instruction instead of a common helper
for the 3 instructions.

-- 
Aurelien Jarno                          GPG: 4096R/1DDD8C9B
address@hidden                 http://www.aurel32.net