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

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/src/__support/number_pair.h:23
+
+using DoubleDouble = number_pair<double>;
+
----------------
lntue wrote:
> 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.
Ah! If the normal convention is to use `hi` and `lo` for the main and delta components of a double-double, then having a single definition is OK.


================
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,
----------------
lntue wrote:
> 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` ?
Yes, like other custom matchers we have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871