Skip to content

[libc][math][c23] Add fmaf16 C23 math function. #130757

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 8 commits into from
Mar 23, 2025

Conversation

harrisonGPU
Copy link
Contributor

@harrisonGPU harrisonGPU commented Mar 11, 2025

Implementation of fmaf16 function for 16-bit inputs.

@harrisonGPU harrisonGPU requested a review from overmighty March 11, 2025 11:49
@harrisonGPU harrisonGPU self-assigned this Mar 11, 2025
@harrisonGPU harrisonGPU requested a review from lntue March 11, 2025 11:50
@harrisonGPU harrisonGPU marked this pull request as ready for review March 12, 2025 02:04
@llvmbot llvmbot added backend:AMDGPU libc bazel "Peripheral" support tier build system: utils/bazel labels Mar 12, 2025
@llvmbot
Copy link
Member

llvmbot commented Mar 12, 2025

@llvm/pr-subscribers-backend-amdgpu

@llvm/pr-subscribers-libc

Author: Harrison Hao (harrisonGPU)

Changes

Implementation of fmaf16 function for 16-bit inputs.


Full diff: https://github.com/llvm/llvm-project/pull/130757.diff

17 Files Affected:

  • (modified) libc/config/gpu/amdgpu/entrypoints.txt (+1)
  • (modified) libc/config/gpu/nvptx/entrypoints.txt (+1)
  • (modified) libc/config/linux/aarch64/entrypoints.txt (+1)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+1)
  • (modified) libc/docs/headers/math/index.rst (+1-1)
  • (modified) libc/include/math.yaml (+9)
  • (modified) libc/src/math/CMakeLists.txt (+1)
  • (added) libc/src/math/fmaf16.h (+21)
  • (modified) libc/src/math/generic/CMakeLists.txt (+10)
  • (added) libc/src/math/generic/fmaf16.cpp (+20)
  • (modified) libc/test/src/math/CMakeLists.txt (+15)
  • (added) libc/test/src/math/fmaf16_test.cpp (+13)
  • (modified) libc/test/src/math/smoke/CMakeLists.txt (+12)
  • (added) libc/test/src/math/smoke/fmaf16_test.cpp (+13)
  • (modified) libc/utils/MPFRWrapper/MPFRUtils.cpp (+5)
  • (modified) utils/bazel/llvm-project-overlay/libc/BUILD.bazel (+2)
  • (modified) utils/bazel/llvm-project-overlay/libc/test/src/math/smoke/BUILD.bazel (+5)
diff --git a/libc/config/gpu/amdgpu/entrypoints.txt b/libc/config/gpu/amdgpu/entrypoints.txt
index 291d86b4dd587..a5d32c8eda39f 100644
--- a/libc/config/gpu/amdgpu/entrypoints.txt
+++ b/libc/config/gpu/amdgpu/entrypoints.txt
@@ -548,6 +548,7 @@ if(LIBC_TYPES_HAS_FLOAT16)
     libc.src.math.fabsf16
     libc.src.math.fdimf16
     libc.src.math.floorf16
+    libc.src.math.fmaf16
     libc.src.math.fmaxf16
     libc.src.math.fmaximum_mag_numf16
     libc.src.math.fmaximum_magf16
diff --git a/libc/config/gpu/nvptx/entrypoints.txt b/libc/config/gpu/nvptx/entrypoints.txt
index 1ea0d9b03b37e..f553f7cc2b210 100644
--- a/libc/config/gpu/nvptx/entrypoints.txt
+++ b/libc/config/gpu/nvptx/entrypoints.txt
@@ -550,6 +550,7 @@ if(LIBC_TYPES_HAS_FLOAT16)
     libc.src.math.fabsf16
     libc.src.math.fdimf16
     libc.src.math.floorf16
+    libc.src.math.fmaf16
     libc.src.math.fmaxf16
     libc.src.math.fmaximum_mag_numf16
     libc.src.math.fmaximum_magf16
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index c7beb3ef3fdfc..f32e03ff2524f 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -676,6 +676,7 @@ if(LIBC_TYPES_HAS_FLOAT16)
     libc.src.math.ffma
     libc.src.math.ffmal
     libc.src.math.floorf16
+    libc.src.math.fmaf16
     libc.src.math.fmaxf16
     libc.src.math.fmaximum_mag_numf16
     libc.src.math.fmaximum_magf16
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 12dc87bf945fd..c73690b7864b4 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -686,6 +686,7 @@ if(LIBC_TYPES_HAS_FLOAT16)
     libc.src.math.fabsf16
     libc.src.math.fdimf16
     libc.src.math.floorf16
+    libc.src.math.fmaf16
     libc.src.math.fmaxf16
     libc.src.math.fmaximum_mag_numf16
     libc.src.math.fmaximum_magf16
diff --git a/libc/docs/headers/math/index.rst b/libc/docs/headers/math/index.rst
index 5b855ce4881c3..dda8855df1a5a 100644
--- a/libc/docs/headers/math/index.rst
+++ b/libc/docs/headers/math/index.rst
@@ -299,7 +299,7 @@ Higher Math Functions
 +-----------+------------------+-----------------+------------------------+----------------------+------------------------+------------------------+----------------------------+
 | expm1     | |check|          | |check|         |                        | |check|              |                        | 7.12.6.6               | F.10.3.6                   |
 +-----------+------------------+-----------------+------------------------+----------------------+------------------------+------------------------+----------------------------+
-| fma       | |check|          | |check|         |                        |                      |                        | 7.12.13.1              | F.10.10.1                  |
+| fma       | |check|          | |check|         |                        | |check|              |                        | 7.12.13.1              | F.10.10.1                  |
 +-----------+------------------+-----------------+------------------------+----------------------+------------------------+------------------------+----------------------------+
 | f16sqrt   | |check|\*        | |check|\*       | |check|\*              | N/A                  | |check|                | 7.12.14.6              | F.10.11                    |
 +-----------+------------------+-----------------+------------------------+----------------------+------------------------+------------------------+----------------------------+
diff --git a/libc/include/math.yaml b/libc/include/math.yaml
index a66f981030864..08a28263a186d 100644
--- a/libc/include/math.yaml
+++ b/libc/include/math.yaml
@@ -736,6 +736,15 @@ functions:
       - type: float
       - type: float
       - type: float
+  - name: fmaf16
+    standards:
+      - stdc
+    return_type: _Float16
+    arguments:
+      - type: _Float16
+      - type: _Float16
+      - type: _Float16
+    guard: LIBC_TYPES_HAS_FLOAT16
   - name: fmax
     standards:
       - stdc
diff --git a/libc/src/math/CMakeLists.txt b/libc/src/math/CMakeLists.txt
index f18a73d46f9aa..a42421614661a 100644
--- a/libc/src/math/CMakeLists.txt
+++ b/libc/src/math/CMakeLists.txt
@@ -209,6 +209,7 @@ add_math_entrypoint_object(floorf128)
 
 add_math_entrypoint_object(fma)
 add_math_entrypoint_object(fmaf)
+add_math_entrypoint_object(fmaf16)
 
 add_math_entrypoint_object(fmax)
 add_math_entrypoint_object(fmaxf)
diff --git a/libc/src/math/fmaf16.h b/libc/src/math/fmaf16.h
new file mode 100644
index 0000000000000..1c4d468e67a7a
--- /dev/null
+++ b/libc/src/math/fmaf16.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for fmaf16 ------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_LIBC_SRC_MATH_FMAF16_H
+#define LLVM_LIBC_SRC_MATH_FMAF16_H
+
+#include "src/__support/macros/config.h"
+#include "src/__support/macros/properties/types.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+float16 fmaf16(float16 x, float16 y, float16 z);
+
+} // namespace LIBC_NAMESPACE_DECL
+
+#endif // LLVM_LIBC_SRC_MATH_FMAF16_H
diff --git a/libc/src/math/generic/CMakeLists.txt b/libc/src/math/generic/CMakeLists.txt
index 3114289bad486..781e6e7e04906 100644
--- a/libc/src/math/generic/CMakeLists.txt
+++ b/libc/src/math/generic/CMakeLists.txt
@@ -4241,6 +4241,16 @@ add_entrypoint_object(
     libc.src.__support.FPUtil.fma
 )
 
+add_entrypoint_object(
+  fmaf16
+  SRCS
+    fmaf16.cpp
+  HDRS
+    ../fmaf16.h
+  DEPENDS
+    libc.src.__support.FPUtil.fma
+)
+
 add_entrypoint_object(
   fma
   SRCS
diff --git a/libc/src/math/generic/fmaf16.cpp b/libc/src/math/generic/fmaf16.cpp
new file mode 100644
index 0000000000000..4f712f5de764f
--- /dev/null
+++ b/libc/src/math/generic/fmaf16.cpp
@@ -0,0 +1,20 @@
+//===-- Implementation of fmaf16 function ---------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "src/math/fmaf16.h"
+#include "src/__support/FPUtil/FMA.h"
+#include "src/__support/common.h"
+#include "src/__support/macros/config.h"
+
+namespace LIBC_NAMESPACE_DECL {
+
+LLVM_LIBC_FUNCTION(float16, fmaf16, (float16 x, float16 y, float16 z)) {
+  return fputil::fma<float16>(x, y, z);
+}
+
+} // namespace LIBC_NAMESPACE_DECL
diff --git a/libc/test/src/math/CMakeLists.txt b/libc/test/src/math/CMakeLists.txt
index 53ddd301900c0..44e499b6297f4 100644
--- a/libc/test/src/math/CMakeLists.txt
+++ b/libc/test/src/math/CMakeLists.txt
@@ -1776,6 +1776,21 @@ add_fp_unittest(
     FMA_OPT__ONLY
 )
 
+add_fp_unittest(
+  fmaf16_test
+  NEED_MPFR
+  SUITE
+    libc-math-unittests
+  SRCS
+    fmaf16_test.cpp
+  HDRS
+    FmaTest.h
+  DEPENDS
+    libc.src.math.fmaf16
+    libc.src.stdlib.rand
+    libc.src.stdlib.srand
+)
+
 add_fp_unittest(
   fma_test
   NEED_MPFR
diff --git a/libc/test/src/math/fmaf16_test.cpp b/libc/test/src/math/fmaf16_test.cpp
new file mode 100644
index 0000000000000..233d3a7c54cf4
--- /dev/null
+++ b/libc/test/src/math/fmaf16_test.cpp
@@ -0,0 +1,13 @@
+//===-- Unittests for fmaf16 ----------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "FmaTest.h"
+
+#include "src/math/fmaf16.h"
+
+LIST_FMA_TESTS(float16, LIBC_NAMESPACE::fmaf16)
diff --git a/libc/test/src/math/smoke/CMakeLists.txt b/libc/test/src/math/smoke/CMakeLists.txt
index 6f94440d826d9..e515633ad6720 100644
--- a/libc/test/src/math/smoke/CMakeLists.txt
+++ b/libc/test/src/math/smoke/CMakeLists.txt
@@ -3562,6 +3562,18 @@ add_fp_unittest(
     libc.src.__support.macros.properties.types
 )
 
+add_fp_unittest(
+  fmaf16_test
+  SUITE
+    libc-math-smoke-tests
+  SRCS
+    fmaf16_test.cpp
+  HDRS
+    FmaTest.h
+  DEPENDS
+    libc.src.math.fmaf16
+)
+
 add_fp_unittest(
   fma_test
   SUITE
diff --git a/libc/test/src/math/smoke/fmaf16_test.cpp b/libc/test/src/math/smoke/fmaf16_test.cpp
new file mode 100644
index 0000000000000..cc1f01e2f9541
--- /dev/null
+++ b/libc/test/src/math/smoke/fmaf16_test.cpp
@@ -0,0 +1,13 @@
+//===-- Unittests for fmaxf16 --------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "FmaTest.h"
+
+#include "src/math/fmaf16.h"
+
+LIST_FMA_TESTS(float16, LIBC_NAMESPACE::fmaf16)
diff --git a/libc/utils/MPFRWrapper/MPFRUtils.cpp b/libc/utils/MPFRWrapper/MPFRUtils.cpp
index 775a3d1a31964..f29fe43ad8c88 100644
--- a/libc/utils/MPFRWrapper/MPFRUtils.cpp
+++ b/libc/utils/MPFRWrapper/MPFRUtils.cpp
@@ -449,6 +449,8 @@ explain_ternary_operation_one_output_error(Operation,
                                            long double, double, RoundingMode);
 
 #ifdef LIBC_TYPES_HAS_FLOAT16
+template void explain_ternary_operation_one_output_error(
+    Operation, const TernaryInput<float16> &, float16, double, RoundingMode);
 template void explain_ternary_operation_one_output_error(
     Operation, const TernaryInput<float> &, float16, double, RoundingMode);
 template void explain_ternary_operation_one_output_error(
@@ -660,6 +662,9 @@ compare_ternary_operation_one_output(Operation,
                                      long double, double, RoundingMode);
 
 #ifdef LIBC_TYPES_HAS_FLOAT16
+template bool
+compare_ternary_operation_one_output(Operation, const TernaryInput<float16> &,
+                                     float16, double, RoundingMode);
 template bool compare_ternary_operation_one_output(Operation,
                                                    const TernaryInput<float> &,
                                                    float16, double,
diff --git a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
index 757db65ca8a77..849f4b728898a 100644
--- a/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/BUILD.bazel
@@ -2826,6 +2826,8 @@ libc_math_function(name = "floorf16")
 
 # TODO: Add fma, fmaf, fmal, fmaf128 functions.
 
+libc_math_function(name = "fmaf16")
+
 libc_math_function(name = "fmax")
 
 libc_math_function(name = "fmaxf")
diff --git a/utils/bazel/llvm-project-overlay/libc/test/src/math/smoke/BUILD.bazel b/utils/bazel/llvm-project-overlay/libc/test/src/math/smoke/BUILD.bazel
index 803548178b0e3..6b959fe559e45 100644
--- a/utils/bazel/llvm-project-overlay/libc/test/src/math/smoke/BUILD.bazel
+++ b/utils/bazel/llvm-project-overlay/libc/test/src/math/smoke/BUILD.bazel
@@ -474,6 +474,11 @@ math_test(
 
 # TODO: Add fma, fmaf, fmal, fmaf128 tests.
 
+math_test(
+    name = "fmaf16",
+    hdrs = ["FMaxTest.h"],
+)
+
 math_test(
     name = "fmax",
     hdrs = ["FMaxTest.h"],

@overmighty
Copy link
Member

Why did the bot add the backend:AMDGPU label?

Comment on lines 2827 to 2835
# TODO: Add fma, fmaf, fmal, fmaf128 functions.

libc_math_function(name = "fmaf16")

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lntue Why the TODO?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It was probably when I didn't set up the target dependency selection for fma.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@harrisonGPU you'll need to add extra dependency similar to ffma as above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, Thanks. :-)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@lntue , I have updated it, what do you think about it? :-)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see changes to this file. Did you forget to push the update?

Copy link
Contributor Author

@harrisonGPU harrisonGPU Mar 20, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I have updated it. What do you think about it? @lntue

Comment on lines 475 to 481
# TODO: Add fma, fmaf, fmal, fmaf128 tests.

math_test(
name = "fmaf16",
hdrs = ["FMaxTest.h"],
)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here, why was there a TODO for the other fma* functions?

@harrisonGPU
Copy link
Contributor Author

Why did the bot add the backend:AMDGPU label?

Because I added entrypoints to amdgpu? Right?

@harrisonGPU harrisonGPU requested a review from overmighty March 13, 2025 03:23
@overmighty
Copy link
Member

My understanding was that the backend:* labels were for LLVM code gen backends. I'm not sure if this label was intended to be applied to PRs that make changes to libc/config/gpu/amdgpu/entrypoints.txt.

@harrisonGPU
Copy link
Contributor Author

My understanding was that the backend:* labels were for LLVM code gen backends. I'm not sure if this label was intended to be applied to PRs that make changes to libc/config/gpu/amdgpu/entrypoints.txt.

I am also not sure why not add this label. So what else can I do? :-)

Comment on lines 3573 to 3578
DEPENDS
libc.src.math.fmaf16
libc.src.__support.macros.properties.types
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
DEPENDS
libc.src.math.fmaf16
libc.src.__support.macros.properties.types
)
DEPENDS
libc.src.math.fmaf16
libc.src.__support.CPP.type_traits
libc.src.__support.FPUtil.cast
libc.src.__support.macros.properties.types
)

Or maybe we should fix this now that we have fputil::cast:

#if defined(LIBC_TYPES_HAS_FLOAT16) && !defined(__LIBC_USE_FLOAT16_CONVERSION)
// Rounding modes other than the default might not be usable with float16.
if constexpr (LIBC_NAMESPACE::cpp::is_same_v<OutType, float16>)
EXPECT_FP_EQ(OutType(0.75) * z, func(InType(1.75), in_z, -in_z));
else
#endif

Then FmaTest.h would only need libc.src.__support.FPUtil.cast.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @overmighty , I have updated it, please take a look. :-)

@arsenm
Copy link
Contributor

arsenm commented Mar 17, 2025

My understanding was that the backend:* labels were for LLVM code gen backends.

No, not really. It's fuzzier than that, relevant to the target can spread to many more components and are not localized to the codegen. The auto-labeling rules we have though are too aggressive (in particular many of the test directories are mostly noise)

Copy link

github-actions bot commented Mar 19, 2025

✅ With the latest revision this PR passed the C/C++ code formatter.

@lntue
Copy link
Contributor

lntue commented Mar 21, 2025

Can you investigate the precommit failures?

@harrisonGPU
Copy link
Contributor Author

Can you investigate the precommit failures?

Okay.

@harrisonGPU
Copy link
Contributor Author

Hi, @lntue @overmighty , I found an issue with the FMA tests. When we enable the fmaf16 type test, the values of IN_MAX_SUBNORMAL_U and IN_MIN_SUBNORMAL_U are 1023 and 1 respectively:

static constexpr OutStorageType OUT_MIN_NORMAL_U =
OutFPBits::min_normal().uintval();
static constexpr InStorageType IN_MAX_NORMAL_U =
InFPBits::max_normal().uintval();
static constexpr InStorageType IN_MIN_NORMAL_U =
InFPBits::min_normal().uintval();
static constexpr InStorageType IN_MAX_SUBNORMAL_U =
InFPBits::max_subnormal().uintval();
static constexpr InStorageType IN_MIN_SUBNORMAL_U =
InFPBits::min_subnormal().uintval();

If we calculate the step as follows:

constexpr InStorageType STEP =
        (IN_MAX_SUBNORMAL_U - IN_MIN_SUBNORMAL_U) / COUNT;

this will result in STEP being zero due to integer division, causing the loop condition never to be met and the loop to run indefinitely:

for (InStorageType v = IN_MIN_SUBNORMAL_U, w = IN_MAX_SUBNORMAL_U;
v <= IN_MAX_SUBNORMAL_U && w >= IN_MIN_SUBNORMAL_U;
v += STEP, w -= STEP) {
InType x = InFPBits(get_random_bit_pattern()).get_val();
InType y = InFPBits(v).get_val();
InType z = InFPBits(w).get_val();
mpfr::TernaryInput<InType> input{x, y, z};
ASSERT_MPFR_MATCH_ALL_ROUNDING(mpfr::Operation::Fma, input, func(x, y, z),
0.5);
}
,
I have made some modifications in this PR to address this issue. Could you please take a look? :-)

Copy link
Contributor

@lntue lntue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Please wait to see if @overmighty has any other comments.

Copy link
Member

@overmighty overmighty left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can remove some includes from libc/test/src/math/smoke/FmaTest.h now that the rounding issues should be solved by fputil::cast. Other than that, LGTM.

@harrisonGPU
Copy link
Contributor Author

Thanks!

@harrisonGPU harrisonGPU merged commit 445837a into llvm:main Mar 23, 2025
18 checks passed
@harrisonGPU harrisonGPU deleted the libcFmaf16 branch March 23, 2025 02:49
googlewalt added a commit that referenced this pull request Mar 25, 2025
These tests failed at Google after #130757. Disable them in bazel for the time being.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel c23 libc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants