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

[Alexander Graf]

On 29.03.2011, at 11:17, Peter Maydell wrote:

> On 29 March 2011 09:55, Alexander Graf <address@hidden> wrote:
>> 
>> On 28.03.2011, at 17:40, Peter Maydell wrote:
>> 
>>> On 24 March 2011 15:58, Alexander Graf <address@hidden> 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.

Hrm - might be useful to increase MAX_OP_PER_INSTR then, no?

> 
>>>> +            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 :-)

Heh, I have vi configured to color in lines >80 chars as well, so I usually 
only keep them there 

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

*shrug* so far CODING_STYLE has only brought badness to qemu. The new style is 
less readable than the common dominator that was there before (Fabrice's coding 
style) and resulted in man-years of wasted time on rejected patches for the 
sake of braces. I'd rather want to remove the file than patching it (which 
again would create a mail thread of 300 mails, waste 5 man-years of 
productivity and bring us no gain in the end).

> 
>>>> +    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.

Unfortunately, it's not that easy as there are 2 different ways of issuing an 
SVC (actual SVC and EXECUTE instruction), both of which having different 
instruction lengths. So we really need to keep the information in the 
instruction decoder :(


Alex