-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
RFC: address issue #20882 #20889
RFC: address issue #20882 #20889
Changes from 6 commits
35b0543
08c6ccd
75c33fa
e228123
09e5b1b
032a0af
07bc45f
e703ae4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -195,29 +195,28 @@ end | |
^(x::Number, p::Integer) = power_by_squaring(x,p) | ||
^(x, p::Integer) = power_by_squaring(x,p) | ||
|
||
# x^p for any literal integer p is lowered to x^Val{p}, | ||
# x^p for any literal integer p is lowered to Base.literal_pow(^, x, Val{p}) | ||
# to enable compile-time optimizations specialized to p. | ||
# However, we still need a fallback that calls the general ^. | ||
# To avoid ambiguities for methods that dispatch on the | ||
# first argument, we dispatch the fallback via internal_pow. | ||
# However, we still need a fallback that calls the function ^ which may either | ||
# mean Base.^ or something else, depending on context. | ||
# We mark these @inline since if the target is marked @inline, | ||
# we want to make sure that gets propagated, | ||
# even if it is over the inlining threshold. | ||
@inline ^(x, p) = internal_pow(x, p) | ||
@inline internal_pow{p}(x, ::Type{Val{p}}) = x^p | ||
@inline literal_pow{p}(^, x, ::Type{Val{p}}) = ^(x,p) | ||
|
||
# Restrict inlining to hardware-supported arithmetic types, which | ||
# are fast enough to benefit from inlining. This also makes it | ||
# easier to override ^ without having to override the Val method. | ||
# are fast enough to benefit from inlining. | ||
const HWReal = Union{Int8,Int16,Int32,Int64,UInt8,UInt16,UInt32,UInt64,Float32,Float64} | ||
const HWNumber = Union{HWReal, Complex{<:HWReal}, Rational{<:HWReal}} | ||
|
||
# inference.jl has complicated logic to inline x^2 and x^3 for | ||
# numeric types. In terms of Val we can do it much more simply: | ||
@inline internal_pow(x::HWNumber, ::Type{Val{0}}) = one(x) | ||
@inline internal_pow(x::HWNumber, ::Type{Val{1}}) = x | ||
@inline internal_pow(x::HWNumber, ::Type{Val{2}}) = x*x | ||
@inline internal_pow(x::HWNumber, ::Type{Val{3}}) = x*x*x | ||
# numeric types. In terms of Val we can do it much more simply. | ||
# (The third argument prevents unexpected behavior if a function ^ | ||
# is defined that is not equal to Base.^) | ||
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{0}}) = one(x) | ||
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{1}}) = x | ||
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{2}}) = x*x | ||
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{3}}) = x*x*x | ||
|
||
Comment on lines
+219
to
220
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a particular reason to stop at 3 rather than e.g. 4? Of course there's virtually no limit, but 4 seems like a common power (much more than 5 and above I'd say). This could help with things like JuliaStats/HypothesisTests.jl#238. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's an increasing loss of accuracy at higher powers. This definition for 3 is already inaccurate by a noticeable bit in some cases, so 4+ will potentially be progressively worse from there (though There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, thanks. So are you suggesting we should add a definition using |
||
# b^p mod m | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2903,9 +2903,12 @@ end | |
end | ||
|
||
import Base.^ | ||
immutable PR20530; end | ||
struct PR20530; end | ||
struct PR20889; x; end | ||
^(::PR20530, p::Int) = 1 | ||
^{p}(::PR20530, ::Type{Val{p}}) = 2 | ||
^(t::PR20889, b) = t.x + b | ||
^(t::PR20889, b::Integer) = t.x + b | ||
Base.literal_pow{p}(::typeof(^), ::PR20530, ::Type{Val{p}}) = 2 | ||
@testset "literal powers" begin | ||
x = PR20530() | ||
p = 2 | ||
|
@@ -2923,6 +2926,12 @@ immutable PR20530; end | |
end | ||
end | ||
end | ||
@test PR20889(2)^3 == 5 | ||
end | ||
module M20889 # do we get the expected behavior without importing Base.^? | ||
struct PR20889; x; end | ||
^(t::PR20889, b) = t.x + b | ||
Base.Test.@test PR20889(2)^3 == 5 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There's no need for the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have to put these tests in a different module than the other tests because I'm explicitly testing for what happens when I do not import There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh, right. Don't mind me then. |
||
end | ||
|
||
@testset "iszero" begin | ||
|
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.
Shouldn't this be
@inline literal_pow{p}(::typeof(^), x, ::Type{Val{p}}) = ^(x,p)
?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.
This generic fallback covers many situations, including the case where a user defines a function
^
without importing it from Base. In that case, if we restricted the type of the first argument, there would be no applicable method forliteral_pow
. Keep in mind that the^
on the right-hand side is really the^
passed as the first argument toliteral_pow
, not necessarilyBase.:^
.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.
maybe it should just be called
f
then ?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.
Right, then
pow
,f
or something like it, instead of^
, would be better here in my opinion.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 think I originally called it
y
, but Stefan suggested we change it to^
earlier in this thread. I am fine with either but can change it to whatever the powers that be decide (pun intended)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.
AFAICT
f
is commonly used for this kind of argument, so I'd vote for it.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.
Changed to
f
, since I think these comments indicate that it was confusing. Should be good to go after CI passes.