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.

Sign up for GitHub

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 gcd/lcm effect-free by using LazyString #44935

Merged
merged 5 commits into from
Apr 12, 2022
Merged

Make gcd/lcm effect-free by using LazyString #44935

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
ea8e90e
Make gcd effect-free by using LazyString
tkf Apr 11, 2022
ab20831
Workaround a bootstrapping issue
tkf Apr 11, 2022
c137496
Make checked_abs effect-free
tkf Apr 11, 2022
ddfca0d
Test that gcd is effect-free
tkf Apr 11, 2022
2c2f43e
Make lcm effect-free
tkf Apr 11, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 5 additions & 4 deletions base/checked.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -115,9 +115,10 @@ function checked_abs end

function checked_abs(x::SignedInt)
r = ifelse(x<0, -x, x)
r<0 && throw(OverflowError(string("checked arithmetic: cannot compute |x| for x = ", x, "::", typeof(x))))
r
end
r<0 || return r
msg = LazyString("checked arithmetic: cannot compute |x| for x = ", x, "::", typeof(x))
throw(OverflowError(msg))
end
checked_abs(x::UnsignedInt) = x
checked_abs(x::Bool) = x

Expand Down Expand Up @@ -151,7 +152,7 @@ end


throw_overflowerr_binaryop(op, x, y) = (@noinline;
throw(OverflowError(Base.invokelatest(string, x, " ", op, " ", y, " overflowed for type ", typeof(x)))))
throw(OverflowError(LazyString(x, " ", op, " ", y, " overflowed for type ", typeof(x)))))
Comment on lines -154 to +155
Copy link
Member Author

@tkf tkf Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This invokelatest was added in #30566 for reducing the latency. But, since with LazyString we also avoid the compilation of print of each interpolated values, I'd guess it has a similar effect?

Though I wonder if we need @nospecialize in the constructor?

julia/base/strings/lazy.jl

Line 28 in 0deb326

LazyString(args...) = new(args)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, LazyString should hide the inference cost for printing values at a time when we compile the parent code containing LazyString constructor (and the deferred printing will essentially be dynamically dispatched as like invokelatest).

We generally don't need @nospecialize annotation when a generic function has only a few # of methods and the method body is very simple. But it won't hurt anything (at least at this moment) if we add @nospecialize for this constructor anyway.


"""
Base.checked_add(x, y)
Expand Down
3 changes: 2 additions & 1 deletion base/intfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,8 @@ function gcd(a::T, b::T) where T<:BitInteger
signbit(r) && __throw_gcd_overflow(a, b)
return r
end
@noinline __throw_gcd_overflow(a, b) = throw(OverflowError("gcd($a, $b) overflows"))
@noinline __throw_gcd_overflow(a, b) =
throw(OverflowError(LazyString("gcd(", a, ", ", b, ") overflows")))

# binary GCD (aka Stein's) algorithm
# about 1.7x (2.1x) faster for random Int64s (Int128s)
Expand Down
7 changes: 7 additions & 0 deletions test/intfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,8 @@

using Random

is_effect_free(args...) = Core.Compiler.is_effect_free(Base.infer_effects(args...))

@testset "gcd/lcm" begin
# All Integer data types take different code paths -- test all
# TODO: Test gcd and lcm for BigInt.
Expand Down Expand Up @@ -146,6 +148,11 @@ using Random
@test gcd(0xf, 20) == 5
@test gcd(UInt32(6), Int8(-50)) == 2
@test gcd(typemax(UInt), -16) == 1

@testset "effects" begin
@test is_effect_free(gcd, Tuple{Int,Int})
@test is_effect_free(lcm, Tuple{Int,Int})
end
end

@testset "gcd/lcm for arrays" begin
Expand Down