From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 56491 invoked by alias); 16 Apr 2015 09:02:25 -0000 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 Received: (qmail 56479 invoked by uid 89); 16 Apr 2015 09:02:24 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.5 required=5.0 tests=AWL,BAYES_00,RCVD_IN_DNSWL_NONE autolearn=ham version=3.3.2 X-HELO: mo4-p00-ob.smtp.rzone.de Received: from mo4-p00-ob.smtp.rzone.de (HELO mo4-p00-ob.smtp.rzone.de) (81.169.146.163) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Thu, 16 Apr 2015 09:02:22 +0000 X-RZG-AUTH: :LXoWVUeid/7A29J/hMvvT3ol15ykJcYwTPbBBR62PQx1xqvTHw== X-RZG-CLASS-ID: mo00 Received: from [192.168.0.22] (ip5b43a95f.dynamic.kabel-deutschland.de [91.67.169.95]) by smtp.strato.de (RZmta 37.5 DYNA|AUTH) with ESMTPSA id g00ac7r3G92F9Zy (using TLSv1.2 with cipher ECDHE-RSA-AES256-SHA (curve secp521r1 with 521 ECDH bits, eq. 15360 bits RSA)) (Client did not present a certificate); Thu, 16 Apr 2015 11:02:15 +0200 (CEST) Message-ID: <552F7A8D.1080709@gjlay.de> Date: Thu, 16 Apr 2015 09:02:00 -0000 From: Georg-Johann Lay User-Agent: Mozilla/5.0 (X11; Linux i686; rv:31.0) Gecko/20100101 Thunderbird/31.6.0 MIME-Version: 1.0 To: Senthil Kumar Selvaraj , gcc-patches@gcc.gnu.org CC: chertykov@gmail.com Subject: Re: [patch,avr] Fix PR 65657 - read from __memx address space tramples arguments to function call References: <20150416064347.GA31491@atmel.com> In-Reply-To: <20150416064347.GA31491@atmel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2015-04/txt/msg00800.txt.bz2 Am 04/16/2015 um 08:43 AM schrieb Senthil Kumar Selvaraj: > This patch fixes PR 65657. The following artifact appears to be PR63633. > When cfgexpand.c expands a function call, it first figures out the > registers in which the arguments will go, followed by expansion of the > arguments themselves (right to left). It later emits mov insns to set > the precomputed registers with the expanded RTXes. > > If one of the arguments is a __memx char pointer dereference, the mov > eventually expands to gen_xload_A (for certain cases), which > clobbers R22, R21 and Z. This causes problems if one of those > clobbered registers was precomputed to hold another argument. > > In general, call expansion does not appear to take clobbers into account - > when it checks for argument overlap, the RTX (args[i].value) is only a MEM > in QImode for the memx deref - the clobber shows up when it eventually > calls emit_move_insn, at which point, it is too late. > > This does not happen for a int pointer dereference - turns out that > precompute_register_parameters does a copy_to_mode_reg if the > cost of args[i].value is more than COSTS_N_INSNS(1) i.e., it creates a > pseudo and later assigns the pseudo to the arg register. This is done > before any moves to arg registers is done, so other arguments are not > overwritten. > > Doing the same thing - providing a better cost estimate for a MEM rtx in > the non-default address space, makes this problem go away, and that is > what this patch does. Regression testing does not show any new failures. > > The fix does seem tangential to the core problem - not sure if there is > a better way to fix this? > > If not, could someone commit this please? I don't have commit access. > > Regards > Senthil > > gcc/ChangeLog > > 2015-04-16 Senthil Kumar Selvaraj > > * config/avr/avr.c (avr_rtx_costs_1): Increase cost for > MEM rtx in non-default address space. > > gcc/testsuite/ChangeLog > > 2015-04-16 Senthil Kumar Selvaraj > > * gcc.target/avr/pr65657.c: New. > > diff --git gcc/config/avr/avr.c gcc/config/avr/avr.c > index 68d5ddc..c9b8678 100644 > --- gcc/config/avr/avr.c > +++ gcc/config/avr/avr.c > @@ -9959,7 +9959,11 @@ avr_rtx_costs_1 (rtx x, int codearg, int outer_code ATTRIBUTE_UNUSED, > return true; > > case MEM: > - *total = COSTS_N_INSNS (GET_MODE_SIZE (mode)); > + /* MEM rtx with non-default address space is more > + expensive. Not expressing that results in reg > + clobber during expand (PR 65657). */ > + *total = COSTS_N_INSNS (GET_MODE_SIZE (mode) > + + (MEM_ADDR_SPACE(x) == ADDR_SPACE_RAM ? 0 : 5)); > return true; IMO costs should not be used to fix wrong code or ICEs. Presumably the fix is similar to the one used by PR63633. Johann > case NEG: > diff --git gcc/testsuite/gcc.target/avr/pr65657.c gcc/testsuite/gcc.target/avr/pr65657.c > new file mode 100644 > index 0000000..d908fe3 > --- /dev/null > +++ gcc/testsuite/gcc.target/avr/pr65657.c > @@ -0,0 +1,39 @@ > +/* { dg-do run } */ > +/* { dg-options "-Os" } */ > + > +/* When cfgexpand expands the call to foo, it > + assigns registers R22:R23 to 0xABCD and R24 > + to *x. Because x is a __memx address space > + pointer, the dereference results in an RTL > + insn that clobbers R22 among other > + registers (xload_A). > + > + Call expansion does not take this into account > + and ends up clobbering R22 set previously to > + hold part of the second argument. > +*/ > + > +#include > + > +void __attribute__((noinline)) > + __attribute__((noclone)) > +foo (char c, volatile unsigned int d) > +{ > + if (d != 0xABCD) > + abort(); > + if (c != 'A') > + abort(); > +} > + > +void __attribute__((noinline)) > + __attribute__((noclone)) > +readx(const char __memx *x) > +{ > + foo (*x, 0xABCD); > +} > + > +const char __memx arr[] = { 'A' }; > +int main() > +{ > + readx (arr); > +} >