From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 21048 invoked by alias); 23 Oct 2012 18:10:14 -0000 Received: (qmail 21000 invoked by uid 22791); 23 Oct 2012 18:10:12 -0000 X-SWARE-Spam-Status: No, hits=-3.8 required=5.0 tests=AWL,BAYES_00,DKIM_ADSP_CUSTOM_MED,DKIM_SIGNED,DKIM_VALID,FREEMAIL_FROM,KHOP_RCVD_TRUST,NML_ADSP_CUSTOM_MED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-wi0-f179.google.com (HELO mail-wi0-f179.google.com) (209.85.212.179) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 23 Oct 2012 18:10:04 +0000 Received: by mail-wi0-f179.google.com with SMTP id hq7so3154876wib.8 for ; Tue, 23 Oct 2012 11:10:03 -0700 (PDT) Received: by 10.180.79.202 with SMTP id l10mr30991777wix.9.1351015803036; Tue, 23 Oct 2012 11:10:03 -0700 (PDT) Received: from localhost ([2.26.192.222]) by mx.google.com with ESMTPS id b7sm57475027wiz.3.2012.10.23.11.10.01 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 23 Oct 2012 11:10:01 -0700 (PDT) From: Richard Sandiford To: Eric Botcazou Mail-Followup-To: Eric Botcazou ,gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Cc: gcc-patches@gcc.gnu.org Subject: Re: Tidy store_bit_field_1 & co. References: <87obk4lu63.fsf@talisman.home> <8248446.zrYi3fceYI@polaris> Date: Tue, 23 Oct 2012 18:13:00 -0000 In-Reply-To: <8248446.zrYi3fceYI@polaris> (Eric Botcazou's message of "Wed, 17 Oct 2012 22:27:58 +0200") Message-ID: <87r4opm550.fsf@talisman.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.3 (gnu/linux) MIME-Version: 1.0 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/msg02073.txt.bz2 Eric Botcazou writes: >> + enum machine_mode op_mode = mode_for_extraction (EP_insv, 3); >> if (HAVE_insv >> && GET_MODE (value) != BLKmode >> && bitsize > 0 >> @@ -690,25 +670,34 @@ store_bit_field_1 (rtx str_rtx, unsigned >> -fstrict-volatile-bitfields is in effect. */ >> && !(MEM_P (op0) && MEM_VOLATILE_P (op0) >> && flag_strict_volatile_bitfields > 0) >> - && ! ((REG_P (op0) || GET_CODE (op0) == SUBREG) >> - && (bitsize + bitpos > GET_MODE_BITSIZE (op_mode))) >> /* Do not use insv if the bit region is restricted and >> op_mode integer at offset doesn't fit into the >> restricted region. */ >> && !(MEM_P (op0) && bitregion_end >> - && bitnum - bitpos + GET_MODE_BITSIZE (op_mode) >> + && bitnum - (bitnum % BITS_PER_UNIT) + GET_MODE_BITSIZE (op_mode) >> >> > bitregion_end + 1)) >> >> { >> struct expand_operand ops[4]; >> - int xbitpos = bitpos; >> + unsigned HOST_WIDE_INT bitpos = bitnum; >> rtx value1; >> rtx xop0 = op0; >> rtx last = get_last_insn (); >> bool copy_back = false; >> >> - /* Add OFFSET into OP0's address. */ >> + unsigned int unit = GET_MODE_BITSIZE (op_mode); >> if (MEM_P (xop0)) >> - xop0 = adjust_bitfield_address (xop0, byte_mode, offset); >> + { >> + /* Get a reference to the first byte of the field. */ >> + xop0 = adjust_bitfield_address (xop0, byte_mode, >> + bitpos / BITS_PER_UNIT); >> + bitpos %= BITS_PER_UNIT; >> + } >> + else >> + { >> + /* Convert from counting within OP0 to counting in OP_MODE. */ >> + if (BYTES_BIG_ENDIAN) >> + bitpos += unit - GET_MODE_BITSIZE (GET_MODE (op0)); >> + } >> >> /* If xop0 is a register, we need it in OP_MODE >> to make it acceptable to the format of insv. */ >> @@ -735,20 +724,13 @@ store_bit_field_1 (rtx str_rtx, unsigned >> copy_back = true; >> } >> >> - /* 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; >> - >> - unit = GET_MODE_BITSIZE (op_mode); >> - >> /* If BITS_BIG_ENDIAN is zero on a BYTES_BIG_ENDIAN machine, we count >> "backwards" from the size of the unit we are inserting into. Otherwise, we >> count bits from the most significant on a >> BYTES/BITS_BIG_ENDIAN machine. */ >> >> if (BITS_BIG_ENDIAN != BYTES_BIG_ENDIAN) >> - xbitpos = unit - bitsize - xbitpos; >> + bitpos = unit - bitsize - bitpos; >> >> /* Convert VALUE to op_mode (which insv insn wants) in VALUE1. */ >> value1 = value; > > I guess I see the reasoning, but I cannot say whether it's right or wrong... 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. Richard