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

[Kirill Okhotnikov via Phabricator via libc-commits]

orex 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;
----------------
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. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127046