From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 2016 invoked by alias); 8 Aug 2012 13:50:18 -0000 Received: (qmail 2004 invoked by uid 22791); 8 Aug 2012 13:50:17 -0000 X-SWARE-Spam-Status: No, hits=-5.0 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,TW_ZJ X-Spam-Check-By: sourceware.org Received: from mail-vc0-f175.google.com (HELO mail-vc0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Aug 2012 13:50:04 +0000 Received: by vcbfy27 with SMTP id fy27so759575vcb.20 for ; Wed, 08 Aug 2012 06:50:03 -0700 (PDT) MIME-Version: 1.0 Received: by 10.58.221.135 with SMTP id qe7mr9554739vec.23.1344433803134; Wed, 08 Aug 2012 06:50:03 -0700 (PDT) Received: by 10.58.165.104 with HTTP; Wed, 8 Aug 2012 06:50:03 -0700 (PDT) In-Reply-To: References: <20120801184138.GA3787@intel.com> <87a9ye1lsh.fsf@talisman.home> <87r4rlkcf1.fsf@talisman.home> Date: Wed, 08 Aug 2012 13:50:00 -0000 Message-ID: Subject: Re: PATCH: PR rtl-optimization/54157: [x32] -maddress-mode=long failures From: "H.J. Lu" To: Uros Bizjak Cc: gcc-patches@gcc.gnu.org, rdsandiford@googlemail.com Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes 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/msg00434.txt.bz2 On Wed, Aug 8, 2012 at 6:43 AM, Uros Bizjak wrote: > On Wed, Aug 8, 2012 at 3:40 PM, H.J. Lu wrote: > >> wrote: >>> "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 >> >> This is the patch I checked in. > > Probably we need to backport this patch to 4.7, where x32 is > -maddress-mode=long by default. > It doesn't fail on 4.7 branch since checking mode on PLUS CONST is new on trunk. However, I think it is a correctness issue. Is this OK to backport to 4.7? Thanks. -- H.J.