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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added a comment.

The actual implementation LGTM. My comments are all for the test infrastructure. I have left comments only in  `fminl_test.cpp`, but most of them are relevant for other tests as well. One of them is about using MPFR for testing `fmin` and friends. While what you have added to MPFRUtil will be required for functions like `hypot`, `fma` etc., it seems like it is an overkill for `fmin`. That said, feel free to convince me otherwise.



================
Comment at: libc/test/src/math/fminl_test.cpp:26
+#define GET_BITS(LD_VALUE)                                                     \
+  static_cast<unsigned long long>(FPBits(LD_VALUE).bitsAsUInt())
+
----------------
Wouldn't casting to `unsigned long long` lead to truncation? If you actually feel strongly about this setup, I would suggest adding an `__uint128` overload to `EXPECT_EQ` and friends.


================
Comment at: libc/test/src/math/fminl_test.cpp:40
+
+  for (size_t i = 0; i < orderedValues.size(); ++i) {
+    for (size_t j = 0; j < orderedValues.size(); ++j) {
----------------
This makes the code concise, but I am finding it really hard to read. Instead of having just one `SpecialNumbers` test class. What do you think of having three test classes like `NaNArg`, `InfArg`, `BothZero`. The `NaNArg` class tests with at least one NaN against a variety of numbers. The `InfArg` test would be similar. The `BothZero` test would test with (+0, +0), (+0, -0), (-0, +0), (-0, -0).


================
Comment at: libc/test/src/math/fminl_test.cpp:60
+  constexpr UIntType step = UIntType(-1) / count;
+  for (UIntType i = 0, v = 0, w = UIntType(-1); i <= count; ++i, v += step, w -= step) {
+    long double x = FPBits(v), y = FPBits(w);
----------------
+1 for this arrangement.


================
Comment at: libc/test/src/math/fminl_test.cpp:62
+    long double x = FPBits(v), y = FPBits(w);
+    if (isnan(x) || isinf(x) || x == 0)
+      continue;
----------------
Continuing on `x == 0` skips comparisons for the kind `fmin(0, <non-zero, non-inf. non-nan>)`. May be skip only if `x == 0 && y == 0`.


================
Comment at: libc/test/src/math/fminl_test.cpp:67
+
+    ASSERT_MPFR_MATCH_BINOP(mpfr::BinaryOperation::Fmin, x, y,
+                            __llvm_libc::fminl(x, y),
----------------
Do we really need MPFR here? Considering all special numbers (including zero) are tested separately, wouldn't it be sufficient to test this way:

```
if (x < y)
  ASSERT_FP_EQ(fmin(x, y), x);
else
  ASSERT_FP_EQ(fmix(x, y), y);
```




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82997