[libc-commits] [PATCH] D130629: [libc] Change sinf range reduction to mod pi/16 to be shared with cosf.

[Kirill Okhotnikov via Phabricator via libc-commits]

orex added inline comments.


================
Comment at: libc/src/math/generic/range_reduction.h:82
 
-  return static_cast<int64_t>(k_hi + k_lo);
+  return static_cast<int64_t>(k_hi) + static_cast<int64_t>(k_lo);
 }
----------------
lntue wrote:
> orex wrote:
> > lntue wrote:
> > > orex wrote:
> > > > lntue wrote:
> > > > > orex wrote:
> > > > > > From my point of view, this line can be changed to `static_cast<int64_t>(k_hi + k_low)`, because `k_hi` and `k_low` are already "integer", so you can do one static cast instead of two. Probably it can increase performance.
> > > > > This is actually a must, since there are inputs which makes `k_hi < 2^54` and `k_hi + k_lo > 2^54`, causing rounding errors on the last integral bits due to rounding.  If we work it out carefully and adjust a bit, we might be able to avoid these rounding errors.  But for simplicity, I went with casting both to int64 in this patch.
> > > > Sorry be intrusive, but below you convert the result to `int`. Does such behavior is covered by C/C++ standard? When you tries to push signed value to type which can't hold it...
> > > I probably mixing up `int` and `int64_t` when moving things around.  But it is well-defined as truncation for 2-complement representations, and guaranteed to be equal modulo `2^(bit size of int)`.  So the end results are unchanged for us.
> > Sorry again, but according to this https://en.cppreference.com/w/cpp/language/implicit_conversion it is well defined only in C++20, but as I remember well we are aiming to C++17.
> Implementation-defined is not UB, so we are fine as long as `clang` and `gcc` have the same convention.  But anyway, feel free to remove the implicit conversion to `int` below to make it strictly conform to C++17.
If you know, that it is OK, I have no problem with it. Thank you.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D130629