From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from [140.186.70.92] (port=54619 helo=eggs.gnu.org) by lists.gnu.org with esmtp (Exim 4.43) id 1Q4V3i-0002xl-R5 for qemu-devel@nongnu.org; Tue, 29 Mar 2011 05:17:56 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1Q4V3h-00068v-II for qemu-devel@nongnu.org; Tue, 29 Mar 2011 05:17:54 -0400 Received: from mail-vw0-f45.google.com ([209.85.212.45]:54614) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1Q4V3h-00068j-DH for qemu-devel@nongnu.org; Tue, 29 Mar 2011 05:17:53 -0400 Received: by vws17 with SMTP id 17so3389821vws.4 for ; Tue, 29 Mar 2011 02:17:52 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: References: <1300982333-12802-1-git-send-email-agraf@suse.de> <1300982333-12802-17-git-send-email-agraf@suse.de> Date: Tue, 29 Mar 2011 10:17:52 +0100 Message-ID: Subject: Re: [Qemu-devel] [PATCH 16/17] s390x: translate engine for s390x CPU From: Peter Maydell Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable List-Id: qemu-devel.nongnu.org List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Alexander Graf Cc: QEMU-devel Developers , Aurelien Jarno , Richard Henderson On 29 March 2011 09:55, Alexander Graf wrote: > > On 28.03.2011, at 17:40, Peter Maydell wrote: > >> On 24 March 2011 15:58, Alexander Graf wrote: >>> diff --git a/target-s390x/translate.c b/target-s390x/translate.c >>> + =C2=A0 =C2=A0case 0x4: =C2=A0/* LMG =C2=A0 =C2=A0 =C2=A0R1,R3,D2(B2) = =C2=A0 =C2=A0 [RSE] */ >>> + =C2=A0 =C2=A0case 0x24: /* STMG =C2=A0 =C2=A0 R1,R3,D2(B2) =C2=A0 =C2= =A0 [RSE] */ >>> + =C2=A0 =C2=A0case 0x26: /* STMH =C2=A0 =C2=A0 R1,R3,D2(B2) =C2=A0 =C2= =A0 [RSE] */ >>> + =C2=A0 =C2=A0case 0x96: /* LMH =C2=A0 =C2=A0 =C2=A0R1,R3,D2(B2) =C2= =A0 =C2=A0 [RSE] */ >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0/* Apparently, unrolling lmg/stmg of any s= ize gains performance - >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 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. >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0tmp2 =3D tcg_const_i64((((ui= nt64_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... >>> + =C2=A0 =C2=A0case 0xa: /* SVC =C2=A0 =C2=A0I =C2=A0 =C2=A0 =C2=A0 =C2= =A0 [RR] */ >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0insn =3D ld_code2(s->pc); >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0debug_insn(insn); >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0i =3D insn & 0xff; >>> +#ifdef CONFIG_USER_ONLY >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0s->pc +=3D 2; >>> +#endif >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0update_psw_addr(s); >>> + =C2=A0 =C2=A0 =C2=A0 =C2=A0gen_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