From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 82126 invoked by alias); 12 Feb 2020 09:23:22 -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 82094 invoked by uid 89); 12 Feb 2020 09:23:21 -0000 Authentication-Results: sourceware.org; auth=none X-Spam-SWARE-Status: No, score=-6.1 required=5.0 tests=AWL,BAYES_00,KAM_SHORT,SPF_PASS autolearn=ham version=3.3.1 spammy=me, H*MI:sk:gkra75p, H*f:sk:gkra75p, H*i:sk:gkra75p X-HELO: smtp.ispras.ru Received: from winnie.ispras.ru (HELO smtp.ispras.ru) (83.149.199.91) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 12 Feb 2020 09:23:19 +0000 Received: from [10.10.3.54] (utre4ko.intra.ispras.ru [10.10.3.54]) by smtp.ispras.ru (Postfix) with ESMTP id B86A2203C9; Wed, 12 Feb 2020 12:23:15 +0300 (MSK) Subject: Re: [PATCH] [arm] Implement Armv8.1-M low overhead loops To: Andrea Corallo , "Richard Earnshaw (lists)" Cc: gcc-patches@gcc.gnu.org, nd@arm.com References: <8468875e-934e-0bee-763d-91dd5ddbe7c9@arm.com> From: Roman Zhuykov Message-ID: <4a28de0a-6790-732f-31bd-0e5bdfc12246@ispras.ru> Date: Wed, 12 Feb 2020 09:23:00 -0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:60.0) Gecko/20100101 Thunderbird/60.9.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8bit X-IsSubscribed: yes X-SW-Source: 2020-02/txt/msg00717.txt.bz2 Hello! 11.02.2020 16:40, Andrea Corallo wrote: > Hi Richard, > > "Richard Earnshaw (lists)" writes: > >>> gcc/ChangeLog: >>> 2020-??-?? Andrea Corallo >>> 2020-??-?? Mihail-Calin Ionescu >>> 2020-??-?? Iain Apreotesei >>> * config/arm/arm.c (TARGET_INVALID_WITHIN_DOLOOP): >>> (arm_invalid_within_doloop): Implement invalid_within_doloop hook. >>> * config/arm/arm.h (TARGET_HAVE_LOB): Add new macro. >>> * config/arm/thumb2.md (*doloop_end, doloop_begin, dls_insn): >>> Add new patterns. >>> * config/arm/unspecs.md: Add new unspec. >>> >> A date should only appear before the first author in a multi-author >> patch, other authors should then be indented to align with the name of >> that first author. > Ack This patch is stage1 material, right? > >> +(define_insn "*doloop_end" >> + [(parallel [(set (pc) >> + (if_then_else >> + (ne (reg:SI LR_REGNUM) (const_int 1)) >> + (label_ref (match_operand 0 "" "")) >> + (pc))) >> + (set (reg:SI LR_REGNUM) >> + (plus:SI (reg:SI LR_REGNUM) (const_int -1)))])] >> + "TARGET_32BIT && TARGET_HAVE_LOB && !flag_modulo_sched" >> + "le\tlr, %l0") I'm not an expert in .md files, but having that "!flag_modulo_sched" condition seems wrong to me.  What was the issue on SMS side to add that? Currently, there are fake doloop_end pattern on ARM.  It is generated only when flag_modulo_sched is set and actually expands to more than one instruction.  This old approach have its pros and cons.  When we HAVE_LOB, target allows us to use a real doloop_end instruction, fake one is not needed at all.  In this case compiler should use real instruction regardless whether SMS in on or off. I hope in stage1 after upgrading modulo scheduler, we will restart old discussion about removing fake doloop_end pattern for ARM: https://gcc.gnu.org/ml/gcc-patches/2011-07/msg01812.html https://gcc.gnu.org/ml/gcc-patches/2012-01/msg00195.html Aarch64 also have such a fake pattern since 2014, probably its removal also will be considered. Roman >> Is it deliberate that this pattern name has a '*' prefix? doloop_end >> is a named expansion pattern according to md.texi. > Yes, this should be expanded already by the define_expand we have in > thumb2.md. Perhaps I'll call it 'doloop_end_internal' and add a > comment. > >> Also, hard-coded register names should be prefixed with '%|' (so >> "%|lr", not just "lr"), just in case the assembler dialect requires >> something (ELF doesn't but others have). Also for dls_insn. > Ack > >> For the tests, your 'require-effective-taret' tests look insufficient >> to prevent problems when testing a multilib environment, you'll need >> (at least) checks that a) passing -marm has not happened and b) that >> the architecture, or a specific CPU isn't being passed on the command >> line. > Ack > > Thanks for reviewing I'll update the patch. > > Andrea