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

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
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);
----------------
orex wrote:
> 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.
> 
So from the readability standpoint of both implementers and reviewers, I think `clz(x)` and `safe_clz(x)` or `clz(x)` and `unsafe_clz(x)' are better to understand than `clz(x)` and `clz<delctype(x), false>(x)`.
I'm also not a big fan of using boolean in template parameter, unless the template name makes it very clear what `true` and `false` mean.  If it's only used in one or a few tests, that's fine.  But this is on a utility header that will be used everywhere.  If we really want to pass these as template parameters, it's better to wrap them in functors, like `function<SafeClz>` vs `function<true>`.  Hope it makes sense?

Also, by changing the default `clz(x)` in this patch, you would need to update all current usages of it like in `sqrt` to the default version.


================
Comment at: libc/test/src/math/FModTest.h:36
+  void testSpecialNumbers(FModFunc f) {
+    using nl = std::numeric_limits<T>;
+
----------------
orex wrote:
> 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. 
For unit tests, we try to limits the use of `std::` and C++ standard header.  You can see other unit tests in `libc/test/src/math`.  Exhaustive tests are a bit different.  They are kind of integration tests, so we do not be so strict about that (yet).   So for example, `std::numeric_limit<>::quiet_NaN()` (most likely) simply call `__builtin_nan*`,  which technically what our library implements explicitly.  You don't have to do it now because this test currently does not have that problem.  But we should clean it up in the future if not now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046