[PATCH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[PATCH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Andrea Corallo]

[-- Attachment #1: Type: text/plain, Size: 1244 bytes --]

Hi all,

when _Float16 in native precision is supported GCC defines
__FLT_EVAL_METHOD__ to 16 as per ISO/IEC TS 18661-3.

Ex:

$ ./arm-none-eabi-gcc -mthumb -march=armv8.1-m.main+fp  -mfloat-abi=softfp -xc /dev/null -E -dM | grep FLT_EVAL
#define __FLT_EVAL_METHOD__ 16
#define __FLT_EVAL_METHOD_TS_18661_3__ 16

Unfortunately libc/include/math.h understands only values 0, 1 and 2
leading to have newlib non compilable for those configurations.

I think in newlib we should either:

1- Handle __FLT_EVAL_METHOD__ == 16
2- Build in a C mode which does not enable _Float16 (ex -std=c11)
3- Build without the _FloatN extensions -fpermitted-flt-eval-methods=c11

I think 1 should be the favorite approach.

ISO/IEC 9899:201x [1] suggests that for other values of FLT_EVAL_METHOD
other than 0, 1, 2 the definition of float_t and double_t is
implementation-defined.

The suggested patch implements that for arm and Aarch64.

Note, some previous discussion on this topic can be found here [2] as
follow-up to a patch addressing the same issue (only for the
case__FLT_EVAL_METHOD__ == 16 case).

Best Regards

  Andrea

[1] <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf>
[2] <https://sourceware.org/legacy-ml/newlib/2019/msg00597.html>



[-- Attachment #2: 0001-PATH-Aarch32-64-Support-__FLT_EVAL_METHOD__-values-o.patch --]
[-- Type: text/plain, Size: 1923 bytes --]

From d4c38a902e7bd5a2686d4d758c5cd563069c5bb2 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Wed, 30 Mar 2022 15:40:59 +0200
Subject: [PATCH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other
 than 0, 1, 2

2022-03-30  Andrea Corallo  <andrea.corallo@arm.com>

	* libc/include/machine/ieeefp.h (__FLOAT_TYPE, __DOUBLE_TYPE): New
	macros.
	* libc/include/math.h: Uses __DOUBLE_TYPE __FLOAT_TYPE to define
	double_t float_t if possible.
---
 newlib/libc/include/machine/ieeefp.h | 4 ++++
 newlib/libc/include/math.h           | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/newlib/libc/include/machine/ieeefp.h b/newlib/libc/include/machine/ieeefp.h
index 4dc13828c..c65c67769 100644
--- a/newlib/libc/include/machine/ieeefp.h
+++ b/newlib/libc/include/machine/ieeefp.h
@@ -90,6 +90,8 @@
 #ifndef __SOFTFP__
 # define _SUPPORTS_ERREXCEPT
 #endif
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif

 #if defined (__aarch64__)
@@ -102,6 +104,8 @@
 #ifdef __ARM_FP
 # define _SUPPORTS_ERREXCEPT
 #endif
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif

 #ifdef __epiphany__
diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
index ba1a8a17e..da056b5b6 100644
--- a/newlib/libc/include/math.h
+++ b/newlib/libc/include/math.h
@@ -158,6 +158,15 @@ extern int isnan (double);
    #else
     /* Implementation-defined.  Assume float_t and double_t have been
      * defined previously for this configuration (e.g. config.h). */
+
+   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
+      defined as well) float_t and double_t definition is suggested by
+      an arch specific header.  */
+   #ifdef __DOUBLE_TYPE
+    typedef __DOUBLE_TYPE double_t
+    typedef __FLOAT_TYPE float_t
+   #endif
+   /* Assume config.h has provided these types.  */
   #endif
 #else
     /* Assume basic definitions.  */
--
2.25.1

RE: [PATCH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Torbjorn SVENSSON]

Hello,

It would have been easier to review the patch if it was inline, but this will have to do anyway.
I think there is a typo in math.h. Aren't you supposed to do "#ifndef" and not "#ifdef"?

Kind regards,
Torbjörn


ST Restricted

-----Original Message-----
From: Newlib <newlib-bounces+torbjorn.svensson=st.com@sourceware.org> On Behalf Of Andrea Corallo
Sent: den 4 april 2022 10:39
To: newlib@sourceware.org
Cc: nd@arm.com
Subject: [PATCH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

Hi all,

when _Float16 in native precision is supported GCC defines
__FLT_EVAL_METHOD__ to 16 as per ISO/IEC TS 18661-3.

Ex:

$ ./arm-none-eabi-gcc -mthumb -march=armv8.1-m.main+fp  -mfloat-abi=softfp -xc /dev/null -E -dM | grep FLT_EVAL
#define __FLT_EVAL_METHOD__ 16
#define __FLT_EVAL_METHOD_TS_18661_3__ 16

Unfortunately libc/include/math.h understands only values 0, 1 and 2
leading to have newlib non compilable for those configurations.

I think in newlib we should either:

1- Handle __FLT_EVAL_METHOD__ == 16
2- Build in a C mode which does not enable _Float16 (ex -std=c11)
3- Build without the _FloatN extensions -fpermitted-flt-eval-methods=c11

I think 1 should be the favorite approach.

ISO/IEC 9899:201x [1] suggests that for other values of FLT_EVAL_METHOD
other than 0, 1, 2 the definition of float_t and double_t is
implementation-defined.

The suggested patch implements that for arm and Aarch64.

Note, some previous discussion on this topic can be found here [2] as
follow-up to a patch addressing the same issue (only for the
case__FLT_EVAL_METHOD__ == 16 case).

Best Regards

  Andrea

[1] <http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf>
[2] <https://sourceware.org/legacy-ml/newlib/2019/msg00597.html>

Re: [PATCH V2] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Andrea Corallo]

[-- Attachment #1: Type: text/plain, Size: 1688 bytes --]

Torbjorn SVENSSON <torbjorn.svensson@st.com> writes:

> Hello,
>
> It would have been easier to review the patch if it was inline, but
> this will have to do anyway.

Hi Torbjorn,

sorry most mail readers easily show inline attacchaments of type
"text/plain" allowing for inline reply, at the same time this way they
can still retain the notion of attached file.  This is how I rutinary
sent my patches to other GNU projects (including GCC) so far.  Has
newlib some specific rule around this?

> I think there is a typo in math.h. Aren't you supposed to do "#ifndef" and not "#ifdef"?

I guess we are talking about this hunk?

 #ifdef __epiphany__
diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
index ba1a8a17e..da056b5b6 100644
--- a/newlib/libc/include/math.h
+++ b/newlib/libc/include/math.h
@@ -158,6 +158,15 @@ extern int isnan (double);
    #else
     /* Implementation-defined.  Assume float_t and double_t have been
      * defined previously for this configuration (e.g. config.h). */
+
+   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
+      defined as well) float_t and double_t definition is suggested by
+      an arch specific header.  */
+   #ifdef __DOUBLE_TYPE
+    typedef __DOUBLE_TYPE double_t;
+    typedef __FLOAT_TYPE float_t;
+   #endif
+   /* Assume config.h has provided these types.  */
   #endif
 #else
     /* Assume basic definitions.  */

I believe the #ifdef is correct.  As the comment suggests if
__DOUBLE_TYPE is defined we'll use it to define double_t otherwise we
assume is config.h has provided the type definition.

I'm reattaching the latest version of this patch with a typo fixed.

Thanks!

  Andrea


[-- Attachment #2: 0001-PATH-Aarch32-64-Support-__FLT_EVAL_METHOD__-values-o.patch --]
[-- Type: text/plain, Size: 1936 bytes --]

From d4c7eab60765bc51b97915744a5fec17b9499400 Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Wed, 30 Mar 2022 15:40:59 +0200
Subject: [PATCH] [PATH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other
 than 0, 1, 2

2022-03-30  Andrea Corallo  <andrea.corallo@arm.com>

	* libc/include/machine/ieeefp.h (__FLOAT_TYPE, __DOUBLE_TYPE): New
	macros.
	* libc/include/math.h: Uses __DOUBLE_TYPE __FLOAT_TYPE to define
	double_t float_t if possible.
---
 newlib/libc/include/machine/ieeefp.h | 4 ++++
 newlib/libc/include/math.h           | 9 +++++++++
 2 files changed, 13 insertions(+)

diff --git a/newlib/libc/include/machine/ieeefp.h b/newlib/libc/include/machine/ieeefp.h
index 4dc13828c..c65c67769 100644
--- a/newlib/libc/include/machine/ieeefp.h
+++ b/newlib/libc/include/machine/ieeefp.h
@@ -90,6 +90,8 @@
 #ifndef __SOFTFP__
 # define _SUPPORTS_ERREXCEPT
 #endif
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif
 
 #if defined (__aarch64__)
@@ -102,6 +104,8 @@
 #ifdef __ARM_FP
 # define _SUPPORTS_ERREXCEPT
 #endif
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif
 
 #ifdef __epiphany__
diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
index ba1a8a17e..af5b5e653 100644
--- a/newlib/libc/include/math.h
+++ b/newlib/libc/include/math.h
@@ -158,6 +158,15 @@ extern int isnan (double);
    #else
     /* Implementation-defined.  Assume float_t and double_t have been
      * defined previously for this configuration (e.g. config.h). */
+
+   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
+      defined as well) float_t and double_t definition is suggested by
+      an arch specific header.  */
+   #ifdef __DOUBLE_TYPE
+    typedef __DOUBLE_TYPE double_t;
+    typedef __FLOAT_TYPE float_t;
+   #endif
+   /* Assume config.h has provided these types.  */
   #endif
 #else
     /* Assume basic definitions.  */
-- 
2.25.1

RE: [PATCH V2] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Torbjorn SVENSSON]

Hello again,

Yes that was the chunk I meant.
Apparently, I did not read the code close enough.  Now when I go back and look at it again, I suppose it's fine.
Sorry for the noise!

Kind regards,
Torbjörn


ST Restricted

-----Original Message-----
From: Andrea Corallo <andrea.corallo@arm.com> 
Sent: den 4 april 2022 16:53
To: Torbjorn SVENSSON <torbjorn.svensson@st.com>
Cc: newlib@sourceware.org; nd@arm.com
Subject: Re: [PATCH V2] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

Torbjorn SVENSSON <torbjorn.svensson@st.com> writes:

> Hello,
>
> It would have been easier to review the patch if it was inline, but
> this will have to do anyway.

Hi Torbjorn,

sorry most mail readers easily show inline attacchaments of type
"text/plain" allowing for inline reply, at the same time this way they
can still retain the notion of attached file.  This is how I rutinary
sent my patches to other GNU projects (including GCC) so far.  Has
newlib some specific rule around this?

> I think there is a typo in math.h. Aren't you supposed to do "#ifndef" and not "#ifdef"?

I guess we are talking about this hunk?

 #ifdef __epiphany__
diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
index ba1a8a17e..da056b5b6 100644
--- a/newlib/libc/include/math.h
+++ b/newlib/libc/include/math.h
@@ -158,6 +158,15 @@ extern int isnan (double);
    #else
     /* Implementation-defined.  Assume float_t and double_t have been
      * defined previously for this configuration (e.g. config.h). */
+
+   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
+      defined as well) float_t and double_t definition is suggested by
+      an arch specific header.  */
+   #ifdef __DOUBLE_TYPE
+    typedef __DOUBLE_TYPE double_t;
+    typedef __FLOAT_TYPE float_t;
+   #endif
+   /* Assume config.h has provided these types.  */
   #endif
 #else
     /* Assume basic definitions.  */

I believe the #ifdef is correct.  As the comment suggests if
__DOUBLE_TYPE is defined we'll use it to define double_t otherwise we
assume is config.h has provided the type definition.

I'm reattaching the latest version of this patch with a typo fixed.

Thanks!

  Andrea

Re: [PATCH V2] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Richard Earnshaw]

On 04/04/2022 15:53, Andrea Corallo wrote:
> Torbjorn SVENSSON <torbjorn.svensson@st.com> writes:
> 
>> Hello,
>>
>> It would have been easier to review the patch if it was inline, but
>> this will have to do anyway.
> 
> Hi Torbjorn,
> 
> sorry most mail readers easily show inline attacchaments of type
> "text/plain" allowing for inline reply, at the same time this way they
> can still retain the notion of attached file.  This is how I rutinary
> sent my patches to other GNU projects (including GCC) so far.  Has
> newlib some specific rule around this?
> 
>> I think there is a typo in math.h. Aren't you supposed to do "#ifndef" and not "#ifdef"?
> 
> I guess we are talking about this hunk?
> 
>   #ifdef __epiphany__
> diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
> index ba1a8a17e..da056b5b6 100644
> --- a/newlib/libc/include/math.h
> +++ b/newlib/libc/include/math.h
> @@ -158,6 +158,15 @@ extern int isnan (double);
>      #else
>       /* Implementation-defined.  Assume float_t and double_t have been
>        * defined previously for this configuration (e.g. config.h). */
> +
> +   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
> +      defined as well) float_t and double_t definition is suggested by
> +      an arch specific header.  */
> +   #ifdef __DOUBLE_TYPE
> +    typedef __DOUBLE_TYPE double_t;
> +    typedef __FLOAT_TYPE float_t;
> +   #endif
> +   /* Assume config.h has provided these types.  */
>     #endif
>   #else
>       /* Assume basic definitions.  */
> 
> I believe the #ifdef is correct.  As the comment suggests if
> __DOUBLE_TYPE is defined we'll use it to define double_t otherwise we
> assume is config.h has provided the type definition.
> 
> I'm reattaching the latest version of this patch with a typo fixed.
> 
> Thanks!
> 
>    Andrea
> 

I think the hunks in machine/ieeefp.h warrant a comment as to why we 
can't rely on __FLT_EVAL_METHOD__.  Other than that it LGTM, but you'll 
need Corinna or Jeff to approve.

R.

Re: [PATCH V2] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Jeff Johnston]

On Fri, Apr 8, 2022 at 6:09 AM Richard Earnshaw <
Richard.Earnshaw@foss.arm.com> wrote:

>
>
> On 04/04/2022 15:53, Andrea Corallo wrote:
> > Torbjorn SVENSSON <torbjorn.svensson@st.com> writes:
> >
> >> Hello,
> >>
> >> It would have been easier to review the patch if it was inline, but
> >> this will have to do anyway.
> >
> > Hi Torbjorn,
> >
> > sorry most mail readers easily show inline attacchaments of type
> > "text/plain" allowing for inline reply, at the same time this way they
> > can still retain the notion of attached file.  This is how I rutinary
> > sent my patches to other GNU projects (including GCC) so far.  Has
> > newlib some specific rule around this?
> >
> >> I think there is a typo in math.h. Aren't you supposed to do "#ifndef"
> and not "#ifdef"?
> >
> > I guess we are talking about this hunk?
> >
> >   #ifdef __epiphany__
> > diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
> > index ba1a8a17e..da056b5b6 100644
> > --- a/newlib/libc/include/math.h
> > +++ b/newlib/libc/include/math.h
> > @@ -158,6 +158,15 @@ extern int isnan (double);
> >      #else
> >       /* Implementation-defined.  Assume float_t and double_t have been
> >        * defined previously for this configuration (e.g. config.h). */
> > +
> > +   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
> > +      defined as well) float_t and double_t definition is suggested by
> > +      an arch specific header.  */
> > +   #ifdef __DOUBLE_TYPE
> > +    typedef __DOUBLE_TYPE double_t;
> > +    typedef __FLOAT_TYPE float_t;
> > +   #endif
> > +   /* Assume config.h has provided these types.  */
> >     #endif
> >   #else
> >       /* Assume basic definitions.  */
> >
> > I believe the #ifdef is correct.  As the comment suggests if
> > __DOUBLE_TYPE is defined we'll use it to define double_t otherwise we
> > assume is config.h has provided the type definition.
> >
> > I'm reattaching the latest version of this patch with a typo fixed.
> >
> > Thanks!
> >
> >    Andrea
> >
>
> I think the hunks in machine/ieeefp.h warrant a comment as to why we
> can't rely on __FLT_EVAL_METHOD__.  Other than that it LGTM, but you'll
> need Corinna or Jeff to approve.
>
> R.
>
>
Please add the comment as suggested by Richard and it will be pushed.

-- Jeff J.

[PATCH V3] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Andrea Corallo]

[-- Attachment #1: Type: text/plain, Size: 618 bytes --]

Jeff Johnston <jjohnstn@redhat.com> writes:

>> On Fri, Apr 8, 2022 at 6:09 AM Richard Earnshaw <mailto:Richard.Earnshaw@foss.arm.com> wrote:
>>
>> I think the hunks in machine/ieeefp.h warrant a comment as to why we 
>> can't rely on __FLT_EVAL_METHOD__.  Other than that it LGTM, but you'll 
>> need Corinna or Jeff to approve.
>
> R.
> Please add the comment as suggested by Richard and it will be pushed.-- Jeff J.

Hi all,

please find attached the updated version of the patch implementing
Richard's suggestion.

Best Regards

  Andrea

PS I've no write access to newlib's repo so feel free to push it if
okay.


[-- Attachment #2: 0001-PATH-Aarch32-64-Support-__FLT_EVAL_METHOD__-values-o.patch --]
[-- Type: text/plain, Size: 2570 bytes --]

From a70d1a11c53d6d6797c5df859dea5b1e584fa9ac Mon Sep 17 00:00:00 2001
From: Andrea Corallo <andrea.corallo@arm.com>
Date: Wed, 30 Mar 2022 15:40:59 +0200
Subject: [PATCH] [PATH] Aarch32/64: Support __FLT_EVAL_METHOD__ values other
 than 0, 1, 2

2022-03-30  Andrea Corallo  <andrea.corallo@arm.com>

	* libc/include/machine/ieeefp.h (__FLOAT_TYPE, __DOUBLE_TYPE): New
	macros.
	* libc/include/math.h: Uses __DOUBLE_TYPE __FLOAT_TYPE to define
	double_t float_t if possible.
---
 newlib/libc/include/machine/ieeefp.h | 14 ++++++++++++++
 newlib/libc/include/math.h           |  9 +++++++++
 2 files changed, 23 insertions(+)

diff --git a/newlib/libc/include/machine/ieeefp.h b/newlib/libc/include/machine/ieeefp.h
index 4dc13828c..1e0680432 100644
--- a/newlib/libc/include/machine/ieeefp.h
+++ b/newlib/libc/include/machine/ieeefp.h
@@ -90,6 +90,13 @@
 #ifndef __SOFTFP__
 # define _SUPPORTS_ERREXCEPT
 #endif
+/* As per ISO/IEC TS 18661 '__FLT_EVAL_METHOD__' will be defined to 16
+   (if compiling with +fp16 support) so it can't be used by math.h to
+   define float_t and double_t.  For values of '__FLT_EVAL_METHOD__'
+   other than 0, 1, 2 the definition of float_t and double_t is
+   implementation-defined.  */
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif
 
 #if defined (__aarch64__)
@@ -102,6 +109,13 @@
 #ifdef __ARM_FP
 # define _SUPPORTS_ERREXCEPT
 #endif
+/* As per ISO/IEC TS 18661 '__FLT_EVAL_METHOD__' will be defined to 16
+   (if compiling with +fp16 support) so it can't be used by math.h to
+   define float_t and double_t.  For values of '__FLT_EVAL_METHOD__'
+   other than 0, 1, 2 the definition of float_t and double_t is
+   implementation-defined.  */
+#define __DOUBLE_TYPE double
+#define __FLOAT_TYPE float
 #endif
 
 #ifdef __epiphany__
diff --git a/newlib/libc/include/math.h b/newlib/libc/include/math.h
index ba1a8a17e..af5b5e653 100644
--- a/newlib/libc/include/math.h
+++ b/newlib/libc/include/math.h
@@ -158,6 +158,15 @@ extern int isnan (double);
    #else
     /* Implementation-defined.  Assume float_t and double_t have been
      * defined previously for this configuration (e.g. config.h). */
+
+   /* If __DOUBLE_TYPE is defined (__FLOAT_TYPE is then supposed to be
+      defined as well) float_t and double_t definition is suggested by
+      an arch specific header.  */
+   #ifdef __DOUBLE_TYPE
+    typedef __DOUBLE_TYPE double_t;
+    typedef __FLOAT_TYPE float_t;
+   #endif
+   /* Assume config.h has provided these types.  */
   #endif
 #else
     /* Assume basic definitions.  */
-- 
2.25.1

Re: [PATCH V3] Aarch32/64: Support __FLT_EVAL_METHOD__ values other than 0, 1, 2

[Jeff Johnston]

Thanks Andrea.  Patch pushed to master.

-- Jeff J.

On Mon, Apr 11, 2022 at 9:52 AM Andrea Corallo <andrea.corallo@arm.com>
wrote:

> Jeff Johnston <jjohnstn@redhat.com> writes:
>
> >> On Fri, Apr 8, 2022 at 6:09 AM Richard Earnshaw <mailto:
> Richard.Earnshaw@foss.arm.com> wrote:
> >>
> >> I think the hunks in machine/ieeefp.h warrant a comment as to why we
> >> can't rely on __FLT_EVAL_METHOD__.  Other than that it LGTM, but you'll
> >> need Corinna or Jeff to approve.
> >
> > R.
> > Please add the comment as suggested by Richard and it will be pushed.--
> Jeff J.
>
> Hi all,
>
> please find attached the updated version of the patch implementing
> Richard's suggestion.
>
> Best Regards
>
>   Andrea
>
> PS I've no write access to newlib's repo so feel free to push it if
> okay.
>
>