From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from eggs.gnu.org ([2001:4830:134:3::10]:36026) by lists.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XX42S-0004XS-5c for qemu-devel@nongnu.org; Thu, 25 Sep 2014 04:04:38 -0400 Received: from Debian-exim by eggs.gnu.org with spam-scanned (Exim 4.71) (envelope-from ) id 1XX42I-0003Ak-7X for qemu-devel@nongnu.org; Thu, 25 Sep 2014 04:04:32 -0400 Received: from lhrrgout.huawei.com ([194.213.3.17]:60179) by eggs.gnu.org with esmtp (Exim 4.71) (envelope-from ) id 1XX42H-0003AB-Uq for qemu-devel@nongnu.org; Thu, 25 Sep 2014 04:04:22 -0400 Message-ID: <5423CC6D.6030904@huawei.com> Date: Thu, 25 Sep 2014 10:03:57 +0200 From: Claudio Fontana MIME-Version: 1.0 References: <1411419461-24390-1-git-send-email-rth@twiddle.net> <1411419461-24390-10-git-send-email-rth@twiddle.net> <54227EDD.7060205@huawei.com> <5422E106.2020208@twiddle.net> In-Reply-To: <5422E106.2020208@twiddle.net> Content-Type: text/plain; charset="windows-1252" Content-Transfer-Encoding: 7bit Subject: Re: [Qemu-devel] [PULL 09/11] tcg-aarch64: Use 32-bit loads for qemu_ld_i32 List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Richard Henderson , qemu-devel@nongnu.org Cc: peter.maydell@linaro.org On 24.09.2014 17:19, Richard Henderson wrote: > On 09/24/2014 01:20 AM, Claudio Fontana wrote: >>> @@ -1118,7 +1119,8 @@ static void tcg_out_qemu_ld_direct(TCGContext *s, TCGMemOp memop, >>> tcg_out_ldst_r(s, I3312_LDRB, data_r, addr_r, off_r); >>> break; >>> case MO_SB: >>> - tcg_out_ldst_r(s, I3312_LDRSBX, data_r, addr_r, off_r); >>> + tcg_out_ldst_r(s, type ? I3312_LDRSBX : I3312_LDRSBW, >>> + data_r, addr_r, off_r); >> >> >> since we are using the enum type TCGType, why do we check type as "type ?" >> >> I would have expected the conditional to be something like >> >> type == TCG_TYPE_I32 ? I3312_LDRSBW : I3312_LDRSBX >> >> It's pretty obvious what is happening but it might spare someone a lookup into the header file >> to test that type 0 is indeed TCG_TYPE_I32. > > We assert the boolean-ish nature of TCGType at the start of the file, and use > it for the "ext" variable throughout. Would it help if the variable weren't > named "type"? > > > r~ I could not find a comment describing that in tcg.h, did I miss it in the patch? I find it difficult to come up with a better name for TCGType, I wonder what that could be.. but what about not caring about the boolish nature of TCGType and instead check it explicitly? I understand that there are multiple enumeration constants with the same value, but if the names of the first two enumeration constants are general enough, that should be understandable, no? A comment before TCGType could explain this, and then we could have values like TCG_TYPE_32 = 0, TCG_TYPE_64, /* .. all the other aliases */ Which is basically what we have now isn't it? TCG_TYPE_I32 and TCG_TYPE_I64 are general enough I think, so why aren't we checking those constants explicitly? Ciao, Claudio