Re: [Qemu-devel] [PATCH 16/17] s390x: translate engine for s390x CPU

[Peter Maydell <peter.maydell@linaro.org>]

On 29 March 2011 09:55, Alexander Graf <agraf@suse.de> wrote:
>
> On 28.03.2011, at 17:40, Peter Maydell wrote:
>
>> On 24 March 2011 15:58, Alexander Graf <agraf@suse.de> wrote:
>>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c
>>> +    case 0x4:  /* LMG      R1,R3,D2(B2)     [RSE] */
>>> +    case 0x24: /* STMG     R1,R3,D2(B2)     [RSE] */
>>> +    case 0x26: /* STMH     R1,R3,D2(B2)     [RSE] */
>>> +    case 0x96: /* LMH      R1,R3,D2(B2)     [RSE] */
>>> +        /* Apparently, unrolling lmg/stmg of any size gains performance -
>>> +           even for very long ones... */
>>
>> Doesn't this take you over MAX_OP_PER_INSTR for some cases?
>
> I haven't encountered any case where it does.

Really? MAX_OP_PER_INSTR's only 96, so if you have 16 registers
in your loop then it only needs 6 ops per register to hit that,
and the op 0x96 case looks like it must generate more than that.

I have an item on my todo list to see if I can add an assert()
check for this limit, because there are cases for Neon load/stores
that apparently hit it.

>>> +            tmp2 = tcg_const_i64((((uint64_t)i2) << 48) | 0x0000ffffffffffffULL);
>>
>> This line is over 80 chars, as are a handful of others in this file.
>
> Yeah, I generally see the 80 char limit as soft limit and make it
> hard on ~90. If a line is only over it by very little, readability
> doesn't improve by breaking it up. So far, everyone agreed to that
> approach :).

>80 chars reduces readability for me because I have emacs configured
to make long lines look very ugly so I don't write them :-)

Also, if we want the standard to be 'soft 80, hard 90' we should
say so in CODING_STYLE...

>>> +    case 0xa: /* SVC    I         [RR] */
>>> +        insn = ld_code2(s->pc);
>>> +        debug_insn(insn);
>>> +        i = insn & 0xff;
>>> +#ifdef CONFIG_USER_ONLY
>>> +        s->pc += 2;
>>> +#endif
>>> +        update_psw_addr(s);
>>> +        gen_op_calc_cc(s);
>>
>> Why do we only need to update s->pc if CONFIG_USER_ONLY?
>> Not saying it's wrong, but it could use an explanatory comment...
>
> The user code needs to know where it jumps back to, while the
> exception generation code needs to get the exact position it was
> in to generate some more metadata.

Ah. For ARM we do this by advancing env->regs[15] in linux-user/main.c
cpu_loop() when we get an EXCP_SWI. It looks like we do it that way
for MIPS and SPARC at least too, so I guess it would be better for
s390 to follow that pattern.

-- PMM