[PATCH] D69776: [APFloat] Fix FP remainder operation

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I'd like to see more test coverage for denormal denominators, and denominators close to the max finite float.



================
Comment at: llvm/lib/Support/APFloat.cpp:1753
+  // We want to make sure the current value is less than twice the denom rhs.
+  if (rhs.isFinite()) {
+    IEEEFloat P2 = rhs;
----------------
Can we return early if rhs.isFinite() is false?  The following code might work for rhs==infinity, but it makes everything more complicated to reason about.


================
Comment at: llvm/lib/Support/APFloat.cpp:1755
+    IEEEFloat P2 = rhs;
+    if (P2.add(rhs, rmNearestTiesToEven) == opOK) {
+      fs = mod(P2);
----------------
If the add is not opOK, that means the add returned opOverflow, so the value is already less than twice rhs?  I guess that's right, but it would be nice to spell it out.


================
Comment at: llvm/lib/Support/APFloat.cpp:1770
+    return opOK;
+  }
 
----------------
Is the special case for `compare(P) == cmpEqual` necessary?


================
Comment at: llvm/lib/Support/APFloat.cpp:1807
+  IEEEFloat P2 = P;
+  if (P2.divide(two, rmNearestTiesToEven) == opOK) {
+    if (compare(P2) == cmpGreaterThan) {
----------------
If this check fails, that means `P2` is denormal?

Can you make this work for denormal cases by just fiddling with the exponent of `P2`, instead of doing a proper divide?  The resulting APFloat might not be properly normalized, but that doesn't matter here.


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

https://reviews.llvm.org/D69776