Skip to content

Adding tuning formulas for tanu, cosu and sinu, fix #112#114

Merged
pavpanchekha merged 4 commits intomainfrom
fix-112
Apr 16, 2025
Merged

Adding tuning formulas for tanu, cosu and sinu, fix #112#114
pavpanchekha merged 4 commits intomainfrom
fix-112

Conversation

@AYadrov
Copy link
Contributor

@AYadrov AYadrov commented Apr 15, 2025

This PR introduces changes to Rival's exponent tricks.
Particularly speaking, tuning formulas for tanu, cosu and sinu have been added.
It is quite interesting that we never ran into this before. One of the reasons why is that Linux uses an older version of MPFR - and we mostly use Linux.
The bug was discovered by @JonasRegehr when using Rival on MacOS.
The new exponent tricks are pretty straight forward, but the fact that the angle for tanu, etc. is not stored as an ival makes the code slightly tricky, as this angle needs to be stored somewhere since it participates in precision tuning.
I store this angle constant into a new rival-machine field called constant-lookup - this is a vector to where we can add constants like this now on that do not participate in evaluation, but do participate in precision tuning.

@AYadrov AYadrov requested a review from pavpanchekha April 15, 2025 22:44
[(list `(sinu ,n) x) (list (ival-sinu n) x)]
[(list `(tanu ,n) x) (list (ival-tanu n) x)]
[(list `(cosu ,n) x)
(when i
Copy link
Contributor Author

@AYadrov AYadrov Apr 15, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We also export this fn->ival-fn to baseline, and baseline does not need to know about these constants, therefore, an ugly fix for that is to use it only in Rival

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not too worried about baseline.

(list (ival-sinu n) x)]
[(list `(tanu ,n) x)
(when i
(vector-set! constants i (exact-floor (log n))))
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here I execute the constant for angle to be (exact-floor (log n)). We do it only once at compile time and seems to be fine? rather than do this every tuning pass we can do it once.
It slows down Rival's compilation and if someone will use Rival for really few points per expressions - probably they better not to optimize the original expressions. Thus, cosu, tanu, sinu will not arise at all

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I think this doesn't matter very much either way, it just can't really be that slow.

By the way, we might want integer-length instead of log? It might be faster?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, we can, I tried to find a cheaper way, maybe integer-length indeed can work

@AYadrov AYadrov changed the title Adding tuning formulas for tanu, cosu and sinu Adding tuning formulas for tanu, cosu and sinu, fix #112 Apr 15, 2025
[(list `(sinu ,n) x) (list (ival-sinu n) x)]
[(list `(tanu ,n) x) (list (ival-tanu n) x)]
[(list `(cosu ,n) x)
(when i
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm not too worried about baseline.

(list (ival-sinu n) x)]
[(list `(tanu ,n) x)
(when i
(vector-set! constants i (exact-floor (log n))))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sure. I think this doesn't matter very much either way, it just can't really be that slow.

By the way, we might want integer-length instead of log? It might be faster?

@pavpanchekha pavpanchekha merged commit 6b4492a into main Apr 16, 2025
1 check passed
@pavpanchekha pavpanchekha deleted the fix-112 branch April 16, 2025 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants