[PATCH] aarch64: Enable Neon search_line_fast

[Ramana Radhakrishnan]

On 02/09/14 16:28, Richard Henderson wrote:
> Is it intentional or not that AArch64 does not define __ARM_NEON__?

Yes I remember so, __ARM_NEON__ is not ACLE compatible so we haven't 
defined it for AArch64 - on AArch32 and AArch64 we now have __ARM_NEON 
defined so that's the macro to be used.

>
> Otherwise, here's a better way to fold the test bits.  AArch64 of
> course does not have dN+1 overlap the high part of the qM register,
> like AArch32, so the current
>
>        l = vpadd_u8 (vget_low_u8 (t), vget_high_u8 (t));
>
> implies extra register moves.  But on the good side, the armv8 ADDV
> instruction allows two instructions to be removed from this fast path.

Cool.

>
> When built for 32-bit, the new form results in the same instruction
> count; we simply keep using "q" registers instead of "d" registers
> for two more insns.  Given that there are currently ifdefs involved,
> it would certainly be possible to keep the 32-bit path unchanged, if
> that's thought to be valuable.

The ADDV instruction isn't available on the AArch32 side IIRC. Given 
that situation there is no intrinsic for ADDV on the AArch32 side which 
is why this doesn't exist in the AArch32 version of arm_neon.h :(

I'll need to take a look at the new code generated for AArch32 and will 
probably be able to get back tomorrow as I'll disappear shortly.

>
> I did wonder if the armv8 stuff was supposed to be included in the
> AArch32 arm_neon.h?  Is it just an oversight that it's missing?

The ARMv8 stuff is included for arm_neon.h - I believe we've implemented 
everything that's ARMv8 specific in arm_neon.h for AArch32 . Anything 
missing would be an oversight.


regards
Ramana



>
>
> r~
>
>
> 	* lex.c (search_line_fast) [__ARM_NEON]: Use __FOO not __FOO__
> 	to detect neon support.  Fold the comparison using ADDV when
> 	available.
>
>
> diff --git a/libcpp/lex.c b/libcpp/lex.c
> index 5366dad..6d1823e 100644
> --- a/libcpp/lex.c
> +++ b/libcpp/lex.c
> @@ -638,7 +638,7 @@ search_line_fast (const uchar *s, const uchar *end ATTRIBUTE_UNUSED)
>     }
>   }
>
> -#elif defined (__ARM_NEON__)
> +#elif defined (__ARM_NEON)
>   #include "arm_neon.h"
>
>   static const uchar *
> @@ -649,6 +649,7 @@ search_line_fast (const uchar *s, const uchar *end ATTRIBUTE_UNUSED)
>     const uint8x16_t repl_bs = vdupq_n_u8 ('\\');
>     const uint8x16_t repl_qm = vdupq_n_u8 ('?');
>     const uint8x16_t xmask = (uint8x16_t) vdupq_n_u64 (0x8040201008040201ULL);
> +  const int16x8_t shift = { 0, 0, 0, 0, 8, 8, 8, 8 };
>
>     unsigned int misalign, found, mask;
>     const uint8_t *p;
> @@ -670,10 +671,8 @@ search_line_fast (const uchar *s, const uchar *end ATTRIBUTE_UNUSED)
>
>     do
>       {
> -      uint8x8_t l;
> -      uint16x4_t m;
> -      uint32x2_t n;
>         uint8x16_t t, u, v, w;
> +      uint16x8_t l;
>
>         p += 16;
>         data = vld1q_u8 (p);
> @@ -685,12 +684,24 @@ search_line_fast (const uchar *s, const uchar *end ATTRIBUTE_UNUSED)
>         v = vorrq_u8 (t, vceqq_u8 (data, repl_bs));
>         w = vorrq_u8 (u, vceqq_u8 (data, repl_qm));
>         t = vandq_u8 (vorrq_u8 (v, w), xmask);
> -      l = vpadd_u8 (vget_low_u8 (t), vget_high_u8 (t));
> -      m = vpaddl_u8 (l);
> -      n = vpaddl_u16 (m);
> -
> -      found = vget_lane_u32 ((uint32x2_t) vorr_u64 ((uint64x1_t) n,
> -	      vshr_n_u64 ((uint64x1_t) n, 24)), 0);
> +
> +      l = vpaddlq_u8 (t);
> +      l = vshlq_u16 (l, shift);
> +
> +      /* ??? Ideally, this would be if (__ARM_ARCH >= 8) since the ADDV insn
> +	 reduces the instruction count by two.  But vaddvq is not present in
> +	 the arm32 arm_neon.h, nor does AArch64 define __ARM_ARCH.  */
> +#ifdef __aarch64__
> +      found = vaddvq_u16 (l);
> +#else
> +      {
> +	uint32x4_t m = vpaddlq_u16 (l);
> +	uint64x2_t n = vpaddlq_u32 (m);
> +	uint64x1_t o = vget_low_u64 (n) + vget_high_u64 (n);
> +	found = vget_lane_u32 ((uint32x2_t)o, 0);
> +      }
> +#endif
> +
>         found &= mask;
>       }
>     while (!found);
>