[libc-commits] [PATCH] D127046: [libc][math] fmod/fmodf implementation.

[Kirill Okhotnikov via Phabricator via libc-commits]

orex added a comment.

Tue. I think, if we will not find agreement with ctz/clz syntax, we can ask Siva to solve the problem. Also, as long as you will use the function in the next changes, you can also try them and if it will not work good, improve it.



================
Comment at: libc/src/__support/FPUtil/builtin_wrappers.h:52
+
+template <typename T, bool CorrectZero = true> static inline int clz(T val) {
+  int result = __internal::clz<T>(val);
----------------
lntue wrote:
> In your use case, extra checks for 0 input are not needed, so commenting on the undefined behavior of `builtin_clz` when inputs are 0 is good enough.  A safe variant with extra checks could be added, but it should use different name.  One of the reason is that now if someone one to use `builtin_clz` without extra checks, they will have to specify the input type `T`:
> ```
>    clz<T, false>(...)
> ```
> So when the type `T` is known, or inside a non-template function, users cannot use type deduction: `clz<uint32_t, false>( ... )`.
> So in summary, my preference is that:
> - `clz` should just be a simple wrap around type matching for `__builtin_clz*`.
> - undefined behavior for inputs 0 can/should be documented.
> - safe versions can be added, but use different name so that type deduction can be applied.
I have a little bit different opinion for this case. If somebody would like to simply use `clz/ctz` he can use it without any complication and in a safe way. But, if somebody know, that he can improve the performance, skipping the test, it looks like OK to have such complications. For example. I don't see much difference between, adding new function have syntax like this `ctz<decltype(i), false>(i)`. Moreover such syntax can help to avoid implicit type conversion. Another point to use such template is to pass check zero parameter above easily. For example, make_value function can looks like `make_value<bool CheckZero>(...`. Of course it can be done with `if consexpr`, but template will be more straightforward.



================
Comment at: libc/test/src/math/FModTest.h:21
+#define TEST_SPECIAL(x, y, expected, dom_err, expected_exception)              \
+  EXPECT_EQ(FPBits(T(expected)).uintval(), FPBits(f(x, y)).uintval());         \
+  EXPECT_MATH_ERRNO((dom_err) ? EDOM : 0);                                     \
----------------
lntue wrote:
> You should be able to feed floating point type `T` directly to `expected`, and use `EXPECT_FQ_EQ` macro instead.
Thank you.


================
Comment at: libc/test/src/math/FModTest.h:36
+  void testSpecialNumbers(FModFunc f) {
+    using nl = std::numeric_limits<T>;
+
----------------
lntue wrote:
> orex wrote:
> > lntue wrote:
> > > Use `NumericLimits` template from `libc/src/__support/CPP/Limits.h`.
> > Unfortunately the library is very primitive. It does not have things, I need.
> Look like you only use it for some special constants.  With `std::numeric_limits`, you are actually using `<limits>` without directly including it which might cause error if some targets or configs that do not transitively include it.
> 
> It would be better to add those constants that you need to constant creating functions in `FPBits` class, and/or update the `DECLARE_SPECIAL_CONSTANTS` macro at https://github.com/llvm/llvm-project/blob/main/libc/utils/UnitTest/FPMatcher.h#L70 .  It could a separate patch that this one can depend on.
Can you explain your point, please. From my point of view, I just forget to `#include <limits>`. From another side, I have a feeling, from you comment, that I should not do this. Is it so? If yes, why. It is a test. Tests are "final" instances, so they can include whatever they want. Fre example, I've checked `exhaustive_test.cpp`. It includes "half of" STL. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046