From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15166 invoked by alias); 8 Aug 2012 08:09:07 -0000 Received: (qmail 15147 invoked by uid 22791); 8 Aug 2012 08:09:02 -0000 X-SWARE-Spam-Status: No, hits=-4.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE X-Spam-Check-By: sourceware.org Received: from mail-bk0-f47.google.com (HELO mail-bk0-f47.google.com) (209.85.214.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Aug 2012 08:08:48 +0000 Received: by bkcik5 with SMTP id ik5so170809bkc.20 for ; Wed, 08 Aug 2012 01:08:47 -0700 (PDT) Received: by 10.204.152.27 with SMTP id e27mr6844941bkw.56.1344413327277; Wed, 08 Aug 2012 01:08:47 -0700 (PDT) Received: from richards-thinkpad.stglab.manchester.uk.ibm.com (gbibp9ph1--blueice2n1.emea.ibm.com. [195.212.29.75]) by mx.google.com with ESMTPS id t23sm9778682bks.4.2012.08.08.01.08.45 (version=TLSv1/SSLv3 cipher=OTHER); Wed, 08 Aug 2012 01:08:46 -0700 (PDT) From: Richard Sandiford To: "H.J. Lu" Mail-Followup-To: "H.J. Lu" ,gcc-patches@gcc.gnu.org, Uros Bizjak , rdsandiford@googlemail.com Cc: gcc-patches@gcc.gnu.org, Uros Bizjak Subject: Re: PATCH: PR rtl-optimization/54157: [x32] -maddress-mode=long failures References: <20120801184138.GA3787@intel.com> <87a9ye1lsh.fsf@talisman.home> <87r4rlkcf1.fsf@talisman.home> Date: Wed, 08 Aug 2012 08:09:00 -0000 In-Reply-To: (H. J. Lu's message of "Tue, 7 Aug 2012 12:28:34 -0700") Message-ID: 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-08/txt/msg00405.txt.bz2 "H.J. Lu" writes: > diff --git a/gcc/combine.c b/gcc/combine.c > index a07c046..b9a3589 100644 > --- a/gcc/combine.c > +++ b/gcc/combine.c > @@ -10784,12 +10784,30 @@ gen_lowpart_for_combine (enum machine_mode omode, rtx > x) > if (omode == imode) > return x; > > - /* Return identity if this is a CONST or symbolic reference. */ > - if (omode == Pmode > - && (GET_CODE (x) == CONST > - || GET_CODE (x) == SYMBOL_REF > - || GET_CODE (x) == LABEL_REF)) > - return x; > + if (omode == Pmode) > + { > + /* Return identity if this is a symbolic reference. */ > + if (GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF) > + return x; > + > + /* Return identity for CONST unless this is a PLUS of 2 constant > + operands. */ > + if (GET_CODE (x) == CONST) > + { > + rtx y = XEXP (x, 0); > + if (GET_CODE (y) == PLUS > + && ((CONST_INT_P (XEXP (y, 1)) > + && (GET_CODE (XEXP (y, 0)) == CONST > + || GET_CODE (XEXP (y, 0)) == SYMBOL_REF > + || GET_CODE (XEXP (y, 0)) == LABEL_REF)) > + || (CONST_INT_P (XEXP (y, 1)) > + && (GET_CODE (XEXP (y, 0)) == CONST > + || GET_CODE (XEXP (y, 0)) == SYMBOL_REF > + || GET_CODE (XEXP (y, 0)) == LABEL_REF)))) > + goto fail; > + return x; > + } > + } > > /* We can only support MODE being wider than a word if X is a > constant integer or has a mode the same size. */ > > works for the testcase. My point was that the "return x" is always wrong. Whenever we return x here, we know we're returning something in a different mode from the one that the caller wanted. Returning a Pmode LABEL_REF might not trigger that plus_constant assert, but it's still wrong. It looks like this came from the mips-rewrite branch: 2003-03-13 Richard Sandiford * combine.c (gen_lowpart_for_combine): Treat the lowpart Pmode of a CONST as identity. Check the return value of gen_lowpart_common. so I can categorically confirm that the person who wrote it didn't know what they were doing. It also means that this case was motivated by an experiment to make Pmode == DImode for n32, which we ended up discarding because it produced worse code. If this case really is important, we might consider using convert_memory_address (Pmode, x) instead. I'm not sure whether that would be right for targets with address spaces though, because we don't know which address space is associated with the address. Hopefully someone who knows address spaces can comment. If it is correct, then it should probably go in gen_lowpart_common rather than gen_lowpart_for_combine. But given how few people have hit this, it doesn't look like a particularly important attempted optimisation. I'll pre-approve a patch that undoes my mistake and simply removes: /* Return identity if this is a CONST or symbolic reference. */ if (omode == Pmode && (GET_CODE (x) == CONST || GET_CODE (x) == SYMBOL_REF || GET_CODE (x) == LABEL_REF)) return x; Richard