[vlc-devel] [PATCH 1/1] arm: fix assembling with llvm's integrated assembler for ios

[Janne Grunau]

On 2018-02-06 23:54:55 +0200, Martin Storsjö wrote:
> On Tue, 6 Feb 2018, Janne Grunau wrote:
> 
> > ---
> > modules/arm_neon/amplify.S                     |  8 ++++--
> > modules/arm_neon/asm.S                         | 31 +++++++++++++++++++++
> > modules/arm_neon/deinterleave_chroma.S         | 10 ++++---
> > modules/arm_neon/i420_rgb.S                    | 10 ++++---
> > modules/arm_neon/i420_rv16.S                   | 10 ++++---
> > modules/arm_neon/i420_yuyv.S                   | 14 +++++-----
> > modules/arm_neon/i422_yuyv.S                   | 14 +++++-----
> > modules/arm_neon/nv12_rgb.S                    | 10 ++++---
> > modules/arm_neon/nv21_rgb.S                    | 10 ++++---
> > modules/arm_neon/simple_channel_mixer.S        | 38 +++++++++-----------------
> > modules/arm_neon/yuyv_i422.S                   | 14 +++++-----
> > modules/video_filter/deinterlace/merge_arm.S   | 20 ++++++--------
> > modules/video_filter/deinterlace/merge_arm64.S | 10 +++----
> > 13 files changed, 112 insertions(+), 87 deletions(-)
> > create mode 100644 modules/arm_neon/asm.S
> > 
> > diff --git a/modules/arm_neon/amplify.S b/modules/arm_neon/amplify.S
> > index 5938118378..711a3ea4f7 100644
> > --- a/modules/arm_neon/amplify.S
> > +++ b/modules/arm_neon/amplify.S
> > @@ -18,18 +18,20 @@
> >  @ Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> >  @****************************************************************************/
> > 
> > +#include "asm.S"
> > +
> > 	.syntax	unified
> > 	.arm
> > +#ifndef __APPLE__
> > 	.fpu	neon
> > +#endif
> > 	.text
> 
> Or maybe go for #ifdef __ELF__, since this isn't supported when targeting
> windows either. (My arm/clang/win builds don't seem to have enabled the
> arm_neon modules though so I haven't touched this code wrt that yet.)

I didn't think about other platforms. I refrained from testing this in 
configure since autotools as no support for invoking the assembler in 
configure compile tests. For .arch/fpu testing with inline asm would 
work since it is supported when .arch/.fpu will be used.

Testing for neon with inline asm could be reason why it is not enabled 
for arm/clang/win builds.
 
[...]

> > diff --git a/modules/arm_neon/deinterleave_chroma.S 
> > b/modules/arm_neon/deinterleave_chroma.S
> > index 019d647ed6..509120b920 100644
> > --- a/modules/arm_neon/deinterleave_chroma.S
> > +++ b/modules/arm_neon/deinterleave_chroma.S
> > @@ -19,8 +19,12 @@
> >  @ Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> >  @****************************************************************************/
> > 
> > +#include "asm.S"
> > +
> > 	.syntax unified
> > -	.fpu neon
> > +#ifndef __APPLE__
> > +	.fpu	neon
> > +#endif
> > 	.text
> > 
> 
> Ditto (or maybe fold this part into asm.S?)

not desireable due to the reasons below. also courmisch didn't like all 
the implicit things libav's asm.S does.

> > diff --git a/modules/video_filter/deinterlace/merge_arm.S b/modules/video_filter/deinterlace/merge_arm.S
> > index dd779029ae..3694ff6de2 100644
> > --- a/modules/video_filter/deinterlace/merge_arm.S
> > +++ b/modules/video_filter/deinterlace/merge_arm.S
> > @@ -18,10 +18,14 @@
> >  @ Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> >  @****************************************************************************/
> > 
> > +#include "../arm_neon/asm.S"
> > +
> > 	.syntax	unified
> > 	.arm
> > +#ifndef __APPLE__
> > 	.arch	armv6
> > 	.fpu	neon
> > +#endif
> 
> Ok, so here it's a slightly different prologue, so sharing it would require
> parameters for .arch anyway... Hmm.
> 
> > diff --git a/modules/video_filter/deinterlace/merge_arm64.S b/modules/video_filter/deinterlace/merge_arm64.S
> > index db19e54caf..7b70678891 100644
> > --- a/modules/video_filter/deinterlace/merge_arm64.S
> > +++ b/modules/video_filter/deinterlace/merge_arm64.S
> > @@ -19,6 +19,8 @@
> >  // Inc., 51 Franklin Street, Fifth Floor, Boston MA 02110-1301, USA.
> >  //****************************************************************************/
> > 
> > +#include "../../arm_neon/asm.S"
> > +
> > 	.text
> > 
> > #define	DEST	x0
> 
> Including an arm source file in an arm64 assembly file feels a bit icky,
> even though the current macros are perfectly shareable. (If you'd move more
> of the prologue there, it wouldn't be quite as shareable though.)

having two identical files in the repo is also not nice. I started from 
libav's asm includes for arm and arm64. When I realized that they are 
mostly identical I joined them. The file isn't even arm specific anymore 
(I think). If there's a good place to put internal includes it would be 
better choice than modules/arm_neon.

Janne