Skip to content

Conversation

nsajko
Copy link
Member

@nsajko nsajko commented May 6, 2023

Without this change prevpow and nextpow fail for, e.g., BigInt; incorrectly throwing a MethodError. For example, calls like prevpow(3, big"10") or nextpow(3, big"10") fail.

The issue is that the code incorrectly assumes the existence of a typemax method.

Another issue was a missing promote for the arguments of a mul_with_overflow call.

Fixes #57906

@Seelengrab
Copy link
Contributor

Seelengrab commented May 7, 2023

applicable requires a dynamic lookup, which means this implementation cannot be constant folded anymore, if I'm not mistaken:

julia> Base.infer_effects(prevpow, (Int, Int))
(+c,+e,!n,+t,+s,+m,+i)

julia> Base.infer_effects(prevpow_new, (Int, Int))
(!c,!e,!n,!t,!s,!m,+i)′

Note how the current version is consistent (+c), free of sideeffects (+e), always terminates (+t), doesn't access task local state (+s) and only uses memory inaccessible to other functions (+m). In contrast, the new version taints all these effects, making it non-constant foldable for all argument types. See ??Base.@assume_effects for more information on what these effects mean in detail.

I think it'll be better to write a specialized method for BigInt (or use the corresponding GMP function, if that exists) in base/gmp.jl if we want to support BigInt in these functions. Such a version would also have the advantage of being able to reuse intermediary memory of the various internal BigInt allocations, which this version can't do (and thus allocates a lot). The approach I'd take here is to have prevpow(x, y::BigInt) = prevpow(big(x), y) (and likewise for BigInt in the first argument position), and then implement _prevpow(x::BigInt, y::BigInt), using the fact that x is in our controlled allocation to mutate x in-place with the Base.GMP.MPZ API, and return that.

@nsajko
Copy link
Member Author

nsajko commented May 7, 2023

Oof, OK. I guess this means that digits! and invmod also need fixing. They use hastypemax, which uses applicable.

@Seelengrab
Copy link
Contributor

Seelengrab commented May 7, 2023

No, hastypemax works on the type, which is known for the important constant foldable cases - it compiles away for e.g. Int, which is specialized to just return true (check @edit Base.hastypemax(Int)). Since pretty much every operation with BigInt needs the runtime for allocations anyway, I guess that's why it's used there as a fallback? You could try to use that here as well, good catch.

Regardless, if performance for BigInt is the goal, an in-place method will likely be required.

@nsajko nsajko marked this pull request as draft August 2, 2023 22:09
@nsajko nsajko marked this pull request as ready for review August 2, 2023 22:22
@nsajko nsajko changed the title fix nextpow, prevpow for types without typemax, mul_with_overflow fix nextpow, prevpow for types without typemax Aug 2, 2023
@nsajko
Copy link
Member Author

nsajko commented Aug 3, 2023

Ping. The failures are system-dependent and surely unrelated.

@brenhinkeller brenhinkeller added the maths Mathematical functions label Aug 6, 2023
@oscardssmith
Copy link
Member

@Seelengrab are your concerns resolved?

@Seelengrab
Copy link
Contributor

Seelengrab commented Aug 8, 2023

If BigInt performance is not a concern, yes, this version seems good. I'll maintain though that explicit BigInt support with promotion at the start & in-place operations would be better, but it's not blocking the PR.

@nsajko nsajko added the bugfix This change fixes an existing bug label Mar 27, 2025
@nsajko
Copy link
Member Author

nsajko commented Mar 27, 2025

If BigInt performance is not a concern, yes, this version seems good. I'll maintain though that explicit BigInt support with promotion at the start & in-place operations would be better, but it's not blocking the PR.

BigInt is a red herring here, this PR is a bugfix to (more or less) generic code, where BigInt just happens to be an example where the code is currently broken. A separate method for BigInt, as a performance optimization, is clearly completely off-topic here.

@nsajko nsajko added backport 1.10 Change should be backported to the 1.10 release backport 1.11 Change should be backported to release-1.11 backport 1.12 Change should be backported to release-1.12 labels Mar 27, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue, revealed after fixing the previous one, was a missing
`promote` for the arguments of a `mul_with_overflow` call.

Fixes JuliaLang#57906
@nsajko nsajko force-pushed the prevpow_typemax branch from b33b8b7 to 1ce3d6c Compare April 3, 2025 15:14
@nsajko
Copy link
Member Author

nsajko commented Apr 3, 2025

bump

@nsajko nsajko added the merge me PR is reviewed. Merge when all tests are passing label Apr 3, 2025
@IanButterworth IanButterworth merged commit 3627a85 into JuliaLang:master Apr 4, 2025
8 checks passed
@oscardssmith oscardssmith removed the merge me PR is reviewed. Merge when all tests are passing label Apr 4, 2025
@nsajko nsajko deleted the prevpow_typemax branch April 4, 2025 08:13
KristofferC pushed a commit that referenced this pull request Apr 4, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue was a missing promote for the arguments of a
`mul_with_overflow` call.

Fixes #57906

(cherry picked from commit 3627a85)
@KristofferC KristofferC mentioned this pull request Apr 4, 2025
51 tasks
KristofferC pushed a commit that referenced this pull request Apr 4, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue was a missing promote for the arguments of a
`mul_with_overflow` call.

Fixes #57906

(cherry picked from commit 3627a85)
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Apr 10, 2025
@KristofferC KristofferC removed the backport 1.12 Change should be backported to release-1.12 label Apr 25, 2025
KristofferC pushed a commit that referenced this pull request Jun 4, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue was a missing promote for the arguments of a
`mul_with_overflow` call.

Fixes #57906

(cherry picked from commit 3627a85)
@KristofferC KristofferC mentioned this pull request Jun 4, 2025
75 tasks
KristofferC pushed a commit that referenced this pull request Jun 5, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue was a missing promote for the arguments of a
`mul_with_overflow` call.

Fixes #57906

(cherry picked from commit 3627a85)
KristofferC pushed a commit that referenced this pull request Jul 3, 2025
Without this change `prevpow` and `nextpow` fail for, e.g., `BigInt`;
incorrectly throwing a `MethodError`. For example, calls like
`prevpow(3, big"10")` or `nextpow(3, big"10")` fail.

The issue is that the code incorrectly assumes the existence of a
`typemax` method.

Another issue was a missing promote for the arguments of a
`mul_with_overflow` call.

Fixes #57906

(cherry picked from commit 3627a85)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport 1.10 Change should be backported to the 1.10 release bignums BigInt and BigFloat bugfix This change fixes an existing bug maths Mathematical functions

Projects

None yet

Development

Successfully merging this pull request may close these issues.

prevpow errors on some BigInt/BigFloat input combinations

7 participants