diff mbox series

[RFC] arm64/math-vec.h: guard off the vector types with CPP constants

Message ID 20230927141839.57421-1-simon.chopin@canonical.com
State Dropped
Headers
Series [RFC] arm64/math-vec.h: guard off the vector types with CPP constants |

Checks

Context Check Description
redhat-pt-bot/TryBot-apply_patch success Patch applied to master at the time it was sent
linaro-tcwg-bot/tcwg_glibc_build--master-arm success Testing passed
redhat-pt-bot/TryBot-32bit success Build for i686
linaro-tcwg-bot/tcwg_glibc_build--master-aarch64 success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-arm success Testing passed
linaro-tcwg-bot/tcwg_glibc_check--master-aarch64 success Testing passed

Commit Message

Simon Chopin Sept. 27, 2023, 2:18 p.m. UTC 
  When implementing a C parser, it's apparently common to use GCC as
preprocessor. However, those programs don't necessarily define the SIMD
intrinsic types exposed by GCC, resulting in failed compilations.

This patch adds a way for those users to bypass entirely the vector
types, as they usually aren't interested in libmvec anyway. They can
just add -D__ARM_VEC_MATH_DISABLED=1 to the CPP_FLAGS they pass on to
GCC.

Fixes: BZ #25422

Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
---
 sysdeps/aarch64/fpu/bits/math-vector.h | 32 ++++++++++++++------------
 1 file changed, 17 insertions(+), 15 deletions(-)


base-commit: 64b1a44183a3094672ed304532bedb9acc707554

Comments

Szabolcs Nagy Sept. 28, 2023, 2:45 p.m. UTC | #1 
The 09/27/2023 16:18, Simon Chopin wrote:
> When implementing a C parser, it's apparently common to use GCC as
> preprocessor. However, those programs don't necessarily define the SIMD
> intrinsic types exposed by GCC, resulting in failed compilations.

i think that's the fault of those parsers, they would also fail
on any new gcc feature that is gated with gcc version checks
or e.g. on include <arm-neon.h>.

> 
> This patch adds a way for those users to bypass entirely the vector
> types, as they usually aren't interested in libmvec anyway. They can
> just add -D__ARM_VEC_MATH_DISABLED=1 to the CPP_FLAGS they pass on to
> GCC.

we can add an ifdef, but don't use the __ARM namespace as those
are for macros documented by the arm c language extensions, not
workarounds for gnu tools users.

i'd probably try gcc -E -D__GNUC__=4 because who knows what other
gcc 9+ feature is missing. (but yes this is a nasty hack)

we can try something like

#ifndef __MISSING_VECTOR_TYPE

> 
> Fixes: BZ #25422

this is a different issue. (maybe we should close it now
although the autovec declaration stuff is missing)

> 
> Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
> ---
>  sysdeps/aarch64/fpu/bits/math-vector.h | 32 ++++++++++++++------------
>  1 file changed, 17 insertions(+), 15 deletions(-)
> 
> diff --git a/sysdeps/aarch64/fpu/bits/math-vector.h b/sysdeps/aarch64/fpu/bits/math-vector.h
> index 7c200599c1..c739e6bc5d 100644
> --- a/sysdeps/aarch64/fpu/bits/math-vector.h
> +++ b/sysdeps/aarch64/fpu/bits/math-vector.h
> @@ -25,29 +25,30 @@
>  /* Get default empty definitions for simd declarations.  */
>  #include <bits/libm-simd-decl-stubs.h>
>  
> -#if __GNUC_PREREQ(9, 0)
> -#  define __ADVSIMD_VEC_MATH_SUPPORTED
> +#if defined __ARM_ARCH_8A && !defined __ARM_VEC_MATH_DISABLED

where did the __ARM_ARCH_8A check came from? it's wrong here.
(i think that's a 32bit arm macro that the aarch64 preprocessor
only defines for interop reasons and nobody should use it.)

this header is only installed for a compiler targetting aarch64,
what are you trying to test? 64bit isa?

> +#  if __GNUC_PREREQ(9, 0)
> +#    define __ADVSIMD_VEC_MATH_SUPPORTED
>  typedef __Float32x4_t __f32x4_t;
>  typedef __Float64x2_t __f64x2_t;
> -#elif __glibc_clang_prereq(8, 0)
> -#  define __ADVSIMD_VEC_MATH_SUPPORTED
> +#  elif __glibc_clang_prereq(8, 0)
> +#    define __ADVSIMD_VEC_MATH_SUPPORTED
>  typedef __attribute__ ((__neon_vector_type__ (4))) float __f32x4_t;
>  typedef __attribute__ ((__neon_vector_type__ (2))) double __f64x2_t;
> -#endif
> +#  endif
>  
> -#if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
> -#  define __SVE_VEC_MATH_SUPPORTED
> +#  if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
> +#    define __SVE_VEC_MATH_SUPPORTED
>  typedef __SVFloat32_t __sv_f32_t;
>  typedef __SVFloat64_t __sv_f64_t;
>  typedef __SVBool_t __sv_bool_t;
> -#endif
> +#  endif
>  
>  /* If vector types and vector PCS are unsupported in the working
>     compiler, no choice but to omit vector math declarations.  */
>  
> -#ifdef __ADVSIMD_VEC_MATH_SUPPORTED
> +#  ifdef __ADVSIMD_VEC_MATH_SUPPORTED
>  
> -#  define __vpcs __attribute__ ((__aarch64_vector_pcs__))
> +#    define __vpcs __attribute__ ((__aarch64_vector_pcs__))
>  
>  __vpcs __f32x4_t _ZGVnN4v_cosf (__f32x4_t);
>  __vpcs __f32x4_t _ZGVnN4v_expf (__f32x4_t);
> @@ -59,10 +60,10 @@ __vpcs __f64x2_t _ZGVnN2v_exp (__f64x2_t);
>  __vpcs __f64x2_t _ZGVnN2v_log (__f64x2_t);
>  __vpcs __f64x2_t _ZGVnN2v_sin (__f64x2_t);
>  
> -#  undef __ADVSIMD_VEC_MATH_SUPPORTED
> -#endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
> +#    undef __ADVSIMD_VEC_MATH_SUPPORTED
> +#  endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
>  
> -#ifdef __SVE_VEC_MATH_SUPPORTED
> +#  ifdef __SVE_VEC_MATH_SUPPORTED
>  
>  __sv_f32_t _ZGVsMxv_cosf (__sv_f32_t, __sv_bool_t);
>  __sv_f32_t _ZGVsMxv_expf (__sv_f32_t, __sv_bool_t);
> @@ -74,5 +75,6 @@ __sv_f64_t _ZGVsMxv_exp (__sv_f64_t, __sv_bool_t);
>  __sv_f64_t _ZGVsMxv_log (__sv_f64_t, __sv_bool_t);
>  __sv_f64_t _ZGVsMxv_sin (__sv_f64_t, __sv_bool_t);
>  
> -#  undef __SVE_VEC_MATH_SUPPORTED
> -#endif /* __SVE_VEC_MATH_SUPPORTED */
> +#    undef __SVE_VEC_MATH_SUPPORTED
> +#  endif /* __SVE_VEC_MATH_SUPPORTED */
> +#endif /* _ARM_ARCH_8A && !__ARM_VEC_MATH_DISABLED */
> 
> base-commit: 64b1a44183a3094672ed304532bedb9acc707554
> -- 
> 2.40.1
>
Adhemerval Zanella Netto Sept. 28, 2023, 2:56 p.m. UTC | #2 
On 28/09/23 11:45, Szabolcs Nagy wrote:
> The 09/27/2023 16:18, Simon Chopin wrote:
>> When implementing a C parser, it's apparently common to use GCC as
>> preprocessor. However, those programs don't necessarily define the SIMD
>> intrinsic types exposed by GCC, resulting in failed compilations.
> 
> i think that's the fault of those parsers, they would also fail
> on any new gcc feature that is gated with gcc version checks
> or e.g. on include <arm-neon.h>.
> 
>>
>> This patch adds a way for those users to bypass entirely the vector
>> types, as they usually aren't interested in libmvec anyway. They can
>> just add -D__ARM_VEC_MATH_DISABLED=1 to the CPP_FLAGS they pass on to
>> GCC.
> 
> we can add an ifdef, but don't use the __ARM namespace as those
> are for macros documented by the arm c language extensions, not
> workarounds for gnu tools users.
> 
> i'd probably try gcc -E -D__GNUC__=4 because who knows what other
> gcc 9+ feature is missing. (but yes this is a nasty hack)
> 
> we can try something like
> 
> #ifndef __MISSING_VECTOR_TYPE


Maybe __GLIBC_NO_ARM_VECTOR_TYPE, to make it clear it comes from an 
installed glibc header.  By I agree with Szabolcs here, you really need 
to fix this C parser otherwise we will need to constantly add workarounds 
like this that only add extra complexity (and tend to be bitrotten or
forgotten once the C parse is fixed).

> 
>>
>> Fixes: BZ #25422
> 
> this is a different issue. (maybe we should close it now
> although the autovec declaration stuff is missing)
> 
>>
>> Signed-off-by: Simon Chopin <simon.chopin@canonical.com>
>> ---
>>  sysdeps/aarch64/fpu/bits/math-vector.h | 32 ++++++++++++++------------
>>  1 file changed, 17 insertions(+), 15 deletions(-)
>>
>> diff --git a/sysdeps/aarch64/fpu/bits/math-vector.h b/sysdeps/aarch64/fpu/bits/math-vector.h
>> index 7c200599c1..c739e6bc5d 100644
>> --- a/sysdeps/aarch64/fpu/bits/math-vector.h
>> +++ b/sysdeps/aarch64/fpu/bits/math-vector.h
>> @@ -25,29 +25,30 @@
>>  /* Get default empty definitions for simd declarations.  */
>>  #include <bits/libm-simd-decl-stubs.h>
>>  
>> -#if __GNUC_PREREQ(9, 0)
>> -#  define __ADVSIMD_VEC_MATH_SUPPORTED
>> +#if defined __ARM_ARCH_8A && !defined __ARM_VEC_MATH_DISABLED
> 
> where did the __ARM_ARCH_8A check came from? it's wrong here.
> (i think that's a 32bit arm macro that the aarch64 preprocessor
> only defines for interop reasons and nobody should use it.)
> 
> this header is only installed for a compiler targetting aarch64,
> what are you trying to test? 64bit isa?
> 
>> +#  if __GNUC_PREREQ(9, 0)
>> +#    define __ADVSIMD_VEC_MATH_SUPPORTED
>>  typedef __Float32x4_t __f32x4_t;
>>  typedef __Float64x2_t __f64x2_t;
>> -#elif __glibc_clang_prereq(8, 0)
>> -#  define __ADVSIMD_VEC_MATH_SUPPORTED
>> +#  elif __glibc_clang_prereq(8, 0)
>> +#    define __ADVSIMD_VEC_MATH_SUPPORTED
>>  typedef __attribute__ ((__neon_vector_type__ (4))) float __f32x4_t;
>>  typedef __attribute__ ((__neon_vector_type__ (2))) double __f64x2_t;
>> -#endif
>> +#  endif
>>  
>> -#if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
>> -#  define __SVE_VEC_MATH_SUPPORTED
>> +#  if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
>> +#    define __SVE_VEC_MATH_SUPPORTED
>>  typedef __SVFloat32_t __sv_f32_t;
>>  typedef __SVFloat64_t __sv_f64_t;
>>  typedef __SVBool_t __sv_bool_t;
>> -#endif
>> +#  endif
>>  
>>  /* If vector types and vector PCS are unsupported in the working
>>     compiler, no choice but to omit vector math declarations.  */
>>  
>> -#ifdef __ADVSIMD_VEC_MATH_SUPPORTED
>> +#  ifdef __ADVSIMD_VEC_MATH_SUPPORTED
>>  
>> -#  define __vpcs __attribute__ ((__aarch64_vector_pcs__))
>> +#    define __vpcs __attribute__ ((__aarch64_vector_pcs__))
>>  
>>  __vpcs __f32x4_t _ZGVnN4v_cosf (__f32x4_t);
>>  __vpcs __f32x4_t _ZGVnN4v_expf (__f32x4_t);
>> @@ -59,10 +60,10 @@ __vpcs __f64x2_t _ZGVnN2v_exp (__f64x2_t);
>>  __vpcs __f64x2_t _ZGVnN2v_log (__f64x2_t);
>>  __vpcs __f64x2_t _ZGVnN2v_sin (__f64x2_t);
>>  
>> -#  undef __ADVSIMD_VEC_MATH_SUPPORTED
>> -#endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
>> +#    undef __ADVSIMD_VEC_MATH_SUPPORTED
>> +#  endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
>>  
>> -#ifdef __SVE_VEC_MATH_SUPPORTED
>> +#  ifdef __SVE_VEC_MATH_SUPPORTED
>>  
>>  __sv_f32_t _ZGVsMxv_cosf (__sv_f32_t, __sv_bool_t);
>>  __sv_f32_t _ZGVsMxv_expf (__sv_f32_t, __sv_bool_t);
>> @@ -74,5 +75,6 @@ __sv_f64_t _ZGVsMxv_exp (__sv_f64_t, __sv_bool_t);
>>  __sv_f64_t _ZGVsMxv_log (__sv_f64_t, __sv_bool_t);
>>  __sv_f64_t _ZGVsMxv_sin (__sv_f64_t, __sv_bool_t);
>>  
>> -#  undef __SVE_VEC_MATH_SUPPORTED
>> -#endif /* __SVE_VEC_MATH_SUPPORTED */
>> +#    undef __SVE_VEC_MATH_SUPPORTED
>> +#  endif /* __SVE_VEC_MATH_SUPPORTED */
>> +#endif /* _ARM_ARCH_8A && !__ARM_VEC_MATH_DISABLED */
>>
>> base-commit: 64b1a44183a3094672ed304532bedb9acc707554
>> -- 
>> 2.40.1
>>
Florian Weimer Sept. 28, 2023, 7:10 p.m. UTC | #3 
* Szabolcs Nagy:

> i'd probably try gcc -E -D__GNUC__=4 because who knows what other
> gcc 9+ feature is missing. (but yes this is a nasty hack)

The side effect of this is that the resulting code can no longer be fed
to GCC itself.  But this may not be a problem in this particular case.

But I agree with the general direction of this thread: compilers should
not pretend to implement recent GCC versions if they do not.

Thanks,
Florian
Simon Chopin Oct. 5, 2023, 10:54 a.m. UTC | #4 
Quoting Florian Weimer (2023-09-28 21:10:21)
> * Szabolcs Nagy:
>
> > i'd probably try gcc -E -D__GNUC__=4 because who knows what other
> > gcc 9+ feature is missing. (but yes this is a nasty hack)
>
> The side effect of this is that the resulting code can no longer be fed
> to GCC itself.  But this may not be a problem in this particular case.

It might still be a problem in some cases, but if it can help in even
half the cases I'll take it :)

> But I agree with the general direction of this thread: compilers should
> not pretend to implement recent GCC versions if they do not.

That's fair enough. Consider this dropped.

Thank you all for your input!
diff mbox series

Patch

diff --git a/sysdeps/aarch64/fpu/bits/math-vector.h b/sysdeps/aarch64/fpu/bits/math-vector.h
index 7c200599c1..c739e6bc5d 100644
--- a/sysdeps/aarch64/fpu/bits/math-vector.h
+++ b/sysdeps/aarch64/fpu/bits/math-vector.h
@@ -25,29 +25,30 @@ 
 /* Get default empty definitions for simd declarations.  */
 #include <bits/libm-simd-decl-stubs.h>
 
-#if __GNUC_PREREQ(9, 0)
-#  define __ADVSIMD_VEC_MATH_SUPPORTED
+#if defined __ARM_ARCH_8A && !defined __ARM_VEC_MATH_DISABLED
+#  if __GNUC_PREREQ(9, 0)
+#    define __ADVSIMD_VEC_MATH_SUPPORTED
 typedef __Float32x4_t __f32x4_t;
 typedef __Float64x2_t __f64x2_t;
-#elif __glibc_clang_prereq(8, 0)
-#  define __ADVSIMD_VEC_MATH_SUPPORTED
+#  elif __glibc_clang_prereq(8, 0)
+#    define __ADVSIMD_VEC_MATH_SUPPORTED
 typedef __attribute__ ((__neon_vector_type__ (4))) float __f32x4_t;
 typedef __attribute__ ((__neon_vector_type__ (2))) double __f64x2_t;
-#endif
+#  endif
 
-#if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
-#  define __SVE_VEC_MATH_SUPPORTED
+#  if __GNUC_PREREQ(10, 0) || __glibc_clang_prereq(11, 0)
+#    define __SVE_VEC_MATH_SUPPORTED
 typedef __SVFloat32_t __sv_f32_t;
 typedef __SVFloat64_t __sv_f64_t;
 typedef __SVBool_t __sv_bool_t;
-#endif
+#  endif
 
 /* If vector types and vector PCS are unsupported in the working
    compiler, no choice but to omit vector math declarations.  */
 
-#ifdef __ADVSIMD_VEC_MATH_SUPPORTED
+#  ifdef __ADVSIMD_VEC_MATH_SUPPORTED
 
-#  define __vpcs __attribute__ ((__aarch64_vector_pcs__))
+#    define __vpcs __attribute__ ((__aarch64_vector_pcs__))
 
 __vpcs __f32x4_t _ZGVnN4v_cosf (__f32x4_t);
 __vpcs __f32x4_t _ZGVnN4v_expf (__f32x4_t);
@@ -59,10 +60,10 @@  __vpcs __f64x2_t _ZGVnN2v_exp (__f64x2_t);
 __vpcs __f64x2_t _ZGVnN2v_log (__f64x2_t);
 __vpcs __f64x2_t _ZGVnN2v_sin (__f64x2_t);
 
-#  undef __ADVSIMD_VEC_MATH_SUPPORTED
-#endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
+#    undef __ADVSIMD_VEC_MATH_SUPPORTED
+#  endif /* __ADVSIMD_VEC_MATH_SUPPORTED */
 
-#ifdef __SVE_VEC_MATH_SUPPORTED
+#  ifdef __SVE_VEC_MATH_SUPPORTED
 
 __sv_f32_t _ZGVsMxv_cosf (__sv_f32_t, __sv_bool_t);
 __sv_f32_t _ZGVsMxv_expf (__sv_f32_t, __sv_bool_t);
@@ -74,5 +75,6 @@  __sv_f64_t _ZGVsMxv_exp (__sv_f64_t, __sv_bool_t);
 __sv_f64_t _ZGVsMxv_log (__sv_f64_t, __sv_bool_t);
 __sv_f64_t _ZGVsMxv_sin (__sv_f64_t, __sv_bool_t);
 
-#  undef __SVE_VEC_MATH_SUPPORTED
-#endif /* __SVE_VEC_MATH_SUPPORTED */
+#    undef __SVE_VEC_MATH_SUPPORTED
+#  endif /* __SVE_VEC_MATH_SUPPORTED */
+#endif /* _ARM_ARCH_8A && !__ARM_VEC_MATH_DISABLED */