From patchwork Tue Jan 17 20:38:00 2012 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 136525 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 8739FB6EEE for ; Wed, 18 Jan 2012 07:38:56 +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=1327437536; 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=FMFXoh6L+rLn5txeE4KB fwGevGg=; b=R5J8x6vfCsz1gSHqiM/wvEjDXN6z0nCbW2kEmNeLSr5zeE3tpEkj crDzqKhwVQ/SfL9WiGoFOLEUpddL0RGEEbV32qTThh5H703QfEHvtYbc7i/CZ36D mbM08P2Ukz+92M36p8cQk7fuguFTD5KrBedjENoN7oau8ebDXJbDMCQ= 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=SxzXU4WlaUi4kYSnh4Odf552IomRvlj4jlFUr2pRej0hYy9GETBpzsFcZDISYB TCcR4PPsMkcaxD0MzZAHE7EntD+Qn3aAl4hxDS8emLOfCzZNJog1uD7pP3q1Xan2 wf1FibWU4XWKYCJ5ZtTCwDlkcx/+WUsZnDkrrcgf3Gs/k=; Received: (qmail 12991 invoked by alias); 17 Jan 2012 20:38:35 -0000 Received: (qmail 12366 invoked by uid 22791); 17 Jan 2012 20:38:26 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL, BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-wi0-f175.google.com (HELO mail-wi0-f175.google.com) (209.85.212.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 17 Jan 2012 20:38:12 +0000 Received: by wibhr14 with SMTP id hr14so28541wib.20 for ; Tue, 17 Jan 2012 12:38:11 -0800 (PST) Received: by 10.180.103.97 with SMTP id fv1mr25229484wib.17.1326832691325; Tue, 17 Jan 2012 12:38:11 -0800 (PST) Received: from localhost (rsandifo.gotadsl.co.uk. [82.133.89.107]) by mx.google.com with ESMTPS id di5sm47797187wib.3.2012.01.17.12.38.08 (version=TLSv1/SSLv3 cipher=OTHER); Tue, 17 Jan 2012 12:38:09 -0800 (PST) From: Richard Sandiford To: Iain Sandoe Mail-Followup-To: Iain Sandoe , Andreas Krebbel , David Edelsohn , GCC Patches , rdsandiford@googlemail.com Cc: Andreas Krebbel , David Edelsohn , GCC Patches Subject: Re: [PATCH] PR50325 store_bit_field: Fix for big endian targets References: <4EC4D324.9070307@linux.vnet.ibm.com> <121213B0-3F2C-4D95-8B79-E8F39226DB95@sandoe-acoustics.co.uk> <87fwhjcdh9.fsf@firetop.home> Date: Tue, 17 Jan 2012 20:38:00 +0000 In-Reply-To: <87fwhjcdh9.fsf@firetop.home> (Richard Sandiford's message of "Sun, 20 Nov 2011 10:09:06 +0000") Message-ID: <8762ga6p5j.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (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 Richard Sandiford writes: > Iain Sandoe writes: >> On 17 Nov 2011, at 09:25, Andreas Krebbel wrote: >>> On 11/17/2011 03:44 AM, David Edelsohn wrote: >>>> Andreas, >>>> >>>> This patch seems to have introduced a failure for all of the >>>> gcc.dg-struct-layout tests on AIX. >>>> >>>> gcc.dg-struct-layout-1/t001_test.h:8:1: internal compiler error: in >>>> int_mode_for_mode, at stor-layout.c:424 >>>> >>>> After your change, int_mode_for_mode now is passed VOIDmode because >>>> the rtx is a CONST_INT. >>> >>> extract_bit_field is only able to deal with regs and mems. For >>> constants it should not be >>> necessary anyway. Could you please test the following patch: >>> >>> Index: gcc/expmed.c >>> =================================================================== >>> *** gcc/expmed.c.orig 2011-11-15 20:03:46.000000000 +0100 >>> --- gcc/expmed.c 2011-11-17 09:12:22.487783491 +0100 >>> *************** store_bit_field_1 (rtx str_rtx, unsigned >>> *** 562,570 **** >>> MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD); >>> >>> /* If the remaining chunk doesn't have full wordsize we have >>> ! to make that for big endian machines the higher order >>> ! bits are used. */ >>> ! if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN) >>> value_word = extract_bit_field (value_word, new_bitsize, 0, >>> true, false, NULL_RTX, >>> BLKmode, word_mode); >>> --- 562,572 ---- >>> MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD); >>> >>> /* If the remaining chunk doesn't have full wordsize we have >>> ! to make sure that for big endian machines the higher >>> ! order bits are used. */ >>> ! if (BYTES_BIG_ENDIAN >>> ! && GET_MODE (value_word) != VOIDmode >>> ! && new_bitsize < BITS_PER_WORD) >>> value_word = extract_bit_field (value_word, new_bitsize, 0, >>> true, false, NULL_RTX, >>> BLKmode, word_mode); >>> >> >> with this + r181436 on powerpc-darwin9, the ICEs are gone .. >> .. but all the struct-layout-1 tests are failing on execute. >> >> Running /GCC/gcc-live-trunk/gcc/testsuite/gcc.dg/compat/struct- >> layout-1.exp ... >> FAIL: tmpdir-gcc.dg-struct-layout-1/t001 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t002 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t003 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t004 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t005 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t006 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t007 c_compat_x_tst.o- >> c_compat_y_tst.o execute >> FAIL: tmpdir-gcc.dg-struct-layout-1/t008 c_compat_x_tst.o- >> c_compat_y_tst.o execute > > Yeah, I don't think constants are any different here. One fix might be > to use simplify_expand_binop instead of extract_bit_field, as per the > patch below. The patch also restricts the shifting to forward walks, > as discussed in the PR trail. > > But I'm not sure I understand the fieldmode != BLKmode test in: > > unsigned int backwards = WORDS_BIG_ENDIAN && fieldmode != BLKmode; > > Adding it was the (only) net effect of r7985 and r8007, > but the comment: > > However, only do that if the value is not BLKmode. */ > > is less than helpful when trying to discern a reason. Even in those days, > the code was followed by: > > /* This is the mode we must force value to, so that there will be enough > subwords to extract. Note that fieldmode will often (always?) be > VOIDmode, because that is what store_field uses to indicate that this > is a bit field, but passing VOIDmode to operand_subword_force will > result in an abort. */ > fieldmode = mode_for_size (nwords * BITS_PER_WORD, MODE_INT, 0); > > This latter comment is clearly talking about the value of fieldmode > before rather than after the call to mode_for_size, so there seems to > have been a bit of confusion about what fieldmode was actually supposed > to be (comment says VOIDmode, r8007 says that BLKmode is both possible > and special). > > The pre-r7985 code honoured the comment: > > for (i = 0; i < nwords; i++) > { > /* If I is 0, use the low-order word in both field and target; > if I is 1, use the next to lowest word; and so on. */ > > And any trimming would always happen in the last iteration (i == nwords - 1); > in other words, in the high word. > > The point of r8007 seems to be that WORDS_BIG_ENDIAN doesn't apply to > BLKmode fieldmodes, but there must surely be some endianness involved > somewhere, given that we're extracting words from a multiword value? > > This of course predates the public mailing lists by several years. > > Anyway, it seems on face value that the original patch for this PR > (the "experimental fix", which is different from the submitted version, > but which AIUI avoids the PowerPC and MIPS regressions) is equivalent > to removing that "&& fieldmode != BLKmode" check on BYTES_BIG_ENDIAN > == WORDS_BIG_ENDIAN targets. If that turns out to be correct, I wonder > what should happen for BYTES_BIG_ENDIAN != WORDS_BIG_ENDIAN? The only > two examples I can see are pdp11 and an obselete ARM mode that will be > removed in 4.8. > > So if possible, I'd prefer to revert the original patch for this PR > and instead adjust the "backwards" test. I'm just not sure whether > it should be "WORDS_BIG_ENDIAN" (reverting r8007, which was obviously > applied for a reason) or something like: > > (fieldmode == BLKmode ? BYTES_BIG_ENDIAN : WORDS_BIG_ENDIAN) It was decided in the PR trail that we should go with the simplify_expand_binop patch, so I (reluctantly!) applied the patch below. Richard gcc/ 2012-01-17 Andreas Krebbel Richard Sandiford PR middle-end/50325 PR middle-end/51192 * optabs.h (simplify_expand_binop): Declare. * optabs.c (simplify_expand_binop): Make global. * expmed.c (store_bit_field_1): Use simplify_expand_binop on big endian targets if the source cannot be exactly covered by word mode chunks. Index: gcc/optabs.h =================================================================== --- gcc/optabs.h 2012-01-17 20:31:45.000000000 +0000 +++ gcc/optabs.h 2012-01-17 20:32:00.000000000 +0000 @@ -859,6 +859,10 @@ extern rtx expand_ternary_op (enum machi extern rtx expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int, enum optab_methods); +extern rtx simplify_expand_binop (enum machine_mode mode, optab binoptab, + rtx op0, rtx op1, rtx target, int unsignedp, + enum optab_methods methods); + extern bool force_expand_binop (enum machine_mode, optab, rtx, rtx, rtx, int, enum optab_methods); Index: gcc/optabs.c =================================================================== --- gcc/optabs.c 2012-01-17 20:31:45.000000000 +0000 +++ gcc/optabs.c 2012-01-17 20:32:00.000000000 +0000 @@ -659,7 +659,7 @@ expand_ternary_op (enum machine_mode mod calculated at compile time. The arguments and return value are otherwise the same as for expand_binop. */ -static rtx +rtx simplify_expand_binop (enum machine_mode mode, optab binoptab, rtx op0, rtx op1, rtx target, int unsignedp, enum optab_methods methods) Index: gcc/expmed.c =================================================================== --- gcc/expmed.c 2012-01-17 20:31:45.000000000 +0000 +++ gcc/expmed.c 2012-01-17 20:32:00.000000000 +0000 @@ -557,9 +557,21 @@ store_bit_field_1 (rtx str_rtx, unsigned 0) : (int) i * BITS_PER_WORD); rtx value_word = operand_subword_force (value, wordnum, fieldmode); + unsigned HOST_WIDE_INT new_bitsize = + MIN (BITS_PER_WORD, bitsize - i * BITS_PER_WORD); - if (!store_bit_field_1 (op0, MIN (BITS_PER_WORD, - bitsize - i * BITS_PER_WORD), + /* If the remaining chunk doesn't have full wordsize we have + to make sure that for big endian machines the higher order + bits are used. */ + if (new_bitsize < BITS_PER_WORD && BYTES_BIG_ENDIAN && !backwards) + value_word = simplify_expand_binop (word_mode, lshr_optab, + value_word, + GEN_INT (BITS_PER_WORD + - new_bitsize), + NULL_RTX, true, + OPTAB_LIB_WIDEN); + + if (!store_bit_field_1 (op0, new_bitsize, bitnum + bit_offset, bitregion_start, bitregion_end, word_mode,