diff mbox series

[1/2] aarch64: Add FMA and FMAF intrinsics and tests

Message ID Z83-CJmVXhUKHXH1@sawtooth
State Committed
Commit f4f7216c56fe2f67c72db5b7c4afa220725f3ed1
Headers
Series [1/2] aarch64: Add FMA and FMAF intrinsics and tests |

Commit Message

Ayan Shafqat March 9, 2025, 8:46 p.m. UTC 
  This patch introduces inline definitions for the __fma and __fmaf
functions in arm_acle.h for AArch64 targets. These definitions rely on
__builtin_fma and __builtin_fmaf to ensure proper inlining and to meet
the ACLE requirements [1].

The patch has been tested locally using a crosstool-NG sysroot for
AArch64, confirming that the generated code uses the expected fused
multiply-accumulate instructions (fmadd).

[1] https://arm-software.github.io/acle/main/acle.html#fused-multiply-accumulate-fma

Signed-off-by: Ayan Shafqat <ayan.x.shafqat@gmail.com>

---
 gcc/config/aarch64/arm_acle.h                   | 14 ++++++++++++++
 .../gcc.target/aarch64/acle/acle_fma.c          | 17 +++++++++++++++++
 2 files changed, 31 insertions(+)
 create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c

Comments

Kyrylo Tkachov March 11, 2025, 8:55 a.m. UTC | #1 
Hi Ayan,

> On 9 Mar 2025, at 21:46, Ayan Shafqat <ayan.x.shafqat@gmail.com> wrote:
> 
> This patch introduces inline definitions for the __fma and __fmaf
> functions in arm_acle.h for AArch64 targets. These definitions rely on
> __builtin_fma and __builtin_fmaf to ensure proper inlining and to meet
> the ACLE requirements [1].
> 
> The patch has been tested locally using a crosstool-NG sysroot for
> AArch64, confirming that the generated code uses the expected fused
> multiply-accumulate instructions (fmadd).
> 

This looks ok to me.
GCC is currently in a regression fixing stage so normally such a change would wait until stage 1 reopens.
But this looks like a pretty safe change so I’m not against taking it now.
Do you need someone to commit this for you?

Thanks,
Kyrill 

> [1] https://arm-software.github.io/acle/main/acle.html#fused-multiply-accumulate-fma
> 
> Signed-off-by: Ayan Shafqat <ayan.x.shafqat@gmail.com>
> 
> ---
> gcc/config/aarch64/arm_acle.h                   | 14 ++++++++++++++
> .../gcc.target/aarch64/acle/acle_fma.c          | 17 +++++++++++++++++
> 2 files changed, 31 insertions(+)
> create mode 100644 gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c
> 
> diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
> index 7976c117daf..d9e2401ea9f 100644
> --- a/gcc/config/aarch64/arm_acle.h
> +++ b/gcc/config/aarch64/arm_acle.h
> @@ -129,6 +129,20 @@ __jcvt (double __a)
> 
> #pragma GCC pop_options
> 
> +__extension__ extern __inline double
> +__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
> +__fma (double __x, double __y, double __z)
> +{
> +  return __builtin_fma (__x, __y, __z);
> +}
> +
> +__extension__ extern __inline float
> +__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
> +__fmaf (float __x, float __y, float __z)
> +{
> +  return __builtin_fmaf (__x, __y, __z);
> +}
> +
> #pragma GCC push_options
> #pragma GCC target ("+nothing+frintts")
> __extension__ extern __inline float
> diff --git a/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c b/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c
> new file mode 100644
> index 00000000000..9363a75b593
> --- /dev/null
> +++ b/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c
> @@ -0,0 +1,17 @@
> +/* { dg-do compile } */
> +/* { dg-options "-O2" } */
> +
> +#include "arm_acle.h"
> +
> +double test_acle_fma (double x, double y, double z)
> +{
> +  return __fma (x, y, z);
> +}
> +
> +float test_acle_fmaf (float x, float y, float z)
> +{
> +  return __fmaf (x, y, z);
> +}
> +
> +/* { dg-final { scan-assembler-times "fmadd\td\[0-9\]" 1 } } */
> +/* { dg-final { scan-assembler-times "fmadd\ts\[0-9\]" 1 } } */
> -- 
> 2.43.0
>
Ayan Shafqat March 11, 2025, 1:53 p.m. UTC | #2 
Hello Kyrylo,

On Tue, Mar 11, 2025 at 08:55:46AM +0000, Kyrylo Tkachov wrote:
> This looks ok to me.
> GCC is currently in a regression fixing stage so normally such a change would wait until stage 1 reopens.
> But this looks like a pretty safe change so I’m not against taking it now.
> Do you need someone to commit this for you?

Thank you very much for reviewing this patch! I do not have commit
access to GCC, so I would greatly appreciate it if you can commit this
patch on my behalf. Please let me know if you need anything else.

Best regards,
Ayan
Kyrylo Tkachov March 13, 2025, 8:31 a.m. UTC | #3 
Hi Ayan,

> On 11 Mar 2025, at 14:53, Ayan Shafqat <ayan.x.shafqat@gmail.com> wrote:
> 
> Hello Kyrylo,
> 
> On Tue, Mar 11, 2025 at 08:55:46AM +0000, Kyrylo Tkachov wrote:
>> This looks ok to me.
>> GCC is currently in a regression fixing stage so normally such a change would wait until stage 1 reopens.
>> But this looks like a pretty safe change so I’m not against taking it now.
>> Do you need someone to commit this for you?
> 
> Thank you very much for reviewing this patch! I do not have commit
> access to GCC, so I would greatly appreciate it if you can commit this
> patch on my behalf. Please let me know if you need anything else.
> 

I forgot during the review, but a patch needs a ChangeLog entry.
Could you provide one please to add to the commit log?

Thanks,
Kyrill


> Best regards,
> Ayan
Ayan Shafqat March 14, 2025, 12:07 p.m. UTC | #4 
On Thu, Mar 13, 2025 at 08:31:24AM +0000, Kyrylo Tkachov wrote:
>
> I forgot during the review, but a patch needs a ChangeLog entry.
> Could you provide one please to add to the commit log?
>

I have submitted the patch again in the mailing list:

https://gcc.gnu.org/pipermail/gcc-patches/2025-March/677588.html

Let me know if you need anything else.

Thanks in advance,
Ayan
diff mbox series

Patch

diff --git a/gcc/config/aarch64/arm_acle.h b/gcc/config/aarch64/arm_acle.h
index 7976c117daf..d9e2401ea9f 100644
--- a/gcc/config/aarch64/arm_acle.h
+++ b/gcc/config/aarch64/arm_acle.h
@@ -129,6 +129,20 @@  __jcvt (double __a)
 
 #pragma GCC pop_options
 
+__extension__ extern __inline double
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+__fma (double __x, double __y, double __z)
+{
+  return __builtin_fma (__x, __y, __z);
+}
+
+__extension__ extern __inline float
+__attribute__ ((__always_inline__, __gnu_inline__, __artificial__))
+__fmaf (float __x, float __y, float __z)
+{
+  return __builtin_fmaf (__x, __y, __z);
+}
+
 #pragma GCC push_options
 #pragma GCC target ("+nothing+frintts")
 __extension__ extern __inline float
diff --git a/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c b/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c
new file mode 100644
index 00000000000..9363a75b593
--- /dev/null
+++ b/gcc/testsuite/gcc.target/aarch64/acle/acle_fma.c
@@ -0,0 +1,17 @@ 
+/* { dg-do compile } */
+/* { dg-options "-O2" } */
+
+#include "arm_acle.h"
+
+double test_acle_fma (double x, double y, double z)
+{
+  return __fma (x, y, z);
+}
+
+float test_acle_fmaf (float x, float y, float z)
+{
+  return __fmaf (x, y, z);
+}
+
+/* { dg-final { scan-assembler-times "fmadd\td\[0-9\]" 1 } } */
+/* { dg-final { scan-assembler-times "fmadd\ts\[0-9\]" 1 } } */