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

RFC: lower x^literal as x^Val{literal} for integer literals #20530

Merged
merged 8 commits into from
Feb 16, 2017
Merged

RFC: lower x^literal as x^Val{literal} for integer literals #20530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
b453ba6
lower x^literal as x^Val{literal} for small integer literals (closes …
stevengj Feb 8, 2017
ce13a79
simplify smallnum check since all Ints get inlined as fixnums into Sc…
stevengj Feb 8, 2017
7eeefee
do x^Val{p} transformation for all literal p, not just small ones
stevengj Feb 8, 2017
5411736
fix comment
stevengj Feb 8, 2017
a7f1f45
docs for x^Val{p}
stevengj Feb 9, 2017
f23f250
documentation tweak
stevengj Feb 9, 2017
3a0d354
added test for x^literal
stevengj Feb 9, 2017
d1dab59
note experimental feature [ci skip]
stevengj Feb 16, 2017
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
4 changes: 4 additions & 0 deletions NEWS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,10 @@ Language changes
* The `typealias` keyword is deprecated, and should be replaced with
`Vector{T} = Array{T,1}` or a `const` assignment.

* Experimental feature: `x^n` for integer literals `n` (e.g. `x^3`
or `x^-3`) is now lowered to `x^Val{n}`, to enable compile-time
specialization for literal integer exponents ([#20530]).

Breaking changes
----------------

Expand Down
5 changes: 5 additions & 0 deletions base/intfuncs.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ 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},
# to enable compile-time optimizations specialized to p.
# However, we still need a fallback that calls the general ^:
^{p}(x, ::Type{Val{p}}) = x^p
Copy link
Member

Choose a reason for hiding this comment

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

How about we rewrite this as ^(x, p) = internal_pow(x, p) and then dispatch on Val in internal_pow? I ask because this is going to be a magnet for ambiguities since packages are likely to specialize on the first argument. E.g., PainterQubits/Unitful.jl#54

Copy link
Member Author

@stevengj stevengj Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Unitful will maybe want to write its own ^(x, Val{p}) methods that are type-stable anyway, however, so in this case the ambiguity warning is helpful.

Copy link
Member Author

@stevengj stevengj Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

But I guess it doesn't hurt to do this, and it may be helpful in other cases?

Copy link
Member Author

@stevengj stevengj Feb 17, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

I dunno, the more I think about it the more I think that the ambiguity warning is actually helpful here. Particularly if we start inlining x^Val{p}, anyone specializing purely on the first argument (and not restricting to p::Number, as most types would) will want to explicitly decide what to do for the Val{p} case.

Copy link
Contributor

Choose a reason for hiding this comment

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

these won't be warnings any more, they'll be errors the first time someone happens to call the ambiguous signature, unless you're going out of your way to check for ambiguities

Copy link
Member

Choose a reason for hiding this comment

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

Unitful wasn't even getting through precompilation, because some of these run at build time and the ambiguity error kicks in. These are fixable errors, as witnessed by that PR, but I don't personally see much downside to engineering the dispatch chain to avoid breakage.

Copy link
Member Author

Choose a reason for hiding this comment

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

The downside is that packages that really should be thinking about doing something different for Val{p} will get no warning of the change.

Copy link
Member Author

Choose a reason for hiding this comment

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

But I don't feel strongly about this; I've updated #20648 to use internal_pow as you suggest.


# b^p mod m

"""
Expand Down
5 changes: 5 additions & 0 deletions base/promotion.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -253,6 +253,11 @@ end

Exponentiation operator. If `x` is a matrix, computes matrix exponentiation.

If `y` is an `Int` literal (e.g. `2` in `x^2` or `-3` in `x^-3`), the Julia code
`x^y` is transformed by the compiler to `x^Val{y}`, to enable compile-time
specialization on the value of the exponent. (As a default fallback,
however, `x^Val{y}` simply calls the `^(x,y)` function.)

```jldoctest
julia> 3^5
243
Expand Down
4 changes: 4 additions & 0 deletions src/julia-syntax.scm
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2057,6 +2057,10 @@
(expand-forms
`(call (core _apply) ,f ,@(tuple-wrap argl '())))))

((and (eq? f '^) (length= e 4) (integer? (cadddr e)))
(expand-forms
`(call ^ ,(caddr e) (call (core apply_type) (top Val) ,(cadddr e)))))

((and (eq? f '*) (length= e 4))
(expand-transposed-op
e
Expand Down
12 changes: 12 additions & 0 deletions test/numbers.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2902,6 +2902,18 @@ end
@test rem2pi(T(-4), RoundUp) == -4
end

import Base.^
immutable PR20530; end
^(::PR20530, p::Int) = 1
^{p}(::PR20530, ::Type{Val{p}}) = 2
@testset "literal powers" begin
x = PR20530()
p = 2
@test x^p == 1
@test x^2 == 2
@test [x,x,x].^2 == [2,2,2]
end

@testset "iszero" begin
# Numeric scalars
for T in [Float16, Float32, Float64, BigFloat,
Expand Down