From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 29031 invoked by alias); 8 Aug 2012 13:40:33 -0000 Received: (qmail 29022 invoked by uid 22791); 8 Aug 2012 13:40:32 -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 X-Spam-Check-By: sourceware.org Received: from mail-vb0-f47.google.com (HELO mail-vb0-f47.google.com) (209.85.212.47) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Wed, 08 Aug 2012 13:40:18 +0000 Received: by vbbfr13 with SMTP id fr13so749212vbb.20 for ; Wed, 08 Aug 2012 06:40:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.52.33.69 with SMTP id p5mr12060758vdi.46.1344433217183; Wed, 08 Aug 2012 06:40:17 -0700 (PDT) Received: by 10.58.165.104 with HTTP; Wed, 8 Aug 2012 06:40:17 -0700 (PDT) In-Reply-To: References: <20120801184138.GA3787@intel.com> <87a9ye1lsh.fsf@talisman.home> <87r4rlkcf1.fsf@talisman.home> Date: Wed, 08 Aug 2012 13:40:00 -0000 Message-ID: Subject: Re: PATCH: PR rtl-optimization/54157: [x32] -maddress-mode=long failures From: "H.J. Lu" To: "H.J. Lu" , gcc-patches@gcc.gnu.org, Uros Bizjak , 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/msg00431.txt.bz2 On Wed, Aug 8, 2012 at 1:08 AM, Richard Sandiford 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. Thanks. -- H.J. --- gcc/ 2012-08-08 Richard Sandiford H.J. Lu PR rtl-optimization/54157 * combine.c (gen_lowpart_for_combine): Don't return identity for CONST or symbolic reference. gcc/testsuite/ 2012-08-08 H.J. Lu PR rtl-optimization/54157 * gcc.target/i386/pr54157.c: New file. diff --git a/gcc/combine.c b/gcc/combine.c index 495e129..2b91eb9 100644 --- a/gcc/combine.c +++ b/gcc/combine.c @@ -10634,13 +10634,6 @@ 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; - /* We can only support MODE being wider than a word if X is a constant integer or has a mode the same size. */ if (GET_MODE_SIZE (omode) > UNITS_PER_WORD diff --git a/gcc/testsuite/gcc.target/i386/pr54157.c b/gcc/testsuite/gcc.target/i386/pr54157.c new file mode 100644 index 0000000..b5c4528 --- /dev/null +++ b/gcc/testsuite/gcc.target/i386/pr54157.c @@ -0,0 +1,21 @@ +/* { dg-do compile { target { ! { ia32 } } } } */ +/* { dg-options "-O2 -mx32 -maddress-mode=long -ftree-vectorize" } */ + +struct s2{ + int n[24 -1][24 -1][24 -1]; +}; + +struct test2{ + struct s2 e; +}; + +struct test2 tmp2[4]; + +void main1 () +{ + int i,j; + + for (i = 0; i < 24 -4; i++) + for (j = 0; j < 24 -4; j++) + tmp2[2].e.n[1][i][j] = 8; +}