From patchwork Sun Nov 18 17:35:56 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 199904 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) by ozlabs.org (Postfix) with SMTP id E40502C0091 for ; Mon, 19 Nov 2012 04:36:11 +1100 (EST) Comment: DKIM? See http://www.dkim.org DKIM-Signature: v=1; a=rsa-sha1; c=relaxed/relaxed; d=gcc.gnu.org; s=default; x=1353864972; h=Comment: DomainKey-Signature:Received:Received:Received:Received:Received: Received:From:To:Mail-Followup-To:Cc:Subject:References:Date: In-Reply-To:Message-ID:User-Agent:MIME-Version:Content-Type: Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive: List-Post:List-Help:Sender:Delivered-To; bh=wEtCyTMRQ+WN5IzTnAqe diydIyM=; b=g0CLkq1ZQD4OKjLRPXhxjWFz6CqUdAPZUtxyOzFZf6veN56v0KPM xNlvmzVvRhEBShzZ478xl2jtKz3KfZu5N9Mf5A4XfhCXi8DdZq9JC/NnYKo8CNLj FEKSvwfzw8Diyn7+zBLWt2pMwje8XOeqkRPceYMp1t/lW5fJDulsuFk= Comment: DomainKeys? See http://antispam.yahoo.com/domainkeys DomainKey-Signature: a=rsa-sha1; q=dns; c=nofws; s=default; d=gcc.gnu.org; h=Received:Received:X-SWARE-Spam-Status:X-Spam-Check-By:Received:Received:Received:Received:From:To:Mail-Followup-To:Cc:Subject:References:Date:In-Reply-To:Message-ID:User-Agent:MIME-Version:Content-Type:Mailing-List:Precedence:List-Id:List-Unsubscribe:List-Archive:List-Post:List-Help:Sender:Delivered-To; b=bfdkekUQaLD8xDYcjdT8o2QL+aL4uK4BWZBFRRw6oImYcxhFULRcvDNLr23bOp KbAvs+6p8DpiiyPgdLHPUb3UOsIomBIaP6yCnXSl+4L7tE5NUMcy8hFYTaaxdy7w 9rUgITt2e2dkqmEFgZrAMmuE7sE8x3JAZ03y0vK/rzMm4=; Received: (qmail 5767 invoked by alias); 18 Nov 2012 17:36:07 -0000 Received: (qmail 5703 invoked by uid 22791); 18 Nov 2012 17:36:06 -0000 X-SWARE-Spam-Status: No, hits=-3.6 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, TW_EG X-Spam-Check-By: sourceware.org Received: from mail-wg0-f51.google.com (HELO mail-wg0-f51.google.com) (74.125.82.51) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Sun, 18 Nov 2012 17:36:00 +0000 Received: by mail-wg0-f51.google.com with SMTP id ei8so68775wgb.8 for ; Sun, 18 Nov 2012 09:35:58 -0800 (PST) Received: by 10.180.86.233 with SMTP id s9mr5782049wiz.22.1353260158691; Sun, 18 Nov 2012 09:35:58 -0800 (PST) Received: from localhost ([2.26.182.84]) by mx.google.com with ESMTPS id az2sm9893817wib.7.2012.11.18.09.35.57 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 18 Nov 2012 09:35:58 -0800 (PST) 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: [4/8] Add bit_field_mode_iterator References: <87k3u3eybu.fsf@talisman.home> <87390rexub.fsf@talisman.home> <1899065.5XNjVddQ6H@polaris> Date: Sun, 18 Nov 2012 17:35:56 +0000 In-Reply-To: <1899065.5XNjVddQ6H@polaris> (Eric Botcazou's message of "Tue, 13 Nov 2012 13:41:57 +0100") Message-ID: <87k3tix18j.fsf@talisman.default> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/24.1 (gnu/linux) MIME-Version: 1.0 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Eric Botcazou writes: >> get_best_mode has various checks to decide what counts as an acceptable >> bitfield mode. It actually has two copies of them, with slightly different >> alignment checks: >> >> MIN (unit, BIGGEST_ALIGNMENT) > align >> >> vs. >> >> unit <= MIN (align, BIGGEST_ALIGNMENT) >> >> The second looks more correct, since we can't necessarily guarantee >> larger alignments than BIGGEST_ALIGNMENT in all cases. > > Under the assumption that integer modes really require maximal > alignment, i.e. whatever BIGGEST_ALIGNMENT is, I agree. > >> This patch adds a new iterator class that can be used to walk through >> the modes, and rewrites get_best_mode to use it. I kept the existing >> checks with two changes: >> >> - bitregion_start is now tested independently of bitregion_end > > The comments needs to be updated then. > >> + volatilep_ (volatilep), count_ (0) >> +{ >> + if (bitregion_end_) >> + bitregion_end_ += 1; >> +} > > IMO this is confusing. I think bitregion_end/bitregion_end_ should have a > consistent meaning. > >> +/* Calls to this function return successively larger modes that can be used >> + to represent the bitfield. Return true if another bitfield mode is + >> available, storing it in *OUT_MODE if so. */ >> + >> +bool bit_field_mode_iterator::next_mode (enum machine_mode *out_mode) > > 'bool' on its own line I think. Thanks. Here's the version I committed. Richard gcc/ * machmode.h (bit_field_mode_iterator): New class. (get_best_mode): Change final parameter to bool. * stor-layout.c (bit_field_mode_iterator::bit_field_mode_iterator) (bit_field_mode_iterator::next_mode): New functions, split out from... (get_best_mode): ...here. Change final parameter to bool. Use bit_field_mode_iterator. Index: gcc/machmode.h =================================================================== --- gcc/machmode.h 2012-11-18 17:22:01.515895170 +0000 +++ gcc/machmode.h 2012-11-18 17:22:43.313844317 +0000 @@ -259,13 +259,36 @@ extern enum machine_mode int_mode_for_mo extern enum machine_mode mode_for_vector (enum machine_mode, unsigned); +/* A class for iterating through possible bitfield modes. */ +class bit_field_mode_iterator +{ +public: + bit_field_mode_iterator (HOST_WIDE_INT, HOST_WIDE_INT, + HOST_WIDE_INT, HOST_WIDE_INT, + unsigned int, bool); + bool next_mode (enum machine_mode *); + bool prefer_smaller_modes (); + +private: + enum machine_mode mode_; + /* We use signed values here because the bit position can be negative + for invalid input such as gcc.dg/pr48335-8.c. */ + HOST_WIDE_INT bitsize_; + HOST_WIDE_INT bitpos_; + HOST_WIDE_INT bitregion_start_; + HOST_WIDE_INT bitregion_end_; + unsigned int align_; + bool volatilep_; + int count_; +}; + /* Find the best mode to use to access a bit field. */ extern enum machine_mode get_best_mode (int, int, unsigned HOST_WIDE_INT, unsigned HOST_WIDE_INT, unsigned int, - enum machine_mode, int); + enum machine_mode, bool); /* Determine alignment, 1<=result<=BIGGEST_ALIGNMENT. */ Index: gcc/stor-layout.c =================================================================== --- gcc/stor-layout.c 2012-11-18 17:22:37.791271752 +0000 +++ gcc/stor-layout.c 2012-11-18 17:26:31.159273478 +0000 @@ -2624,14 +2624,103 @@ fixup_unsigned_type (tree type) layout_type (type); } +/* Construct an iterator for a bitfield that spans BITSIZE bits, + starting at BITPOS. + + BITREGION_START is the bit position of the first bit in this + sequence of bit fields. BITREGION_END is the last bit in this + sequence. If these two fields are non-zero, we should restrict the + memory access to that range. Otherwise, we are allowed to touch + any adjacent non bit-fields. + + ALIGN is the alignment of the underlying object in bits. + VOLATILEP says whether the bitfield is volatile. */ + +bit_field_mode_iterator +::bit_field_mode_iterator (HOST_WIDE_INT bitsize, HOST_WIDE_INT bitpos, + HOST_WIDE_INT bitregion_start, + HOST_WIDE_INT bitregion_end, + unsigned int align, bool volatilep) +: mode_ (GET_CLASS_NARROWEST_MODE (MODE_INT)), bitsize_ (bitsize), + bitpos_ (bitpos), bitregion_start_ (bitregion_start), + bitregion_end_ (bitregion_end), align_ (MIN (align, BIGGEST_ALIGNMENT)), + volatilep_ (volatilep), count_ (0) +{ +} + +/* Calls to this function return successively larger modes that can be used + to represent the bitfield. Return true if another bitfield mode is + available, storing it in *OUT_MODE if so. */ + +bool +bit_field_mode_iterator::next_mode (enum machine_mode *out_mode) +{ + for (; mode_ != VOIDmode; mode_ = GET_MODE_WIDER_MODE (mode_)) + { + unsigned int unit = GET_MODE_BITSIZE (mode_); + + /* Skip modes that don't have full precision. */ + if (unit != GET_MODE_PRECISION (mode_)) + continue; + + /* Skip modes that are too small. */ + if ((bitpos_ % unit) + bitsize_ > unit) + continue; + + /* Stop if the mode is too wide to handle efficiently. */ + if (unit > MAX_FIXED_MODE_SIZE) + break; + + /* Don't deliver more than one multiword mode; the smallest one + should be used. */ + if (count_ > 0 && unit > BITS_PER_WORD) + break; + + /* Stop if the mode is wider than the alignment of the containing + object. + + It is tempting to omit the following line unless STRICT_ALIGNMENT + is true. But that is incorrect, since if the bitfield uses part + of 3 bytes and we use a 4-byte mode, we could get a spurious segv + if the extra 4th byte is past the end of memory. + (Though at least one Unix compiler ignores this problem: + that on the Sequent 386 machine. */ + if (unit > align_) + break; + + /* Stop if the mode goes outside the bitregion. */ + HOST_WIDE_INT start = bitpos_ - (bitpos_ % unit); + if (bitregion_start_ && start < bitregion_start_) + break; + if (bitregion_end_ && start + unit > bitregion_end_ + 1) + break; + + *out_mode = mode_; + mode_ = GET_MODE_WIDER_MODE (mode_); + count_++; + return true; + } + return false; +} + +/* Return true if smaller modes are generally preferred for this kind + of bitfield. */ + +bool +bit_field_mode_iterator::prefer_smaller_modes () +{ + return (volatilep_ + ? targetm.narrow_volatile_bitfield () + : !SLOW_BYTE_ACCESS); +} + /* Find the best machine mode to use when referencing a bit field of length BITSIZE bits starting at BITPOS. BITREGION_START is the bit position of the first bit in this sequence of bit fields. BITREGION_END is the last bit in this sequence. If these two fields are non-zero, we should restrict the - memory access to a maximum sized chunk of - BITREGION_END - BITREGION_START + 1. Otherwise, we are allowed to touch + memory access to that range. Otherwise, we are allowed to touch any adjacent non bit-fields. The underlying object is known to be aligned to a boundary of ALIGN bits. @@ -2655,69 +2744,21 @@ get_best_mode (int bitsize, int bitpos, unsigned HOST_WIDE_INT bitregion_start, unsigned HOST_WIDE_INT bitregion_end, unsigned int align, - enum machine_mode largest_mode, int volatilep) + enum machine_mode largest_mode, bool volatilep) { + bit_field_mode_iterator iter (bitsize, bitpos, bitregion_start, + bitregion_end, align, volatilep); + enum machine_mode widest_mode = VOIDmode; enum machine_mode mode; - unsigned int unit = 0; - unsigned HOST_WIDE_INT maxbits; - - /* If unset, no restriction. */ - if (!bitregion_end) - maxbits = MAX_FIXED_MODE_SIZE; - else - maxbits = bitregion_end - bitregion_start + 1; - - /* Find the narrowest integer mode that contains the bit field. */ - for (mode = GET_CLASS_NARROWEST_MODE (MODE_INT); mode != VOIDmode; - mode = GET_MODE_WIDER_MODE (mode)) + while (iter.next_mode (&mode) + && (largest_mode == VOIDmode + || GET_MODE_SIZE (mode) <= GET_MODE_SIZE (largest_mode))) { - unit = GET_MODE_BITSIZE (mode); - if (unit == GET_MODE_PRECISION (mode) - && (bitpos % unit) + bitsize <= unit) + widest_mode = mode; + if (iter.prefer_smaller_modes ()) break; } - - if (mode == VOIDmode - /* It is tempting to omit the following line - if STRICT_ALIGNMENT is true. - But that is incorrect, since if the bitfield uses part of 3 bytes - and we use a 4-byte mode, we could get a spurious segv - if the extra 4th byte is past the end of memory. - (Though at least one Unix compiler ignores this problem: - that on the Sequent 386 machine. */ - || MIN (unit, BIGGEST_ALIGNMENT) > align - || (largest_mode != VOIDmode && unit > GET_MODE_BITSIZE (largest_mode)) - || unit > maxbits - || (bitregion_end - && bitpos - (bitpos % unit) + unit > bitregion_end + 1)) - return VOIDmode; - - if ((SLOW_BYTE_ACCESS && ! volatilep) - || (volatilep && !targetm.narrow_volatile_bitfield ())) - { - enum machine_mode wide_mode = VOIDmode, tmode; - - for (tmode = GET_CLASS_NARROWEST_MODE (MODE_INT); tmode != VOIDmode; - tmode = GET_MODE_WIDER_MODE (tmode)) - { - unit = GET_MODE_BITSIZE (tmode); - if (unit == GET_MODE_PRECISION (tmode) - && bitpos / unit == (bitpos + bitsize - 1) / unit - && unit <= BITS_PER_WORD - && unit <= MIN (align, BIGGEST_ALIGNMENT) - && unit <= maxbits - && (largest_mode == VOIDmode - || unit <= GET_MODE_BITSIZE (largest_mode)) - && (bitregion_end == 0 - || bitpos - (bitpos % unit) + unit <= bitregion_end + 1)) - wide_mode = tmode; - } - - if (wide_mode != VOIDmode) - return wide_mode; - } - - return mode; + return widest_mode; } /* Gets minimal and maximal values for MODE (signed or unsigned depending on