[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/UInt.h:24
 
-template <size_t Bits> class UInt {
+template <size_t Bits> struct UInt {
 
----------------
lntue wrote:
> sivachandra wrote:
> > lntue wrote:
> > > michaelrj wrote:
> > > > is there a reason this is changed?
> > > I put everything in this class as `public` for 3 reasons:
> > > - To access to `WordCount` without recomputing it
> > > - To make this class behave as a value class, I'm going to remove the `operator[]`, and simply use `.val[i]` directly if we want to access to its ith `uint64_t` block.
> > > - Utility functions `low` and `high` (and `MASK32` constant) will be removed and switched completely to use `split(uint64_t)` instead.  That would separate what we want to do with the class vs what we want to do with `uint64_t`.
> > > I put everything in this class as `public` for 3 reasons:
> > > - To access to `WordCount` without recomputing it
> > 
> > You can make that public alone?
> > 
> > > - To make this class behave as a value class, I'm going to remove the `operator[]`, and simply use `.val[i]` directly if we want to access to its ith `uint64_t` block.
> > 
> > Whats wrong with `operator[]` for such word access?
> > 
> > > - Utility functions `low` and `high` (and `MASK32` constant) will be removed and switched completely to use `split(uint64_t)` instead.  That would separate what we want to do with the class vs what we want to do with `uint64_t`.
> > 
> > I did not get how this is related to `class` vs `struct`. 
> It's just that accessing a specific `uint64_t` block is clearer with `.val[i]` vs `[i]`, especially when we have an array of `UInt`.  And with `low` and `high` functions being factored out to `number_pair.h`, there is no need for `private` members, and we can simply keep everything `public`.
I don't find the reasoning compelling enough to not hide the implementation detail. But, there is not a lot of implementation detail so I am on the fence.


================
Comment at: libc/src/__support/UInt.h:296
+      if (s >= 128) {
+        a.val[0] = 0;
+        a.val[1] = 0;
----------------
What is `a` ?


================
Comment at: libc/src/__support/UInt.h:541
 template <> struct is_integral<UInt<128>> : public cpp::true_type {};
+template <> struct is_integral<UInt<192>> : public cpp::true_type {};
+template <> struct is_integral<UInt<256>> : public cpp::true_type {};
----------------
I think we can make this a template like this:

```
template <size_t S>
struct is_integral<UInt<S>> : public cpp::true_type { static_assert(valid S); };
```


================
Comment at: libc/src/__support/UInt.h:545
+// Provides is_unsigned of UInt<128>, UInt<192>, UInt<256>.
+template <> struct is_unsigned<UInt<128>> : public cpp::true_type {};
+template <> struct is_unsigned<UInt<192>> : public cpp::true_type {};
----------------
We can use a template instead of a specialization here also.


================
Comment at: libc/src/__support/number_pair.h:23
+
+using DoubleDouble = number_pair<double>;
+
----------------
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; }
};
```


================
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:
> > 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D137871