[libc-commits] [PATCH] D84725: [libc] Add ULP function to MPFRNumber class to test correctly rounded functions such as SQRT, FMA.

[Siva Chandra via Phabricator via libc-commits]

sivachandra added inline comments.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:172
+  // ULP(mpfr_value, value) = abs(mpfr_value - value) / eps(value)
+  // ULP < 0.5 will imply that the value is correctly rounded.
+  template <typename T>
----------------
Few comments about documentation.

1. I think this function assumes that the exponent of `input` matches that of the MPFR number. If yes, document it here?
2. Explain why a `double` return value is good enough?
3. Lastly, is the name `ulp` of this method correct? I guess, because you defined ULP above as a ratio, the name `ulp` is correct and is OK as is.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:200
 
+template <typename T> T toFloatingPoint(MPFRNumber mpfrInput);
+
----------------
Instead of non-member functions, can we replace the existing methods with a template method:

```
template <typename T>
T as() const;
```

We will still need specializations like below though.


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:230
+  __llvm_libc::fputil::testing::describeValue(
+      "   MPFR rounded: ", toFloatingPoint<T>(mpfrResult), OS);
+  OS << '\n';
----------------
In what circumstances would a rounded MPFR result help understand a test failure?


================
Comment at: libc/utils/MPFRWrapper/MPFRUtils.cpp:233
+  if (useULP) {
+    OS << "     ULP errors: " << std::to_string(mpfrResult.ulp(matchValue))
+       << '\n';
----------------
Is plural `errors` correct here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D84725