From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 78704 invoked by alias); 23 Mar 2017 09:05:16 -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 77153 invoked by uid 89); 23 Mar 2017 09:05:15 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-0.4 required=5.0 tests=BAYES_00,KAM_LAZY_DOMAIN_SECURITY,RP_MATCHES_RCVD,URI_NOVOWEL autolearn=no version=3.3.2 spammy=interest X-HELO: foss.arm.com Received: from foss.arm.com (HELO foss.arm.com) (217.140.101.70) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Thu, 23 Mar 2017 09:05:14 +0000 Received: from usa-sjc-imap-foss1.foss.arm.com (unknown [10.72.51.249]) by usa-sjc-mx-foss1.foss.arm.com (Postfix) with ESMTP id 508A12B; Thu, 23 Mar 2017 02:05:13 -0700 (PDT) Received: from [10.2.206.201] (e105545-lin.cambridge.arm.com [10.2.206.201]) by usa-sjc-imap-foss1.foss.arm.com (Postfix) with ESMTPSA id 66FC43F3E1; Thu, 23 Mar 2017 02:05:12 -0700 (PDT) Subject: Re: [PATCH][ARM] PR target/71436: Restrict *load_multiple pattern till after LRA To: Kyrill Tkachov , Jakub Jelinek , "Richard Earnshaw (lists)" References: <583F02B0.3030406@foss.arm.com> <325eb8f2-a5e0-62af-be0c-0d85a53812a4@arm.com> <64d133e5-75b7-2bde-ce66-b629e8a094d4@arm.com> <20161219145357.GQ21933@tucnak> <587F3A3A.7050003@foss.arm.com> <5899DE97.1030900@foss.arm.com> Cc: GCC Patches , Ramana Radhakrishnan From: Ramana Radhakrishnan Message-ID: <1f6745c1-c677-15a4-4c21-3aa832906830@foss.arm.com> Date: Thu, 23 Mar 2017 09:05:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.7.0 MIME-Version: 1.0 In-Reply-To: <5899DE97.1030900@foss.arm.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit X-IsSubscribed: yes X-SW-Source: 2017-03/txt/msg01206.txt.bz2 On 07/02/17 14:49, Kyrill Tkachov wrote: > > On 18/01/17 09:49, Kyrill Tkachov wrote: >> >> On 19/12/16 14:53, Jakub Jelinek wrote: >>> On Thu, Dec 15, 2016 at 10:00:14AM +0000, Richard Earnshaw (lists) >>> wrote: >>>> sorry, pasted the wrong bit of code. >>>> >>>> That should read when we generate: >>>> >>>> (insn 55 19 67 3 (parallel [ >>>> (set (reg:SI 0 r0) >>>> (mem/u/c:SI (reg/f:SI 147) [2 c+0 S4 A32])) >>>> (set (reg:SI 158) >>>> (mem/u/c:SI (plus:SI (reg/f:SI 147) >>>> (const_int 4 [0x4])) [2 c+4 S4 A32])) >>>> ]) "ldm.c":25 404 {*load_multiple} >>>> (expr_list:REG_UNUSED (reg:SI 0 r0) >>>> (nil))) >>>> >>>> ie when we put a pseudo into the register load list. >>> We put a pseudo there because the predicate on the insn allows it: >>> (define_special_predicate "load_multiple_operation" >>> (match_code "parallel") >>> { >>> return ldm_stm_operation_p (op, /*load=*/true, SImode, >>> /*consecutive=*/false, >>> /*return_pc=*/false); >>> }) >>> and the consecutive = false argument says that (almost) no verification >>> is performed on the SET_DEST, just that it is a REG and doesn't have >>> REGNO smaller than the first reg. >>> That said, RA is still not able to cope with such instructions, because >>> only the first set is represented with constraints, so if such an insn >>> needs any kind of reloading, it just will not happen. >>> So I think the posted patch makes lots of sense, otherwise if you use >>> such a pattern before reload, you just have to hope no reloading will be >>> needed on it. >> >> In that case I believe restricting the pattern till after LRA is the >> way to go. >> However, we should still add the check from [1] to ldm_stm_operation_p >> for when >> 'consecutive' is true as consecutive order has no useful meaning on >> pseudos here. >> > > In the interest of fixing this PR I think the patch at > https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html > is the way to go, that is disable this pattern until after reload. > It was intended to handle epilogue pops that is generated after reload > anyway and all the general load/store multiple > patterns are handled by ldmstmd.md so not having this before reload is > not risky. > > I'll also propose a variant of > https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html to tighten up > the consecutivity > check when 'consecutive' is passed down as true, but that is > indepdendent of this PR. > > Is the patch at https://gcc.gnu.org/ml/gcc-patches/2016-11/msg03078.html > ok for trunk? > It's been in my tree for a couple of months now getting testing and > there's been no fallout. > We shouldn't generate any ldm / stm operations for anything other than memcpy before reload and all of those are capped at 4 integer registers (MAX_LDM_STM_OPS in arm.h) at a time and that will be handled almost entirely by the patterns in ldmstm.md. The way to generate this generically this would be through movmem expanders or through the load_multiple expander in the backend. However all of this falls through the arm_gen_load_multiple interface all of which goes through arm_gen_multiple_op and we have an assert in arm_gen_multiple_op that the number of registers is <= MAX_LDM_STM_OPS. Thus OK, please apply but be aware of any fallout Thanks, Ramana > Thanks, > Kyrill > >> >> >> [1] https://gcc.gnu.org/ml/gcc-patches/2016-12/msg01400.html >> >>> >>> Jakub >> >