From patchwork Fri Oct 18 20:17:21 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Mikael Pettersson X-Patchwork-Id: 284715 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]) (using TLSv1 with cipher DHE-RSA-AES256-SHA (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id 1A1282C0092 for ; Sat, 19 Oct 2013 07:17:38 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :mime-version:content-type:content-transfer-encoding:message-id :date:to:cc:subject:in-reply-to:references; q=dns; s=default; b= TnkDL04NoiEQ9oPJg5d+xkuwl0tw+y8kZy74kytq1nKiXNuCWoI7kMwOdNSP7y/e Y8Aokg/NBwK9KjmYfkLo9wBoBpg+Jjb3fy+gZoeBCbRHDhBJUbb583OLRBbI+9BK 0USMjCCFUIXCY1v0jbpaOuLXY4AW026HoAAW/DCncys= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:from :mime-version:content-type:content-transfer-encoding:message-id :date:to:cc:subject:in-reply-to:references; s=default; bh=Gb4byM RtTv2g1X1pvFi0j8ISOAk=; b=Lfv04zlFbxjoBXV8e59koCY7MElrZnTXwaLsKc c+13YFS+vcEPbRsjK6oGbOUKzS/4IOLslfDHFLnC8p2UBzu+u/taFKxxlgWLp3Gz KsvTHsnG8qYlHUm34T6BysF5Ihi8kGWqUvw8usX1OudtUL4HH3y3LmnyxFfognKS hgbno= Received: (qmail 7356 invoked by alias); 18 Oct 2013 20:17:32 -0000 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 Received: (qmail 7346 invoked by uid 89); 18 Oct 2013 20:17:31 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.6 required=5.0 tests=AWL, BAYES_00, FREEMAIL_FROM, RCVD_IN_DNSWL_LOW, SPF_PASS autolearn=ham version=3.3.2 X-HELO: mail-la0-f49.google.com Received: from mail-la0-f49.google.com (HELO mail-la0-f49.google.com) (209.85.215.49) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Fri, 18 Oct 2013 20:17:29 +0000 Received: by mail-la0-f49.google.com with SMTP id ev20so1122655lab.8 for ; Fri, 18 Oct 2013 13:17:26 -0700 (PDT) X-Received: by 10.152.171.72 with SMTP id as8mr2955995lac.33.1382127445988; Fri, 18 Oct 2013 13:17:25 -0700 (PDT) Received: from cascade (h109n3-u-a31.ias.bredband.telia.com. [213.65.120.109]) by mx.google.com with ESMTPSA id kx1sm3017736lac.7.2013.10.18.13.17.23 for (version=TLSv1 cipher=RC4-SHA bits=128/128); Fri, 18 Oct 2013 13:17:24 -0700 (PDT) Received: by cascade (sSMTP sendmail emulation); Fri, 18 Oct 2013 22:17:22 +0200 From: Mikael Pettersson MIME-Version: 1.0 Message-ID: <21089.38737.974487.155238@gargle.gargle.HOWL> Date: Fri, 18 Oct 2013 22:17:21 +0200 To: Jeff Law Cc: Mikael Pettersson , gcc-patches@gcc.gnu.org Subject: Re: [PATCH][RFC] fix reload causing ICE in subreg_get_info on m68k (PR58369) In-Reply-To: <5255AC6B.4030009@redhat.com> References: <21062.62988.797370.296379@gargle.gargle.HOWL> <5255AC6B.4030009@redhat.com> Jeff Law writes: > On 09/28/13 09:30, Mikael Pettersson wrote: > > This patch fixes PR58369, an ICE in subreg_get_info when compiling > > boost for m68k-linux. > > > > choose_reload_regs attempts to reload a DFmode (8-byte) reg, finds > > an XFmode (12-byte) reg in "last_reg", and calls subreg_regno_offset > > with these two modes and a subreg offset of zero. However, this is > > not a correct lowpart subreg offset for big-endian and these two modes, > > so the lowpart subreg check in subreg_get_info fails, and the code > > continues to > > > > gcc_assert ((GET_MODE_SIZE (xmode) % GET_MODE_SIZE (ymode)) == 0); > > > > which fails because (12 % 8) != 0. > > > > choose_reload_regs passes the constant zero, in all cases where the reg > > isn't already a subreg, as the subreg offset to subreg_regno_offset, even > > though lowpart subregs on big-endian targets require an explicit offset > > computation. I think that is a bug. > > > > I believe other big-endian targets don't see this ICE because > > a) they define CANNOT_CHANGE_MODE_CLASS to reject differently-sized > > modes in floating-point registers (which prevents this path in > > choose_reload_regs), or > > b) their differently-sized modes are such that the size of a larger > > mode is a whole multiple of the size of the smaller mode (which > > allows the gcc_assert above to pass). > > > > This patch changes choose_reload_regs to call subreg_lowpart_offset > > to pass an endian-correct offset to subreg_regno_offset, except where > > the offset comes from a pre-existing subreg. > > > > [Defining CANNOT_CHANGE_MODE_CLASS appropriately for m68k also fixes > > the ICE, but I don't think the m68k backend really wants that, and I > > think it just papers over a generic bug.] > > > > Tested with trunk and 4.8 on {m68k,sparc64,powerpc64}-linux (big-endian), > > and on x86_64-linux/armv5tel-linux-gnueabi (little-endian). No regressions. > > > > Comments? > > Is this Ok for trunk? > > > > gcc/ > > > > 2013-09-28 Mikael Pettersson > > > > PR rtl-optimization/58369 > > * reload1.c (choose_reload_regs): Use subreg_lowpart_offset > > to pass endian-correct lowpart offset to subreg_regno_offset. > Thanks Mikael. My only concern is the lack of adjustment when the value > found was already a SUBREG. > > ie, let's assume rld[r].in_reg was something like > (subreg:XF (reg:DF) 0) > > and our target is (reg:DF) > > In this case it seems to me we still want to compute the subreg offset, > right? > > jeff Thanks Jeff. Sorry about the delay, but it took me a while to work out the details of the various cases (I was afraid of having to do something like simplify_subreg, but the cases in reload are simpler), and then to re-test on my various targets. Let the reloadee be rld[r].in_reg, outermode be its mode, and innermode be the mode of the hard reg in last_reg. The trivial cases are when the reloadee is a plain REG or a SUBREG of a hard reg. For these, reload virtually forms a normal lowpart subreg of last_reg, and subreg_lowpart_offset (outermode, innermode) computes the endian-correct offset for subreg_regno_offset. This is exactly what my previous patch did. In remaining cases the reloadee is a SUBREG of a pseudo. Let outer_offset be its BYTE, and middlemode be the mode of its REG. Another simple case is when the reloadee is paradoxical. Then outer_offset is zero (by convention), and reload should just form a normal lowpart subreg as in the trivial cases. Even though the reloadee is paradoxical, this subreg will fit thanks to the mode size check on lines 6546-6547. If the reloadee is a normal lowpart SUBREG, then again reload should just form a normal lowpart subreg as in the trivial cases. (But see below.) The tricky case is when the reloadee is a normal but not lowpart SUBREG. We get the correct offset for reload's virtual subreg by adding outer_offset to the lowpart offset of middlemode and innermode. This works for both big-endian and little-endian. It also handles normal lowpart SUBREGs, so we don't need to check for lowpart vs non-lowpart normal SUBREGs. Tested with trunk and 4.8 on {m68k,sparc64,powerpc64,x86_64}-linux-gnu and armv5tel-linux-gnueabi. No regressions. Ok for trunk? gcc/ 2013-10-18 Mikael Pettersson PR rtl-optimization/58369 * reload1.c (compute_reload_subreg_offset): Define. (choose_reload_regs): Use it to pass endian-correct offset to subreg_regno_offset. --- gcc-4.9-20131006/gcc/reload1.c.~1~ 2013-09-28 10:42:34.000000000 +0200 +++ gcc-4.9-20131006/gcc/reload1.c 2013-10-14 01:04:29.815104039 +0200 @@ -6363,6 +6363,34 @@ replaced_subreg (rtx x) } #endif +/* Compute the offset to pass to subreg_regno_offset, for a pseudo of + mode OUTERMODE that is available in a hard reg of mode INNERMODE. + SUBREG is non-NULL if the pseudo is a subreg whose reg is a pseudo, + otherwise it is NULL. */ + +static int +compute_reload_subreg_offset (enum machine_mode outermode, rtx subreg, enum machine_mode innermode) +{ + int outer_offset; + enum machine_mode middlemode; + + if (! subreg) + return subreg_lowpart_offset (outermode, innermode); + + outer_offset = SUBREG_BYTE (subreg); + middlemode = GET_MODE (SUBREG_REG (subreg)); + + /* If SUBREG is paradoxical then return the normal lowpart offset + for OUTERMODE and INNERMODE. Our caller has already checked + that OUTERMODE fits in INNERMODE. */ + if (outer_offset == 0 && GET_MODE_SIZE (outermode) > GET_MODE_SIZE (middlemode)) + return subreg_lowpart_offset (outermode, innermode); + + /* SUBREG is normal, but may not be lowpart; return OUTER_OFFSET + plus the normal lowpart offset for MIDDLEMODE and INNERMODE. */ + return outer_offset + subreg_lowpart_offset (middlemode, innermode); +} + /* Assign hard reg targets for the pseudo-registers we must reload into hard regs for this insn. Also output the instructions to copy them in and out of the hard regs. @@ -6500,6 +6528,7 @@ choose_reload_regs (struct insn_chain *c int byte = 0; int regno = -1; enum machine_mode mode = VOIDmode; + rtx subreg = NULL_RTX; if (rld[r].in == 0) ; @@ -6520,7 +6549,10 @@ choose_reload_regs (struct insn_chain *c if (regno < FIRST_PSEUDO_REGISTER) regno = subreg_regno (rld[r].in_reg); else - byte = SUBREG_BYTE (rld[r].in_reg); + { + subreg = rld[r].in_reg; + byte = SUBREG_BYTE (subreg); + } mode = GET_MODE (rld[r].in_reg); } #ifdef AUTO_INC_DEC @@ -6558,6 +6590,7 @@ choose_reload_regs (struct insn_chain *c rtx last_reg = reg_last_reload_reg[regno]; i = REGNO (last_reg); + byte = compute_reload_subreg_offset (mode, subreg, GET_MODE (last_reg)); i += subreg_regno_offset (i, GET_MODE (last_reg), byte, mode); last_class = REGNO_REG_CLASS (i);