Skip to content
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

make @fastmath mirror how lowering applies literal_pow #53819

Merged
merged 11 commits into from
Apr 3, 2024

Conversation

KlausC
Copy link
Contributor

@KlausC KlausC commented Mar 22, 2024

The expressions a^x and @fastmath a^x are now producing equivalent results (apart from floating point accuracy) in the case of literal integer x.
The logic in the fastmath macro, trying to mimic the behaviour of the compiler is fixed.

Fixes #53817

@Seelengrab
Copy link
Contributor

Per CONTRIBUTING.md, please provide a good description of what the PR does:

Descriptive commit messages are good.

It's very cumbersome to have to go through three layers of indirection when looking at the history of a file (the change itself, this PR, and finally the issue itself) to figure out what exactly was broken with @fastmath in conjunction with literal_pow.

@KlausC
Copy link
Contributor Author

KlausC commented Mar 22, 2024

It's very cumbersome to have to go through three layers of indirection

Sorry for the inconvenience - I wanted to avoid repeating the error description for the issue. I added a description of what has been changed.

@Seelengrab
Copy link
Contributor

Thank you!

@KlausC KlausC marked this pull request as draft March 23, 2024 15:38
@KlausC KlausC marked this pull request as ready for review March 23, 2024 15:38
@ViralBShah ViralBShah added the maths Mathematical functions label Mar 25, 2024
@oscardssmith
Copy link
Member

LGTM, although it would probably be good to add performance results to the PR, and ideally an example to the base benchmarks so we can see if this regresses.

@KlausC
Copy link
Contributor Author

KlausC commented Mar 26, 2024

add performance results to the PR

No performance changes are expected during runtime, because only the macro logic is changed.

The cases, which behave different do not exhibit changed performance figures. The 4th example in the changed
version now throws DomainError, like the corresponding non-fastmath version.

For the cases, which now create different runtime code:

NEW: Version 1.12.0-DEV.233 (2024-03-22) krc/fastmath-literalpow/8615c0bdff

julia> foop1(a) = @fastmath a ^ 2305843009213693951
foop1 (generic function with 1 method)

julia> foop2(a) = @fastmath a ^ 2305843009213693952
foop2 (generic function with 1 method)

julia> foom2(a) = @fastmath a ^ -2305843009213693952
foom2 (generic function with 1 method)

julia> foom3(a) = @fastmath a ^ -2305843009213693953
foom3 (generic function with 1 method)

julia> a = 2
2

julia> @btime foop1($a)
  113.349 ns (0 allocations: 0 bytes)
0

julia> @btime foop2($a)
  24.203 ns (0 allocations: 0 bytes)
0

julia> @btime foom2($a)
  209.300 ns (0 allocations: 0 bytes)
0.0

julia> @btime foom3($a)
ERROR: DomainError with -2305843009213693953:

OLD: Julia Version 1.12.0-DEV.225

julia> @btime foop1($a)
  111.230 ns (0 allocations: 0 bytes)
0

julia> @btime foop2($a)
  24.192 ns (0 allocations: 0 bytes)
0

julia> @btime foom2($a)
  209.556 ns (0 allocations: 0 bytes)
0.0

julia> @btime foom3($a)
  209.655 ns (0 allocations: 0 bytes)
0.0

@oscardssmith
Copy link
Member

Looks good to me.

@mbauman mbauman changed the title @fastmath literal_pow fix make @fastmath mirror Julia's literal_pow limit of 2^±61 Mar 26, 2024
@mbauman
Copy link
Sponsor Member

mbauman commented Mar 26, 2024

My only hesitation — both here and in #53713 is that this really seems to be a bug in the Julia-flisp interface that doesn't match the expected and documented behaviors. Ideally we'd fix that interface. But I don't know how to do that and this is better than nothing.

@mbauman mbauman changed the title make @fastmath mirror Julia's literal_pow limit of 2^±61 make @fastmath mirror Julia's literal_pow limit of ±2^61 Mar 26, 2024
@KlausC
Copy link
Contributor Author

KlausC commented Mar 26, 2024

As soon as the flisp part is fixed, the limits introduced in #53713 and here have to be removed.

@mbauman mbauman changed the title make @fastmath mirror Julia's literal_pow limit of ±2^61 make @fastmath mirror how lowering applies literal_pow Mar 26, 2024
@KlausC KlausC marked this pull request as draft March 28, 2024 19:00
@KlausC KlausC marked this pull request as ready for review April 1, 2024 10:04
@KlausC
Copy link
Contributor Author

KlausC commented Apr 1, 2024

This PR is now finished and can be merged if no objections.

@mbauman mbauman merged commit d7dc9a8 into JuliaLang:master Apr 3, 2024
7 checks passed
@KlausC KlausC deleted the krc/fastmath-literalpow branch April 3, 2024 21:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

@fastmath and literal_pow not compatible
5 participants