From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 5680 invoked by alias); 14 Aug 2007 13:18:12 -0000 Received: (qmail 5584 invoked by uid 22791); 14 Aug 2007 13:18:09 -0000 X-Spam-Check-By: sourceware.org Received: from mail.codesourcery.com (HELO mail.codesourcery.com) (65.74.133.4) by sourceware.org (qpsmtpd/0.31) with ESMTP; Tue, 14 Aug 2007 13:18:04 +0000 Received: (qmail 30876 invoked from network); 14 Aug 2007 13:18:02 -0000 Received: from unknown (HELO gateway) (10.0.0.100) by mail.codesourcery.com with SMTP; 14 Aug 2007 13:18:02 -0000 Received: by gateway (Postfix, from userid 1010) id 6D22C6C0D5; Tue, 14 Aug 2007 06:18:02 -0700 (PDT) From: Richard Sandiford To: gcc-patches@gcc.gnu.org Mail-Followup-To: gcc-patches@gcc.gnu.org, richard@codesourcery.com Subject: Re: RFA: Fix PR 32897 (invalid constants generated by find_reloads_toplev) References: <877ioncqt7.fsf@firetop.home> Date: Tue, 14 Aug 2007 13:18:00 -0000 In-Reply-To: <877ioncqt7.fsf@firetop.home> (Richard Sandiford's message of "Thu\, 26 Jul 2007 13\:06\:28 +0100") Message-ID: <87r6m66yra.fsf@firetop.home> User-Agent: Gnus/5.110006 (No Gnus v0.6) Emacs/21.4 (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: 2007-08/txt/msg00860.txt.bz2 Ping^2 Richard Sandiford writes: > The following testcase: > > --------------------------------------------------------------------------- > volatile int g[32]; > long long gll; > double gd; > > #define MULTI(X) \ > X( 1), X( 2), X( 3), X( 4), X( 5), X( 6), X( 7), X( 8), X( 9), X(10), \ > X(11), X(12), X(13), X(14), X(15), X(16), X(17), X(18), X(19), X(20), \ > X(21), X(22), X(23), X(24), X(25), X(26), X(27), X(28), X(29), X(30) > > #define DECLARE(INDEX) x##INDEX > #define COPY_IN(INDEX) x##INDEX = g[INDEX] > #define COPY_OUT(INDEX) g[INDEX] = x##INDEX > > void > test (int n) > { > union { long long l; double d; } u = { 0x12345678 }; > gll = u.l; > int MULTI (DECLARE); > MULTI (COPY_IN); > MULTI (COPY_OUT); > MULTI (COPY_OUT); > MULTI (COPY_OUT); > gd = u.d; > } > --------------------------------------------------------------------------- > > fails for mips*-elf targets when compiled with -G0 and with > optimisation enabled: > > internal compiler error: in emit_move_insn, at expr.c:3317 > > The pseudo register that holds "u" is not allocated a hard > register, so we rematerialise the constant 0x1234567 on demand. > We check when setting reg_equiv_constant that 0x1234567 does indeed > satisfy LEGITIMATE_CONSTANT_P; we'd force it into memory otherwise. > > However, find_reloads_toplev assumes that simplified subregs of > reg_equiv_constant entries also satisfy LEGITIMATE_CONSTANT_P. > This isn't true here: the original 0x12345678 CONST_INT is legitimate, > but the equivalent DFmode CONST_DOUBLE is not. > > (I came across this while working on another patch. The original > test was more natural; the one above was just reverse-engineered > after the fact.) > > I think the fix is to force the subregged constant into the > constant pool if it isn't itself legitimate. We must then > reload the new MEM's address. > > There are actually two blocks of code that try to create the > subreg of a constant: > > if (subreg_lowpart_p (x) > && regno >= FIRST_PSEUDO_REGISTER > && reg_renumber[regno] < 0 > && reg_equiv_constant[regno] != 0 > && (tem = gen_lowpart_common (GET_MODE (x), > reg_equiv_constant[regno])) != 0) > return tem; > > if (regno >= FIRST_PSEUDO_REGISTER > && reg_renumber[regno] < 0 > && reg_equiv_constant[regno] != 0) > { > tem = > simplify_gen_subreg (GET_MODE (x), reg_equiv_constant[regno], > GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x)); > gcc_assert (tem); > return tem; > } > > But this is redundant: gen_lowpart_common now uses simplify_gen_subreg > for constants anyway. The first block isn't even a fast path; we spend > longer working out whether the special case applies, and what > simplify_gen_subreg's arguments should be if so, than we would if we > went straight to the second block. > > So, rather than add special constant pool handling to both blocks of code, > I simply removed the first block and added the special handling to the second. > > find_reloads assumes that, if find_reloads_toplev returns a MEM for > a SUBREG of a REG, the inner register must be equivalent to a memory > location. That's no longer true, so I've made it also check that the > REG is not equivalent to a constant. (I think that's better than checking > the reg_equiv_mem* arrays, because it documents why the extra condition > is needed.) > > Bootstrapped & regression tested on x86_64-linux-gnu. Also regression- > tested on mipsisa64-elf (flags {,-mips32}{-EB,-EL}{,-msoft-float}). > OK to install? > > Richard > > > gcc/ > PR middle-end/32897 > * reload.c (find_reloads): Check that the memory returned by > find_reloads_toplev was not the result of forcing a constant > to memory. > (find_reloads_toplev): Always use simplify_gen_subreg to get > the subreg of a constant. If the result is also a constant, > but not a legitimate one, force it into the constant pool > and reload its address. > > gcc/testsuite/ > * gcc.dg/torture/pr32897.c: New test. > > Index: gcc/reload.c > =================================================================== > --- gcc/reload.c (revision 126918) > +++ gcc/reload.c (working copy) > @@ -2795,7 +2795,8 @@ find_reloads (rtx insn, int replace, int > && MEM_P (op) > && REG_P (reg) > && (GET_MODE_SIZE (GET_MODE (reg)) > - >= GET_MODE_SIZE (GET_MODE (op)))) > + >= GET_MODE_SIZE (GET_MODE (op))) > + && reg_equiv_constant[REGNO (reg)] == 0) > set_unique_reg_note (emit_insn_before (gen_rtx_USE (VOIDmode, reg), > insn), > REG_EQUAL, reg_equiv_memory_loc[REGNO (reg)]); > @@ -4601,14 +4602,6 @@ find_reloads_toplev (rtx x, int opnum, e > int regno = REGNO (SUBREG_REG (x)); > rtx tem; > > - if (subreg_lowpart_p (x) > - && regno >= FIRST_PSEUDO_REGISTER > - && reg_renumber[regno] < 0 > - && reg_equiv_constant[regno] != 0 > - && (tem = gen_lowpart_common (GET_MODE (x), > - reg_equiv_constant[regno])) != 0) > - return tem; > - > if (regno >= FIRST_PSEUDO_REGISTER > && reg_renumber[regno] < 0 > && reg_equiv_constant[regno] != 0) > @@ -4617,6 +4610,15 @@ find_reloads_toplev (rtx x, int opnum, e > simplify_gen_subreg (GET_MODE (x), reg_equiv_constant[regno], > GET_MODE (SUBREG_REG (x)), SUBREG_BYTE (x)); > gcc_assert (tem); > + if (CONSTANT_P (tem) && !LEGITIMATE_CONSTANT_P (tem)) > + { > + tem = force_const_mem (GET_MODE (x), tem); > + i = find_reloads_address (GET_MODE (tem), &tem, XEXP (tem, 0), > + &XEXP (tem, 0), opnum, type, > + ind_levels, insn); > + if (address_reloaded) > + *address_reloaded = i; > + } > return tem; > } > > Index: gcc/testsuite/gcc.dg/torture/pr32897.c > =================================================================== > --- gcc/testsuite/gcc.dg/torture/pr32897.c (revision 0) > +++ gcc/testsuite/gcc.dg/torture/pr32897.c (revision 0) > @@ -0,0 +1,27 @@ > +/* { dg-options "-G0" { target mips*-*-* } } */ > + > +volatile int g[32]; > +long long gll; > +double gd; > + > +#define MULTI(X) \ > + X( 1), X( 2), X( 3), X( 4), X( 5), X( 6), X( 7), X( 8), X( 9), X(10), \ > + X(11), X(12), X(13), X(14), X(15), X(16), X(17), X(18), X(19), X(20), \ > + X(21), X(22), X(23), X(24), X(25), X(26), X(27), X(28), X(29), X(30) > + > +#define DECLARE(INDEX) x##INDEX > +#define COPY_IN(INDEX) x##INDEX = g[INDEX] > +#define COPY_OUT(INDEX) g[INDEX] = x##INDEX > + > +void > +test (int n) > +{ > + union { long long l; double d; } u = { 0x12345678 }; > + gll = u.l; > + int MULTI (DECLARE); > + MULTI (COPY_IN); > + MULTI (COPY_OUT); > + MULTI (COPY_OUT); > + MULTI (COPY_OUT); > + gd = u.d; > +}