Re: [PATCH][GCC][AArch64] Vectorise __builtin_signbit on aarch64

[Richard Sandiford]

Przemyslaw Wirkus <przemyslaw.wir...@arm.com> writes:
> Hi Richard,
> New patch adds a new IFN_SIGNBIT internal function that maps
> to signbit_optab.

Thanks.

> gcc/ChangeLog:
>
> 2019-05-05  Przemyslaw Wirkus  <przemyslaw.wir...@arm.com>
>
>       * gcc/internal-fn.def (SIGNBIT): New.
>       * gcc/config/aarch64/aarch64-simd.md (signbitv4sf2): New expand
>       defined.

Sorry for the nitpicks (I'm not really a fan of ChangeLogs), but:
the filenames are relative to the changelog file, so no "gcc/" here and

> gcc/testsuite/ChangeLog:
>
> 2019-05-05  Przemyslaw Wirkus  <przemyslaw.wir...@arm.com>
>
>       * gcc/testsuite/gcc.target/aarch64/signbitv4sf.c: New test.

no "gcc/testsuite/" here.

> diff --git a/gcc/config/aarch64/aarch64-simd.md 
> b/gcc/config/aarch64/aarch64-simd.md
> index 
> e3852c5d182b70978d7603225fce55c0b8ee2894..3374ce95b912cceaca49660df0579467f758974d
>  100644
> --- a/gcc/config/aarch64/aarch64-simd.md
> +++ b/gcc/config/aarch64/aarch64-simd.md
> @@ -935,6 +935,21 @@
>    [(set_attr "type" "neon_ins<q>")]
>  )
>  
> +(define_expand "signbitv4sf2"
> +  [(use (match_operand:V4SI 0 "register_operand"))
> +   (use (match_operand:V4SF 1 "register_operand"))]
> +  "TARGET_SIMD"
> +{
> +  int shift_amount = GET_MODE_UNIT_BITSIZE (V4SImode) - 1;
> +  rtx shift_vector = aarch64_simd_gen_const_vector_dup (V4SImode,
> +                          shift_amount);
> +  operands[1] = lowpart_subreg (V4SImode, operands[1], V4SFmode);
> +
> +  emit_insn (gen_aarch64_simd_lshrv4si (operands[0], operands[1],
> +                  shift_vector));

Formatting nit: argument should be indented to the column after the
innermost unclosed "(".

> +  DONE;
> +})
> +

Looks good, but I think it can be generalised to handle v2sf if you use:

- :VDQSF instead of :V4SF
- <MODE> instead of other instances of V4SF (and <mode> instead of v4sf)
- <V_INT_EQUIV> instead of V4SI (and <v_int_equiv> instead of v4si)

E.g. this will handle SLP instances like:

void
f (int *i, float *f)
{
  i[0] = __builtin_signbitf (f[0]);
  i[1] = __builtin_signbitf (f[1]);
}

It could also be used for epilogue loop vectorisation, if we ever
turn that on by default for AArch64.

Thanks,
Richard