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

[Martin Storsjö]

On Tue, 6 Feb 2018, Janne Grunau wrote:

> 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.

No, it turned out to be because I'm building with a triplet 
armv7-w64-mingw32, while configure currently explicitly looks for 
host_cpu = "arm". I'll send patches for that later, after this one 
settles.

> [...]
>
>> > 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.

Fair enough.

>> > 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.

In that case yes, a common include also is fine. I can live with it being 
in the arm_neon directory for now as well, as long as the issue is 
acknowledged :-)

So my only remaining suggestion is to use #ifdef __ELF__ instead of 
#ifndef __APPLE__ for these, since out of elf/coff/macho, only elf 
actually handles them. (The exact condition within LLVM for enabling them 
is !IsMachO && !IsCOFF though: 
https://github.com/llvm-mirror/llvm/blob/master/lib/Target/ARM/AsmParser/ARMAsmParser.cpp#L9168)

// Martin