This is an archive of the discontinued LLVM Phabricator instance.

[APFloat] Fix FP remainder operation
ClosedPublic

Authored by ekatz on Nov 3 2019, 1:02 PM.

Details

Summary

Fix IEEEFloat::remainder() function.

Fix PR3359.

Diff Detail

Event Timeline

ekatz created this revision.Nov 3 2019, 1:02 PM
ekatz updated this revision to Diff 227811.Nov 4 2019, 7:40 PM

Rebased to master.

Thanks for posting patches to fix problems with the APFloat implementation. FWIW, this code is hard to review, from my perspective, because it lacks comments (both your new code and the code it is replacing). As you clearly have gone through this and understand what it's doing, could you add a bunch of comments explaining what's going on? I think that would be very helpful for everyone else.

arsenm added inline comments.Nov 19 2019, 7:25 AM
llvm/unittests/ADT/APFloatTest.cpp
3418

These should be swapped due to a gtestism (same for the rest, it should be expected, actual)

ekatz updated this revision to Diff 230805.Nov 24 2019, 10:04 AM

Added comments and fix tests to meet gtestism.

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

llvm/lib/Support/APFloat.cpp
1823

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.

1825

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.

1881

Is the special case for compare(P) == cmpEqual necessary?

1926

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.

ekatz updated this revision to Diff 243655.Feb 10 2020, 12:39 PM

Simplified logics according to the notes.
Also added more extensive tests.

@efriedma , I updated the files according to your requests.

efriedma added inline comments.Feb 10 2020, 4:32 PM
llvm/lib/Support/APFloat.cpp
1818

The opDivByZero thing is sort of confusing. Maybe just something like if (!isFinite() || !rhs.isFinite()) return remainderSpecials(rhs);?

1899

Are all the subtractions here guaranteed to produce exact results? If they are, would it make sense to assert that?

ekatz marked an inline comment as done.Feb 10 2020, 9:09 PM
ekatz added inline comments.
llvm/lib/Support/APFloat.cpp
1818

I thought about that, but I just used the same convention as in addOrSubtractSpecials.

If we were to add this if statement, then we are actually handling specials outside the function that supposed to do exactly that. And that alone makes even less sense than the strange opStatus (which is commented, and used only internally).

It would have made more sense to add a new opStatus just for that case, but it seems a bit too much, I guess (at least for those internal functions).

Which is why I decided to take this approach.

ekatz updated this revision to Diff 243820.Feb 11 2020, 5:03 AM

Added asserts for the opStatus returned by the operations.

ekatz marked an inline comment as done.Feb 11 2020, 5:04 AM
This revision is now accepted and ready to land.Feb 11 2020, 2:33 PM
This revision was automatically updated to reflect the committed changes.