From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 18561 invoked by alias); 26 Apr 2013 17:05:15 -0000 Mailing-List: contact libc-ports-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: List-Subscribe: List-Post: List-Help: , Sender: libc-ports-owner@sourceware.org Received: (qmail 18549 invoked by uid 89); 26 Apr 2013 17:05:14 -0000 X-Spam-SWARE-Status: No, score=-4.6 required=5.0 tests=AWL,BAYES_00,KHOP_RCVD_UNTRUST,KHOP_THREADED,RCVD_IN_HOSTKARMA_W,RCVD_IN_HOSTKARMA_WL,TW_CP,TW_HW,TW_VF autolearn=ham version=3.3.1 Received: from relay1.mentorg.com (HELO relay1.mentorg.com) (192.94.38.131) by sourceware.org (qpsmtpd/0.84/v0.84-167-ge50287c) with ESMTP; Fri, 26 Apr 2013 17:05:13 +0000 Received: from svr-orw-exc-10.mgc.mentorg.com ([147.34.98.58]) by relay1.mentorg.com with esmtp id 1UVm59-0002By-QS from joseph_myers@mentor.com ; Fri, 26 Apr 2013 10:05:11 -0700 Received: from SVR-IES-FEM-01.mgc.mentorg.com ([137.202.0.104]) by SVR-ORW-EXC-10.mgc.mentorg.com with Microsoft SMTPSVC(6.0.3790.4675); Fri, 26 Apr 2013 10:05:11 -0700 Received: from digraph.polyomino.org.uk (137.202.0.76) by SVR-IES-FEM-01.mgc.mentorg.com (137.202.0.104) with Microsoft SMTP Server id 14.2.247.3; Fri, 26 Apr 2013 18:05:09 +0100 Received: from jsm28 (helo=localhost) by digraph.polyomino.org.uk with local-esmtp (Exim 4.76) (envelope-from ) id 1UVm56-0006ny-RB; Fri, 26 Apr 2013 17:05:08 +0000 Date: Fri, 26 Apr 2013 17:05:00 -0000 From: "Joseph S. Myers" To: Will Newton CC: , Subject: Re: [PATCH, v3] ARM: Add Cortex-A15 optimized NEON and VFP memcpy routines, with IFUNC. In-Reply-To: <51752F94.8080105@linaro.org> Message-ID: References: <51752F94.8080105@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset="US-ASCII" X-SW-Source: 2013-04/txt/msg00103.txt.bz2 On Mon, 22 Apr 2013, Will Newton wrote: > diff --git a/ports/sysdeps/arm/armv7/multiarch/ifunc-impl-list.c b/ports/sysdeps/arm/armv7/multiarch/ifunc-impl-list.c > +#include > +#include > +#include > +#include > +#include > +#include What's the need for and ? I don't see anything obviously using them in this file. (If it's an indirect use, it's probably better to make the header in question include the other header it needs, so each file is including exactly those headers from which it uses functionality directly.) > + IFUNC_IMPL_ADD (array, i, memcpy, hwcap & HWCAP_ARM_VFPv3, > + __memcpy_vfp) Why VFPv3 here? In the .S file you use HWCAP_ARM_VFP. > diff --git a/ports/sysdeps/arm/armv7/multiarch/memcpy.S b/ports/sysdeps/arm/armv7/multiarch/memcpy.S > +#include I don't see any actual use of anything from arm-features.h here. (Arguably you *should* be using it - if ARM_HAVE_VFP, you don't need to consider the possibility of VFP being missing, although you still need to build __memcpy_arm for __aeabi_memcpy use. And if the compiler has NEON enabled for the build, __memcpy_vfp isn't needed at all, although in that case it would be better to avoid all the IFUNC handling and just have __memcpy_arm for __aeabi_memcpy and use in ld.so. But in any case, either use arm-features.h or remove the include.) > +/* Old versions of GAS incorrectly implement the NEON align semantics. */ State explicit version numbers, "versions before x.y". That way we can tell when this code is obsolete because the versions in question aren't supported by glibc. But, since I don't see a configure test that would set this BROKEN_ASM_NEON_ALIGN macro, if it's relevant for binutils 2.20 (the oldest version supported by glibc at present) then there should be such a test, and if it's not relevant for 2.20 then the conditionals shouldn't be here at all. (Or, you could make a case for a more recent binutils version as the minimum on ARM, but there should probably be a configure test for that if a more recent version is needed.) > +#define PC_OFFSET 8 /* PC pipeline compensation. */ We have a macro PC_OFS in sysdep.h, I suggest using it. > +.Lcpy_not_short: > + /* At least 64 bytes to copy, but don't know the alignment yet. */ > + str tmp2, [sp, #-FRAME_SIZE]! All stack adjustments and saves/restores of call-preserved registers should have associated CFI annotations for debug info (you may also need cfi_remember_state / cfi_restore_start in appropriate places where branches are involved, so that the CFI is correct at all places in the function; this is just the first example of a place needing CFI that I saw). -- Joseph S. Myers joseph@codesourcery.com