Skip to content

[libc][math][c23] Implement fmaxf16 and fminf16 function #94131

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 6 commits into from
Jun 6, 2024

Conversation

HendrikHuebner
Copy link
Contributor

Implements fmaxf16 and fminf16, which are two missing functions listed here: #93566

@llvmbot llvmbot added the libc label Jun 2, 2024
@llvmbot
Copy link
Member

llvmbot commented Jun 2, 2024

@llvm/pr-subscribers-libc

Author: Hendrik Hübner (HendrikHuebner)

Changes

Implements fmaxf16 and fminf16, which are two missing functions listed here: #93566


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

12 Files Affected:

  • (modified) libc/config/linux/aarch64/entrypoints.txt (+2)
  • (modified) libc/config/linux/x86_64/entrypoints.txt (+2)
  • (modified) libc/spec/stdc.td (+2)
  • (modified) libc/src/math/CMakeLists.txt (+2)
  • (added) libc/src/math/fmaxf16.h (+21)
  • (added) libc/src/math/fminf16.h (+20)
  • (modified) libc/src/math/generic/CMakeLists.txt (+27)
  • (added) libc/src/math/generic/fmaxf16.cpp (+19)
  • (added) libc/src/math/generic/fminf16.cpp (+19)
  • (modified) libc/test/src/math/smoke/CMakeLists.txt (+13)
  • (added) libc/test/src/math/smoke/fmaxf16_test.cpp (+13)
  • (added) libc/test/src/math/smoke/fminf16_test.cpp (+13)
diff --git a/libc/config/linux/aarch64/entrypoints.txt b/libc/config/linux/aarch64/entrypoints.txt
index ca0418c3618ae..6552d32ef6c87 100644
--- a/libc/config/linux/aarch64/entrypoints.txt
+++ b/libc/config/linux/aarch64/entrypoints.txt
@@ -500,6 +500,8 @@ if(LIBC_TYPES_HAS_FLOAT16)
   list(APPEND TARGET_LIBM_ENTRYPOINTS
     # math.h C23 _Float16 entrypoints
     libc.src.math.fabsf16
+    libc.src.math.fmaxf16
+    libc.src.math.fminf16
   )
 endif()
 
diff --git a/libc/config/linux/x86_64/entrypoints.txt b/libc/config/linux/x86_64/entrypoints.txt
index 367db7d384d23..dca2ec6d08e64 100644
--- a/libc/config/linux/x86_64/entrypoints.txt
+++ b/libc/config/linux/x86_64/entrypoints.txt
@@ -533,6 +533,8 @@ if(LIBC_TYPES_HAS_FLOAT16)
   list(APPEND TARGET_LIBM_ENTRYPOINTS
     # math.h C23 _Float16 entrypoints
     libc.src.math.fabsf16
+    libc.src.math.fmaxf16
+    libc.src.math.fminf16
   )
 endif()
 
diff --git a/libc/spec/stdc.td b/libc/spec/stdc.td
index 109721b8b12a0..e1cc00497761d 100644
--- a/libc/spec/stdc.td
+++ b/libc/spec/stdc.td
@@ -412,11 +412,13 @@ def StdC : StandardSpec<"stdc"> {
           FunctionSpec<"fminf", RetValSpec<FloatType>, [ArgSpec<FloatType>, ArgSpec<FloatType>]>,
           FunctionSpec<"fminl", RetValSpec<LongDoubleType>, [ArgSpec<LongDoubleType>, ArgSpec<LongDoubleType>]>,
           GuardedFunctionSpec<"fminf128", RetValSpec<Float128Type>, [ArgSpec<Float128Type>, ArgSpec<Float128Type>], "LIBC_TYPES_HAS_FLOAT128">,
+          GuardedFunctionSpec<"fminf16", RetValSpec<Float16Type>, [ArgSpec<Float16Type>, ArgSpec<Float16Type>], "LIBC_TYPES_HAS_FLOAT16">,
 
           FunctionSpec<"fmax", RetValSpec<DoubleType>, [ArgSpec<DoubleType>, ArgSpec<DoubleType>]>,
           FunctionSpec<"fmaxf", RetValSpec<FloatType>, [ArgSpec<FloatType>, ArgSpec<FloatType>]>,
           FunctionSpec<"fmaxl", RetValSpec<LongDoubleType>, [ArgSpec<LongDoubleType>, ArgSpec<LongDoubleType>]>,
           GuardedFunctionSpec<"fmaxf128", RetValSpec<Float128Type>, [ArgSpec<Float128Type>, ArgSpec<Float128Type>], "LIBC_TYPES_HAS_FLOAT128">,
+          GuardedFunctionSpec<"fmaxf16", RetValSpec<Float16Type>, [ArgSpec<Float16Type>, ArgSpec<Float16Type>], "LIBC_TYPES_HAS_FLOAT16">,
 	  
 	  FunctionSpec<"fmaximum", RetValSpec<DoubleType>, [ArgSpec<DoubleType>, ArgSpec<DoubleType>]>,
           FunctionSpec<"fmaximumf", RetValSpec<FloatType>, [ArgSpec<FloatType>, ArgSpec<FloatType>]>,
diff --git a/libc/src/math/CMakeLists.txt b/libc/src/math/CMakeLists.txt
index 31df5d0ab8809..ef463565f9146 100644
--- a/libc/src/math/CMakeLists.txt
+++ b/libc/src/math/CMakeLists.txt
@@ -119,11 +119,13 @@ add_math_entrypoint_object(fmax)
 add_math_entrypoint_object(fmaxf)
 add_math_entrypoint_object(fmaxl)
 add_math_entrypoint_object(fmaxf128)
+add_math_entrypoint_object(fmaxf16)
 
 add_math_entrypoint_object(fmin)
 add_math_entrypoint_object(fminf)
 add_math_entrypoint_object(fminl)
 add_math_entrypoint_object(fminf128)
+add_math_entrypoint_object(fminf16)
 
 add_math_entrypoint_object(fmaximum)
 add_math_entrypoint_object(fmaximumf)
diff --git a/libc/src/math/fmaxf16.h b/libc/src/math/fmaxf16.h
new file mode 100644
index 0000000000000..f8835abbe6daa
--- /dev/null
+++ b/libc/src/math/fmaxf16.h
@@ -0,0 +1,21 @@
+//===-- Implementation header for fmaxf16 -------------------------*- 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_FMAXF_H
+#define LLVM_LIBC_SRC_MATH_FMAXF_H
+
+#include "src/__support/macros/properties/types.h"
+
+namespace LIBC_NAMESPACE {
+
+float16 fmaxf16(float16 x, float16 y);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_MATH_FMAXF16_H
+
diff --git a/libc/src/math/fminf16.h b/libc/src/math/fminf16.h
new file mode 100644
index 0000000000000..1b9a4a484064c
--- /dev/null
+++ b/libc/src/math/fminf16.h
@@ -0,0 +1,20 @@
+//===-- Implementation header for fminf16 ----------------------*- 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_FMINF16_H
+#define LLVM_LIBC_SRC_MATH_FMINF16_H
+
+#include "src/__support/macros/properties/types.h"
+
+namespace LIBC_NAMESPACE {
+
+float16 fminf128(float16 x, float16 y);
+
+} // namespace LIBC_NAMESPACE
+
+#endif // LLVM_LIBC_SRC_MATH_FMINF16_H
diff --git a/libc/src/math/generic/CMakeLists.txt b/libc/src/math/generic/CMakeLists.txt
index 04656e3186181..e37d465260cb4 100644
--- a/libc/src/math/generic/CMakeLists.txt
+++ b/libc/src/math/generic/CMakeLists.txt
@@ -1612,6 +1612,20 @@ add_entrypoint_object(
     -O3
 )
 
+add_entrypoint_object(
+  fminf16
+  SRCS
+    fminf16.cpp
+  HDRS
+    ../fminf16.h
+  DEPENDS
+    libc.src.__support.macros.properties.types
+    libc.src.__support.FPUtil.basic_operations
+  COMPILE_OPTIONS
+    -O3
+)
+
+
 add_entrypoint_object(
   fmax
   SRCS
@@ -1661,6 +1675,19 @@ add_entrypoint_object(
     -O3
 )
 
+add_entrypoint_object(
+  fmaxf16
+  SRCS
+    fmaxf16.cpp
+  HDRS
+    ../fmaxf16.h
+  DEPENDS
+    libc.src.__support.macros.properties.types
+    libc.src.__support.FPUtil.basic_operations
+  COMPILE_OPTIONS
+    -O3
+)
+
 add_entrypoint_object(
   fmaximum
   SRCS
diff --git a/libc/src/math/generic/fmaxf16.cpp b/libc/src/math/generic/fmaxf16.cpp
new file mode 100644
index 0000000000000..022ccb6287eda
--- /dev/null
+++ b/libc/src/math/generic/fmaxf16.cpp
@@ -0,0 +1,19 @@
+//===-- Unittest of fmaxf16 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/fmaxf16.h"
+#include "src/__support/FPUtil/BasicOperations.h"
+#include "src/__support/common.h"
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(float16, fmaxf16, (float16 x, float16 y)) {
+  return fputil::fmax(x, y);
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/src/math/generic/fminf16.cpp b/libc/src/math/generic/fminf16.cpp
new file mode 100644
index 0000000000000..c8abb22554878
--- /dev/null
+++ b/libc/src/math/generic/fminf16.cpp
@@ -0,0 +1,19 @@
+//===-- Unittest of fminf16 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/fminf16.h"
+#include "src/__support/FPUtil/BasicOperations.h"
+#include "src/__support/common.h"
+
+namespace LIBC_NAMESPACE {
+
+LLVM_LIBC_FUNCTION(float16, fminf16, (float16 x, float16 y)) {
+  return fputil::fmin(x, y);
+}
+
+} // namespace LIBC_NAMESPACE
diff --git a/libc/test/src/math/smoke/CMakeLists.txt b/libc/test/src/math/smoke/CMakeLists.txt
index b46fe5e902c67..c67aaf738567f 100644
--- a/libc/test/src/math/smoke/CMakeLists.txt
+++ b/libc/test/src/math/smoke/CMakeLists.txt
@@ -1558,6 +1558,19 @@ add_fp_unittest(
     libc.src.__support.FPUtil.fp_bits
 )
 
+add_fp_unittest(
+  fmaxf16_test
+  SUITE
+    libc-math-smoke-tests
+  SRCS
+    fmaxf16_test.cpp
+  HDRS
+    FMaxTest.h
+  DEPENDS
+    libc.src.math.fmaxf16
+    libc.src.__support.FPUtil.fp_bits
+)
+
 add_fp_unittest(
   fmaximuml_test
   SUITE
diff --git a/libc/test/src/math/smoke/fmaxf16_test.cpp b/libc/test/src/math/smoke/fmaxf16_test.cpp
new file mode 100644
index 0000000000000..1f4a1a5341da6
--- /dev/null
+++ b/libc/test/src/math/smoke/fmaxf16_test.cpp
@@ -0,0 +1,13 @@
+//===-- Unittests for fmaxf128 --------------------------------------------===//
+//
+// 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 "FMaxTest.h"
+
+#include "src/math/fmaxf16.h"
+
+LIST_FMAX_TESTS(float16, LIBC_NAMESPACE::fmaxf16)
diff --git a/libc/test/src/math/smoke/fminf16_test.cpp b/libc/test/src/math/smoke/fminf16_test.cpp
new file mode 100644
index 0000000000000..7b408866195d3
--- /dev/null
+++ b/libc/test/src/math/smoke/fminf16_test.cpp
@@ -0,0 +1,13 @@
+//===-- Unittests for fminf -----------------------------------------------===//
+//
+// 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 "FMaxTest.h"
+
+#include "src/math/fminf16.h"
+
+LIST_FMAX_TESTS(float16, LIBC_NAMESPACE::fminf16)

Copy link

github-actions bot commented Jun 2, 2024

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

@HendrikHuebner HendrikHuebner force-pushed the dev/libc-f16minmax branch 2 times, most recently from 6d5bce0 to 9c40026 Compare June 2, 2024 09:13
@overmighty
Copy link
Member

Thanks, but I'm not sure if I'm allowed to have other people work on this issue as it is part of a Google Summer of Code project: https://summerofcode.withgoogle.com/programs/2024/projects/1vPVUj93.

@HendrikHuebner
Copy link
Contributor Author

HendrikHuebner commented Jun 2, 2024

Uhm... well it would certainly be good if that was stated in the issue somewhere. Maybe put [gsoc] in the title?

Could you check if this can be merged anyways?

@overmighty
Copy link
Member

From what I've seen, you're expected to ask to be assigned to an issue before opening a PR for it. You should see that I am listed in the "Assignees" list.

I'll ask GSoC support about this.

@lntue lntue self-requested a review June 3, 2024 15:13
@lntue
Copy link
Contributor

lntue commented Jun 3, 2024

Uhm... well it would certainly be good if that was stated in the issue somewhere. Maybe put [gsoc] in the title?

Could you check if this can be merged anyways?

Sorry for the confusion and it's totally my fault to not making the tracking issue cleared that it's for this year GSoC project.

@overmighty : can you update #93566 title and description to indicate that it is a tracking issue for this year GSoC project?

@HendrikHuebner : thanks for the patch, I'll review and merge the PR. And let me know if you are interested in working on math functions for libc, so that we can coordinate to avoid duplicated work. Direct messaging me on discord or on libc channel would work best for me.

@@ -0,0 +1,13 @@
//===-- Unittests for fmaxf128 --------------------------------------------===//
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fmaxf16

@overmighty
Copy link
Member

@overmighty : can you update #93566 title and description to indicate that it is a tracking issue for this year GSoC project?

Done.

@@ -0,0 +1,13 @@
//===-- Unittests for fminf -----------------------------------------------===//
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: fminf16

@@ -0,0 +1,19 @@
//===-- Unittest of fmaxf16 function --------------------------------------===//
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: Implementation

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.

Overall, it looks good to me. Can you also update the status page at docs/math/index.rst?

Comment on lines 9 to 10
#ifndef LLVM_LIBC_SRC_MATH_FMAXF_H
#define LLVM_LIBC_SRC_MATH_FMAXF_H
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
#ifndef LLVM_LIBC_SRC_MATH_FMAXF_H
#define LLVM_LIBC_SRC_MATH_FMAXF_H
#ifndef LLVM_LIBC_SRC_MATH_FMAXF16_H
#define LLVM_LIBC_SRC_MATH_FMAXF16_H

@@ -0,0 +1,20 @@
//===-- Implementation header for fminf16 ----------------------*- C++ -*-===//
Copy link
Member

Choose a reason for hiding this comment

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

Nit:

Suggested change
//===-- Implementation header for fminf16 ----------------------*- C++ -*-===//
//===-- Implementation header for fminf16 -----------------------*- C++ -*-===//

@HendrikHuebner
Copy link
Contributor Author

HendrikHuebner commented Jun 3, 2024

@lntue sure, I'll update the comments and the status page later. If there are any non-gsoc math functions that need implementation let me know. I can do a few others.

@lntue
Copy link
Contributor

lntue commented Jun 4, 2024

@lntue sure, I'll update the comments and the status page later. If there are any non-gsoc math functions that need implementation let me know. I can do a few others.

@HendrikHuebner can you comment on #94312 so that I can assign it to you?

@overmighty
Copy link
Member

I emailed GSoC support and it turns out this is fine.

@HendrikHuebner
Copy link
Contributor Author

HendrikHuebner commented Jun 4, 2024

@lntue I added the suggested changes

@lntue
Copy link
Contributor

lntue commented Jun 5, 2024

@lntue I added the suggested changes

@HendrikHuebner Can you sync / rebase the PR to resolve conflicts?

@HendrikHuebner
Copy link
Contributor Author

done

@lntue lntue merged commit 8e67495 into llvm:main Jun 6, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants