From patchwork Sun May 29 20:33:20 2011 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Sandiford X-Patchwork-Id: 1663 Return-Path: Delivered-To: unknown Received: from imap.gmail.com (74.125.159.109) by localhost6.localdomain6 with IMAP4-SSL; 08 Jun 2011 14:54:02 -0000 Delivered-To: patches@linaro.org Received: by 10.52.110.9 with SMTP id hw9cs204618vdb; Sun, 29 May 2011 13:33:25 -0700 (PDT) Received: by 10.227.201.16 with SMTP id ey16mr4327581wbb.30.1306701204867; Sun, 29 May 2011 13:33:24 -0700 (PDT) Received: from mail-wy0-f178.google.com (mail-wy0-f178.google.com [74.125.82.178]) by mx.google.com with ESMTPS id e7si7343490wbh.65.2011.05.29.13.33.23 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 May 2011 13:33:23 -0700 (PDT) Received-SPF: pass (google.com: domain of rdsandiford@googlemail.com designates 74.125.82.178 as permitted sender) client-ip=74.125.82.178; Authentication-Results: mx.google.com; spf=pass (google.com: domain of rdsandiford@googlemail.com designates 74.125.82.178 as permitted sender) smtp.mail=rdsandiford@googlemail.com; dkim=pass (test mode) header.i=@googlemail.com Received: by wyb33 with SMTP id 33so2791949wyb.37 for ; Sun, 29 May 2011 13:33:23 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=googlemail.com; s=gamma; h=domainkey-signature:from:to:mail-followup-to:cc:subject:references :date:in-reply-to:message-id:user-agent:mime-version:content-type; bh=SckwM2g9+Um1aHiaTiJPP0rtTnnwA8SfnDTnzkOvwj4=; b=uzh6v8f8CqQV6cvKfEc1X9kBHWm1NOrreVKfW64FU0eWajR2BUWnLx5ZETIHinYwbZ 9ujItd10jqfDb7yuKjLPthppLwAKG0u6u5dYemJbu9qJynBVci/n0P9G1TbKkl3yH85a v+tcY6L9TPhXn1i/y6xqhrDhmH/pYQn0ZOR54= DomainKey-Signature: a=rsa-sha1; c=nofws; d=googlemail.com; s=gamma; h=from:to:mail-followup-to:cc:subject:references:date:in-reply-to :message-id:user-agent:mime-version:content-type; b=b/84UPieholA1i7cZpsXciZMs1QRvi943FKZycQI6YUoRWBRgS5JbKEA0qYvbj5WJ/ 1P/FvO62mh4lMcz3s6rIAl67Tw4j9TOV557iuF3ikeN2ftkpyAELppzoxqLH/RknW4s/ qhzh657yP8sHZA1B07wdX2fu9Lcjl7iPCDZGU= Received: by 10.227.37.220 with SMTP id y28mr4093562wbd.82.1306701203265; Sun, 29 May 2011 13:33:23 -0700 (PDT) Received: from localhost (rsandifo.gotadsl.co.uk [82.133.89.107]) by mx.google.com with ESMTPS id fm14sm2604058wbb.7.2011.05.29.13.33.21 (version=TLSv1/SSLv3 cipher=OTHER); Sun, 29 May 2011 13:33:22 -0700 (PDT) From: Richard Sandiford To: Chung-Lin Tang Mail-Followup-To: Chung-Lin Tang , gcc-patches , Richard Earnshaw , patches@linaro.org, rdsandiford@googlemail.com Cc: gcc-patches , Richard Earnshaw , patches@linaro.org Subject: Re: [patch, ARM] Fix PR48808, PR48792: More work on CANNOT_CHANGE_MODE_CLASS References: <4DE28F42.2070507@codesourcery.com> Date: Sun, 29 May 2011 21:33:20 +0100 In-Reply-To: <4DE28F42.2070507@codesourcery.com> (Chung-Lin Tang's message of "Mon, 30 May 2011 02:24:02 +0800") Message-ID: <87mxi5jlgv.fsf@firetop.home> User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/23.2 (gnu/linux) MIME-Version: 1.0 Chung-Lin Tang writes: > An alternative fix direction would be to add similar mode/regclass > validity checks to all places where CANNOT_CHANGE_MODE_CLASS is used in > the RTL core, which may stratify things a bit more (the internals > documentation does not mention anything on whether the backend should > check this mode/regclass condition by itself). But this seems a bit > scattered and probably a little risky, so I'll settle for the ARM > specific fix for now. I don't think the backend should need to check it, although there are probably backends have done so as the path of least resistance. It's true that CANNOT_CHANGE_CLASS_MODE is used in places where unsimplifiable subregs of hard regs are otherwise allowed. This is because some ports generate such subregs deliberately. (That is, they say that register R cannot have mode N, then generate (subreg:N (reg:M R)) in cases where they'd like to refer to R in mode N anyway.) No-one has been brave enough to clean that up. But what's happening here seems like a plain bug to me. Or rather, two bugs: - push_reload turns (subreg:M (foo:N)) reloads into (foo:N) reloads without first checking whether the register class has any registers of mode N. (Note that IRA _does_ perform the corresponding check. That is, IRA checks contains_reg_of_mode whenever it checks invalid_mode_change_p. This seems like a good thing.) - If we have a reload of a paradoxical subreg, reload can turn the other reload operand into an unsimplifiable (subreg:M (reg:N R)). We might have to allow ports to generate that kind of subreg themselves (for now!), but reload shouldn't be generating new ones like this. Something like the attached patch. Not tested yet, and I'm sure it'll break things in lots of fun and interesting ways... Richard Index: gcc/reload.c =================================================================== --- gcc/reload.c (revision 174400) +++ gcc/reload.c (working copy) @@ -1019,6 +1019,7 @@ #ifdef CANNOT_CHANGE_MODE_CLASS && !CANNOT_CHANGE_MODE_CLASS (GET_MODE (SUBREG_REG (in)), inmode, rclass) #endif + && contains_reg_of_mode[(int) rclass][(int) GET_MODE (SUBREG_REG (in))] && (CONSTANT_P (SUBREG_REG (in)) || GET_CODE (SUBREG_REG (in)) == PLUS || strict_low @@ -1125,6 +1126,7 @@ #ifdef CANNOT_CHANGE_MODE_CLASS && !CANNOT_CHANGE_MODE_CLASS (GET_MODE (SUBREG_REG (out)), outmode, rclass) #endif + && contains_reg_of_mode[(int) rclass][(int) GET_MODE (SUBREG_REG (out))] && (CONSTANT_P (SUBREG_REG (out)) || strict_low || (((REG_P (SUBREG_REG (out)) Index: gcc/reload1.c =================================================================== --- gcc/reload1.c (revision 174400) +++ gcc/reload1.c (working copy) @@ -4471,6 +4471,43 @@ } } } + +/* *FOCUS_PTR and *OTHER_PTR are two operands to a conceptual reload. + If *FOCUS_PTR is a paradoxical subreg, try to remove that subreg + and apply the corresponding narrowing subreg to *OTHER_PTR. + Return true if the operands were changed, false otherwise. */ + +static bool +strip_paradoxical_subreg (rtx *focus_ptr, rtx *other_ptr) +{ + rtx focus, inner, other, tem; + + focus = *focus_ptr; + if (GET_MODE (focus) != SUBREG) + return false; + + inner = SUBREG_REG (focus); + if (GET_MODE_SIZE (GET_MODE (focus)) <= GET_MODE_SIZE (GET_MODE (inner))) + return false; + + other = *other_ptr; + tem = gen_lowpart_common (GET_MODE (inner), other); + if (!tem) + return false; + + /* If the lowpart operation turned a hard register into a subreg, + rather than simplifying it to another hard register, then the + mode change cannot be properly represented. For example, OTHER + might be valid in its current mode, but not in the new one. */ + if (GET_CODE (tem) == SUBREG + && REG_P (other) + && HARD_REGISTER_P (other)) + return false; + + *focus_ptr = inner; + *other_ptr = tem; + return true; +} /* A subroutine of reload_as_needed. If INSN has a REG_EH_REGION note, examine all of the reload insns between PREV and NEXT exclusive, and @@ -5538,7 +5575,7 @@ chain reloads or do need an intermediate hard registers. */ bool result = true; int regno, n, code; - rtx out, in, tem, insn; + rtx out, in, insn; rtx last = get_last_insn (); /* Make r2 a component of r1. */ @@ -5557,11 +5594,7 @@ /* If IN is a paradoxical SUBREG, remove it and try to put the opposite SUBREG on OUT. Likewise for a paradoxical SUBREG on OUT. */ - if (GET_CODE (in) == SUBREG - && (GET_MODE_SIZE (GET_MODE (in)) - > GET_MODE_SIZE (GET_MODE (SUBREG_REG (in)))) - && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (in)), out)) != 0) - in = SUBREG_REG (in), out = tem; + strip_paradoxical_subreg (&in, &out); if (GET_CODE (in) == PLUS && (REG_P (XEXP (in, 0)) @@ -7557,7 +7590,6 @@ if (tertiary_icode != CODE_FOR_nothing) { rtx third_reloadreg = rld[tertiary_reload].reg_rtx; - rtx tem; /* Copy primary reload reg to secondary reload reg. (Note that these have been swapped above, then @@ -7566,13 +7598,7 @@ /* If REAL_OLD is a paradoxical SUBREG, remove it and try to put the opposite SUBREG on RELOADREG. */ - if (GET_CODE (real_old) == SUBREG - && (GET_MODE_SIZE (GET_MODE (real_old)) - > GET_MODE_SIZE (GET_MODE (SUBREG_REG (real_old)))) - && 0 != (tem = gen_lowpart_common - (GET_MODE (SUBREG_REG (real_old)), - reloadreg))) - real_old = SUBREG_REG (real_old), reloadreg = tem; + strip_paradoxical_subreg (&real_old, &reloadreg); gen_reload (reloadreg, second_reloadreg, rl->opnum, rl->when_needed); @@ -8388,16 +8414,8 @@ /* If IN is a paradoxical SUBREG, remove it and try to put the opposite SUBREG on OUT. Likewise for a paradoxical SUBREG on OUT. */ - if (GET_CODE (in) == SUBREG - && (GET_MODE_SIZE (GET_MODE (in)) - > GET_MODE_SIZE (GET_MODE (SUBREG_REG (in)))) - && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (in)), out)) != 0) - in = SUBREG_REG (in), out = tem; - else if (GET_CODE (out) == SUBREG - && (GET_MODE_SIZE (GET_MODE (out)) - > GET_MODE_SIZE (GET_MODE (SUBREG_REG (out)))) - && (tem = gen_lowpart_common (GET_MODE (SUBREG_REG (out)), in)) != 0) - out = SUBREG_REG (out), in = tem; + if (!strip_paradoxical_subreg (&in, &out)) + strip_paradoxical_subreg (&out, &in); /* How to do this reload can get quite tricky. Normally, we are being asked to reload a simple operand, such as a MEM, a constant, or a pseudo