From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22511 invoked by alias); 23 Oct 2012 21:39:47 -0000 Received: (qmail 22502 invoked by uid 22791); 23 Oct 2012 21:39:46 -0000 X-SWARE-Spam-Status: No, hits=-1.5 required=5.0 tests=AWL,BAYES_50,KHOP_THREADED X-Spam-Check-By: sourceware.org Received: from mel.act-europe.fr (HELO mel.act-europe.fr) (194.98.77.210) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 23 Oct 2012 21:39:40 +0000 Received: from localhost (localhost [127.0.0.1]) by filtered-smtp.eu.adacore.com (Postfix) with ESMTP id 36148290021; Tue, 23 Oct 2012 23:39:40 +0200 (CEST) Received: from mel.act-europe.fr ([127.0.0.1]) by localhost (smtp.eu.adacore.com [127.0.0.1]) (amavisd-new, port 10024) with ESMTP id kmenN7LS9eYK; Tue, 23 Oct 2012 23:39:40 +0200 (CEST) Received: from polaris.localnet (bon31-6-88-161-99-133.fbx.proxad.net [88.161.99.133]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (No client certificate requested) by mel.act-europe.fr (Postfix) with ESMTP id F22DE290020; Tue, 23 Oct 2012 23:39:39 +0200 (CEST) From: Eric Botcazou To: Richard Sandiford Cc: gcc-patches@gcc.gnu.org Subject: Re: Tidy store_bit_field_1 & co. Date: Tue, 23 Oct 2012 21:46:00 -0000 Message-ID: <1760664.7SOE4dSe93@polaris> User-Agent: KMail/4.7.2 (Linux/3.1.10-1.16-desktop; KDE/4.7.2; x86_64; ; ) In-Reply-To: <87r4opm550.fsf@talisman.home> References: <87obk4lu63.fsf@talisman.home> <8248446.zrYi3fceYI@polaris> <87r4opm550.fsf@talisman.home> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2012-10/txt/msg02103.txt.bz2 > I should probably have responded to this earlier, sorry. I'm not sure > which part you mean, so here's an attempt at justifying the whole block: > > 1) WORDS_BIG_ENDIAN is deliberately ignored: > > /* The following line once was done only if WORDS_BIG_ENDIAN, > but I think that is a mistake. WORDS_BIG_ENDIAN is > meaningful at a much higher level; when structures are copied > between memory and regs, the higher-numbered regs > always get higher addresses. */ > > 2) For MEM: > > The old code reached this "if" statement with: > > unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD; > offset = bitnum / unit; > bitpos = bitnum % unit; > > I.e.: > > offset = bitnum / BITS_PER_UNIT; > bitpos = bitnum % BITS_PER_UNIT; > > which the new code does explicitly with: > > unsigned HOST_WIDE_INT bitpos = bitnum; > if (MEM_P (xop0)) > { > /* Get a reference to the first byte of the field. */ > xop0 = adjust_bitfield_address (xop0, byte_mode, > bitpos / BITS_PER_UNIT); <-- offset > bitpos %= BITS_PER_UNIT; > } > > The following: > > unit = GET_MODE_BITSIZE (op_mode); > if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN) > xbitpos = unit - bitsize - xbitpos; > > code is the same as before. > > 3) For REG, !BYTES_BIG_ENDIAN, !BITS_BIG_ENDIAN: > > The easy case. The old code reached this "if" statement with: > > unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD; > offset = bitnum / unit; > bitpos = bitnum % unit; > ... > if (!MEM_P (op0) && ...) > { > ...set op0 to the word containing the field... > offset = 0; > } > > where the awkward thing is that OFFSET and BITPOS are now out of sync > with BITNUM. So before the "if" statement: > > offset = 0; > bitpos = bitnum % BITS_PER_WORD; > > which if the !MEM_P block above had updated BITNUM too would just > have been: > > offset = 0; > bitpos = bitnum; > > The new code does update BITNUM: > > if (!MEM_P (op0) && ...) > { > ...set op0 to the word containing the field... > bitnum %= BITS_PER_WORD; > } > ... > unsigned HOST_WIDE_INT bitpos = bitnum; > > so both the following hold: > > offset = 0; > bitpos = bitnum % BITS_PER_WORD; > > offset = 0; > bitpos = bitnum; > > The "if" statement doesn't do any endian modification to (X)BITPOS > before or after the patch. > > 4) For REG, !BYTES_BIG_ENDIAN, BITS_BIG_ENDIAN: > > Like (3), but at the end we also have: > > unit = GET_MODE_BITSIZE (op_mode); > if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN) > xbitpos = unit - bitsize - xbitpos; > > This is the same as before, although the assignment to UNIT has moved > up a bit. > > 5) For REG, BYTES_BIG_ENDIAN, BITS_BIG_ENDIAN: > > In this case the code leading up to the "if" statement was: > > unsigned int unit = (MEM_P (str_rtx)) ? BITS_PER_UNIT : BITS_PER_WORD; > offset = bitnum / unit; > bitpos = bitnum % unit; > ... > /* If OP0 is a register, BITPOS must count within a word. > But as we have it, it counts within whatever size OP0 now has. > On a bigendian machine, these are not the same, so convert. */ > if (BYTES_BIG_ENDIAN > && !MEM_P (op0) > && unit > GET_MODE_BITSIZE (GET_MODE (op0))) > bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0)); > ... > if (!MEM_P (op0) && ...) > { > ...set op0 to the word containing the field... > offset = 0; > } > > So: > > offset = 0 > bitpos = bitnum % BITS_PER_WORD; > if (...original op0 was smaller than a word...) > bitpos += BITS_PER_WORD - ; > > The !MEM_P block narrows OP0s that are wider than a word to word_mode -- > it never narrows more than that -- so this is equivalent to: > > offset = 0 > bitpos = bitnum % BITS_PER_WORD; > if (...current op0 is smaller than a word...) > bitpos += BITS_PER_WORD - ; > > And thanks to the !MEM_P code, op0 is never _wider_ than a word at > this point, so we have: > > offset = 0 > bitpos = bitnum % BITS_PER_WORD + BITS_PER_WORD - ; > > Then, inside the "if" statement we did: > > /* We have been counting XBITPOS within UNIT. > Count instead within the size of the register. */ > if (BYTES_BIG_ENDIAN && !MEM_P (xop0)) > xbitpos += GET_MODE_BITSIZE (op_mode) - unit; > > where UNIT is still BITS_PER_WORD. So we have: > > offset = 0 > bitpos = (bitnum % BITS_PER_WORD + BITS_PER_WORD - > + - BITS_PER_WORD); > > which cancels to: > > offset = 0 > bitpos = bitnum % BITS_PER_WORD + ( - ); > > As above, if the subreg code had updated BITNUM too, this would be > equivalent to: > > offset = 0 > bitpos = bitnum + ( - ); > > but isn't as things stand. The new code does update BITNUM: > > if (!MEM_P (op0) && ...) > { > ...set op0 to the word containing the field... > bitnum %= BITS_PER_WORD; > } > > and then calculates the final BITPOS directly: > > unsigned HOST_WIDE_INT bitpos = bitnum; > unsigned int unit = GET_MODE_BITSIZE (op_mode); > if (MEM_P (xop0)) > ... > else > { > /* Convert from counting within OP0 to counting in OP_MODE. */ > if (BYTES_BIG_ENDIAN) > bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0)); > } > > so both the following hold: > > offset = 0 > bitpos = bitnum % BITS_PER_WORD + ( - ); > > offset = 0 > bitpos = bitnum + ( - ); > > 6) For REG, BYTES_BIG_ENDIAN, !BITS_BIG_ENDIAN: > > Like (5) but we also have: > > unit = GET_MODE_BITSIZE (op_mode); > if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN) > xbitpos = unit - bitsize - xbitpos; > > This is the same as before, except that the UNIT assignment became > part of (5). > > Hope I've got that right... > > Of course, the idea is that the new code should make sense from first > principles. This was just trying to show that it isn't supposed to have > changed the behaviour. Thanks for the explanation (or rather the proof :-). No objections by me. -- Eric Botcazou