[libc-commits] [PATCH] D137871: [libc][math] Improve the performance and error printing of UInt.

[Tue Ly via Phabricator via libc-commits]

lntue added inline comments.


================
Comment at: libc/src/__support/UInt.h:296
+      if (s >= 128) {
+        a.val[0] = 0;
+        a.val[1] = 0;
----------------
sivachandra wrote:
> What is `a` ?
Thanks for finding the bug!  It turns out that I missed extra `__` at the end of the macro in `#ifdef`, hence dead-code here.


================
Comment at: libc/src/__support/number_pair.h:23
+
+using DoubleDouble = number_pair<double>;
+
----------------
sivachandra wrote:
> Few questions:
> 1. Is this the only reason why the rest of the content from this file is not in `integer_utils.h`?
> 2. Is `lo`, `hi` split for a `DoubleDouble` the correct nomenclature?
> 
> I am still not convinced of the different styles used here. For example, why is `number_pair` not `NumberPair` or `DoubleDouble` not `double_double`? We should follow a uniform style. The exceptions are only applicable for the `cpp` directory. So, I suggest this:
> 
> 1. Add a simple `cpp::pair<T1, T2>` class.
> 2. Instead of `number_pair`, define `IntegerSplit`:
> 
> ```
> template <typename T>
> class IntegerSplit : protected cpp::pair<T, T> {
> public:
>   IntegerSplit() = default;
>   IntegerSplit(T l, T h) : first(l), second(h) {}
>   T &lo() { return cpp::pair<T, T>::first; }
>   T &hi() { return cpp::pair<T, T>::second; }
>   const T &lo() const { return cpp::pair<T, T>::first; }
>   const T &hi() const { return cpp::pair<T, T>::second; }
> };
> ```
> 
> 3. Not in this patch, but when required:
> 
> ```
> class DoubleDouble : protected cpp::pair<double, double> {
> public:
>   DoubleDouble() = default;
>   Double(double m, double d) : cpp::pair<T, T>(m, d) {}
>   double &main() { return cpp::pair<T, T>::first; }
>   double &delta() { return cpp::pair<T, T>::second; }
>   const double &main() const { return cpp::pair<T, T>::first; }
>   const double &delta() const { return cpp::pair<T, T>::second; }
> };
> ```
Yes, I think the normal splitting in the literature with double-double data type will call high/low parts, with abbreviations hi/lo.  I do update the style name to `NumberPair`, but `IntegerSplit` does not seem fit, since it could be more generic and applied to `double` to get `DoubleDouble` type.

On the other hand, I don't see any benefit of a more complicated class structure over a simple POD `struct` for this yet.  If there is a need for them, I'd be happy to update its definition.


================
Comment at: libc/utils/UnitTest/LibcTest.cpp:281
 
+template bool test<__llvm_libc::cpp::UInt<192>>(
+    RunContext *Ctx, TestCondition Cond, __llvm_libc::cpp::UInt<192> LHS,
----------------
sivachandra wrote:
> lntue wrote:
> > sivachandra wrote:
> > > Ideally, these specializations should be in a different file, say `integer_test_utils.cpp`, right next to where `UInt` is defined. May be it leads to a CMake cyclic directory problem?
> > Since we `UInt<128>` is required here as replacement for `__uint128_t` if not available, instantiate other `UInt` here is the most suitable, without complicating the dependency, I think.
> We can keep specializations of only standard types here. Everything else, including those for `__uint128_t`, should go to another file.
Does it also mean that by default we will need to link to the other file, in addition to `LibcTest` ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871