[libc-commits] [PATCH] D82997: Add implementations of fmin and fminf

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

There are a few inline comments, but a few additional items:

1. This patch should also add cmake targets.
2. Why not also add `fminl`?



================
Comment at: libc/test/src/math/fminf_test.cpp:13
 #include "utils/FPUtil/FloatOperations.h"
 #include "utils/FPUtil/FloatProperties.h"
 #include "utils/UnitTest/Test.h"
----------------
We want to get rid of `BitPatterns.h`, `FloatOperations.h` and `FloatProperites.h`. Look at this to see how we can do it: https://github.com/llvm/llvm-project/blob/master/libc/test/src/math/ceill_test.cpp

Same for `fmin_test.cpp` as well.


================
Comment at: libc/test/src/math/fminf_test.cpp:51
 
 }
----------------
Can we also add tests for non-special numbers?


================
Comment at: libc/utils/FPUtil/ComparisonFunctions.h:1
 //===-- Floating-point manipulation functions -------------------*- C++ -*-===//
 //
----------------
On [[ https://en.cppreference.com/w/c/numeric/math | cppreference.com ]], `fmin` et al are listed under //Basic operations//. So, lets put their implementations in `BasicOperations.h`.


================
Comment at: libc/utils/FPUtil/ComparisonFunctions.h:30
+    return x;
+  } else if (bit_x.sign != bit_y.sign) {
+    return (bit_x.sign ? x : y);
----------------
After the NaN checks, can we just do:

```
  return x > y ? y : x;
```


================
Comment at: libc/utils/FPUtil/ComparisonFunctions.h:32
+    return (bit_x.sign ? x : y);
+  } else if (absBits(x) < absBits(y)) {
+    return bit_x.sign ? y : x;
----------------
This might become irrelevant if we can do as I suggested above. But, `absBits(..)` returns an integer. So, the integers are being compared here and not the underlying floating point numbers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D82997/new/

https://reviews.llvm.org/D82997