Page MenuHomePhabricator

[libc] Add implementations of fmin, fminf and fminl.
AcceptedPublic

Authored by lntue on Jul 1 2020, 2:38 PM.

Details

Reviewers
sivachandra

Diff Detail

Event Timeline

lntue created this revision.Jul 1 2020, 2:38 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 1 2020, 2:38 PM
Herald added a subscriber: mgorny. · View Herald Transcript
lntue updated this revision to Diff 275074.Jul 2 2020, 5:36 AM

Fix clang-format

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?
libc/test/src/math/fminf_test.cpp
13

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.

51

Can we also add tests for non-special numbers?

libc/utils/FPUtil/ComparisonFunctions.h
1 ↗(On Diff #275074)

On cppreference.com, fmin et al are listed under Basic operations. So, lets put their implementations in BasicOperations.h.

30 ↗(On Diff #275074)

After the NaN checks, can we just do:

return x > y ? y : x;
32 ↗(On Diff #275074)

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.

lntue updated this revision to Diff 276807.Thu, Jul 9, 12:34 PM

Add implementations of fmin, fminf, and fminl

lntue updated this revision to Diff 277092.Fri, Jul 10, 10:24 AM

Fix clang-format

lntue marked 2 inline comments as done.Fri, Jul 10, 10:25 AM

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.

libc/test/src/math/fminl_test.cpp
27

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.

41

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).

61

+1 for this arrangement.

63

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.

68

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);
lntue updated this revision to Diff 278027.Tue, Jul 14, 5:13 PM

Add implementations of fmin, fminf, and fminl. Update Test to accept floating point types and __uint128_t.

lntue marked 5 inline comments as done.Tue, Jul 14, 5:17 PM
lntue added inline comments.
libc/test/src/math/fminl_test.cpp
27

EXPECT_EQ and friends are now updated to accept __uint128 and floating point types.

lntue updated this revision to Diff 278077.Tue, Jul 14, 11:18 PM
lntue marked an inline comment as done.

Refactor utohexstr and overloads of raw_stream.operator<<

Still, overall LGTM. I have some inline comments for further chiseling. Also:

  1. Can you split the UnitTest changes into a separate patch?
  2. The variable naming style in Test.h and Test.cpp is old style like VarName. To keep the files uniform, please follow the existing style.
libc/utils/UnitTest/CMakeLists.txt
11 ↗(On Diff #278027)

I would prefer not to need this, especially for tests.

libc/utils/UnitTest/Test.cpp
111 ↗(On Diff #278027)

Isn't this always true? For, ValType is integral or floating point type anyway?

124 ↗(On Diff #278027)

Instead of printing hex values conditioned on printHex (which I think is always true), WDYT of this when streaming out LHS and RHS values:

<< "     Which is: " << describeValue(LHS) << '\n';

The function describeValue is a template function which does this:

  1. For integral types less than or of 64 bits, describeValue just returns the value as string.
  2. For 128 bit integers, it returns a string of hex bits.
  3. For floating point values, it returns a string like this: Sign: <1|0>, Exponent bits: <hex bits>, Manstissa Bits: <hex bits>
libc/utils/UnitTest/Test.h
82 ↗(On Diff #278027)

Instead of checking for __uint128_t, you should extend IsIntegral trait to include __uint128_t.

lntue updated this revision to Diff 278383.Wed, Jul 15, 11:51 PM

Add implementations of fmin, fminf, and fminl. Testing infrastructure update is splitted to https://reviews.llvm.org/D83931.

lntue marked 4 inline comments as done.Wed, Jul 15, 11:54 PM
lntue added inline comments.
libc/utils/UnitTest/Test.cpp
111 ↗(On Diff #278027)

There is also StringRef type being instantiated.

124 ↗(On Diff #278027)

There are also StringRef type being instantiated. I refactor this part and use template function describeValue to avoid constexpr in https://reviews.llvm.org/D83931

sivachandra accepted this revision.Thu, Jul 16, 10:08 AM

The tests look so much more readable now. Thanks for patiently working through the comments.
Just a note for future: We should setup differential fuzzers for math functions comparing LLVM libc results with results from system libc like glibc. And may be also a doc listing how we differ from glibc and the justification.

I have accepted this revision, but I guess you need to wait for the other patch to land before you can submit this. Also, I left couple nits inline. Please fix them if relevant.

libc/src/math/fminf.cpp
19

Newline?

libc/test/src/math/fmin_test.cpp
18

Here, and in the other test files, I don't think you need the static_cast. We have implicit conversion operators for FPBits.

This revision is now accepted and ready to land.Thu, Jul 16, 10:08 AM
sivachandra retitled this revision from Add implementations of fmin and fminf to [libc] Add implementations of fmin, fminf and fminl..Thu, Jul 16, 10:18 AM
sivachandra edited the summary of this revision. (Show Details)

I have updated the patch description. Please use the same when you submit via git.