-
Notifications
You must be signed in to change notification settings - Fork 2
Direct rounding for many operators #126
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
pavpanchekha
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The biggest issue here is the allocation of mutable bigfloat values; we can talk more in meeting today.
A secondary issue is that this is just a huge PR, difficult to review (I skipped reviewing the trig portion). Ideally we'd break it up into pieces.
Finally, I notice that when we use this branch in Herbie runs, the bear benchmark claims "no valid values"; that can't be right and suggests there's a bug somewhere.
ops/core.rkt
Outdated
| (define (ival-max-prec x) | ||
| (max (bigfloat-precision (ival-lo-val x)) (bigfloat-precision (ival-hi-val x)))) | ||
|
|
||
| (define (ival-exact-fabs x) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is fine and all but realistically we should either 1) rewrite the callers to not call these methods, or 2) rewrite these methods into ! variations, in which case the caller decides the precision anyway so these become normal ival-abs! and ival-neg! methods.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My understanding is that means replacing something like this:
(define x* (ival-exact-fabs x))with
(define x* (parameterize ([bf-precision (ival-max-prec x)]) (ival-fabs x)))This seems unnecessarily verbose to me, or am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My point is that we're often doing ival-exact-fabs in the caller (like hypot) only because we want the max/min logic, not because we actually need to "compute" the fabs. So probably in many cases we can skip actually "computing" fabs (it's just copying bits anyway) if we write the caller in a smarter way.
…ival into direct-rounding-mode
|
Totally fine to leave this PR up if you want to use it as a "cheat sheet" to copy from, but once you're done with it, please close it so we don't just have it hanging around forever. (Even when closed, it's still accessible, so you could even close it now and keep using the PR.) |
Implements rounding directly using mpfr instead of bigfloat in a number of functions, including all of the major ones. Reduces memory usage on the rival benchmark by 13.6%. Eliminates the use of
rndin most places. I also tried removing the use ofparameterizefor settingbf-precision(e.g.ival-exact-fabs) like this:However, I was reading an article which suggested using
dynamic-wind. We could consider using this to replaceparameterize?