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

Fri Jun 24 07:30:26 PDT 2022

lntue added inline comments.

================
Comment at: libc/src/__support/FPUtil/FPBits.h:173
+    // offset: +1 for sign, but -1 for implicit first bit
+    int lz = fputil::clz(number) - FloatProp::EXPONENT_WIDTH;
+    number <<= lz;
----------------
orex wrote:
> sivachandra wrote:
> > orex wrote:
> > > lntue wrote:
> > > > lntue wrote:
> > > > > orex wrote:
> > > > > > orex wrote:
> > > > > > > sivachandra wrote:
> > > > > > > > lntue wrote:
> > > > > > > > > orex wrote:
> > > > > > > > > > lntue wrote:
> > > > > > > > > > > For safety, you might need to add a quick return for when `number == 0`:
> > > > > > > > > > > ```
> > > > > > > > > > >   if (unlikely(number == 0)) return zero ...
> > > > > > > > > > > ```
> > > > > > > > > > It was Siva suggestion to move the function here. There is another class called `NormalFloat` which can handle such things. I propose to move the function back to FMod and implement "full" functionality in that class. What do you think? `if number == 0` is not only one case which we need to check. `ep` value after processing below should also be checked for overflow. 
> > > > > > > > > The main reason you need to check for `number == 0` here is because `fputil::clz` is a wrapper around `__built_in_clz` and when the inputs are 0, the behavior/output is undefined.
> > > > > > > > I am unable to locate the context of my suggestion. Can you help me find it?
> > > > > > > Sorry for bothering you. I mixed your suggestion to extract "FBits improvements" and Tue particular suggestion. After new push all old comments are moved to new places for me, so It was difficult for me to figure it out.
> > > > > > I never call `ctz` or `clz` with zero in fmod case. If the functions now in the separate module, probably we should check zero there? It can have a performance impact, of course, but may be compiler can optimized out. If you always want to check it, I think that it is better to check it there? We can also think about best way to check it. My proposition is to move the function `make_value` back to FMod and add x == 0 check in ctz/clz functions (probably separate commit).
> > > > > You can use `https://reviews.llvm.org/DXXXXXX/new/` instead of `https://reviews.llvm.org/DXXXXXX`.  That should separate old and new comments better.
> > > > Now that this function `make_value` is refactored to stay under `FPBits` class, which might be used beyond `fmod`.  So you will need to either add some comments explicitly stating its non-zero input assumption, or adding an extra check to make sure that it won't silently return wrong outputs when being used by other functions.
> > > Don't you think, that we need to mention this in clz/ctz builtin wrappers?
> > > Don't you think, that we need to mention this in clz/ctz builtin wrappers?
> > 
> > Yes. I think handling the corner case as reasonable should be done within our wrappers. We should not ideally have to pepper user code with checks.
> Thank you for support Siva. I've implemented it in a way it check the case by default. 
Sorry that I confused you and Siva!  What I meant was that since the function `make_value` had undefined behavior when the input is 0, and it was put in the common library, it should either:
- be documented
- add extra checks for 0.
Since you already documented it in item #4, that's good enough for me, and adding extra checks is not needed anymore since there are legitimate usage of `make_value` and `clz` when inputs are guaranteed to be non-zero such as in this patch and in other places.  You can definitely add extra comments to document the undefined behavior of `clz` when inputs are 0.

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

================
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);                                     \
----------------
You should be able to feed floating point type `T` directly to `expected`, and use `EXPECT_FQ_EQ` macro instead.

================
Comment at: libc/test/src/math/FModTest.h:36
+  void testSpecialNumbers(FModFunc f) {
+    using nl = std::numeric_limits<T>;
+
----------------
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.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046