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

RFC: address issue #20882 #20889

Merged
merged 8 commits into from
Mar 10, 2017
Merged

Conversation

ajkeller34
Copy link
Contributor

@ajkeller34 ajkeller34 commented Mar 4, 2017

This is exploratory (i.e. Friday night entertainment) and meant to demonstrate a proof-of-concept for how I might fix #20882, keeping the new Val type optimizations, while also making it much less confusing for average users. It is quite possible I overlooked something but I think this could be a nice solution.

After this PR:

julia> type T
    x
    end

julia> ^(t::T, b) = t.x + b
^ (generic function with 2 methods)

julia> T(2)^3
5

and if you import Base.^:

julia> import Base: ^

julia> type T
              x
              end

julia> ^(t::T, b) = t.x + b
^ (generic function with 52 methods)

julia> T(2)^3
ERROR: MethodError: ^(::T, ::Int64) is ambiguous. Candidates:
  ^(x, p::Integer) in Base at intfuncs.jl:196
  ^(t::T, b) in Main at REPL[3]:1
Stacktrace:
 [1] internal_pow(::T, ::Type{Val{3}}, ::Base.#^) at ./intfuncs.jl:206

julia> ^(t::T, b::Integer) = t.x + b
^ (generic function with 53 methods)

julia> T(2)^3
5

which is pretty much a recovery of the behavior you'd expect. Sure, internal_pow appears in the stack trace, so there may still be lingering concerns about referential transparency, but now the average user only has to reason about ^ as was the case in Julia 0.5. So, I think the concern becomes much more academic.

Some comments:

  • Base.internal_pow takes three arguments, the third being the function that was called to get there, which could either be Base.^ or a different function named ^ defined in a module that did not import Base.^.

  • The optimizations for raising HWNumber to integer literal powers 0, 1, 2, 3 are only applied if Base.^ is the operator, not if the user defines ^ without importing it. In this way, even if ^ is defined in a module, we will get predictable behavior for HWNumber types using a literal integer power:

julia> ^(x::Int, y::Int) = 7
^ (generic function with 52 methods)
julia> 2^0
7

The only downside of this PR that I can think of is that if the user is a type pirate and tries to override methods in Base, then they will discover that they cannot dodge the Val optimizations so easily:

julia> import Base: ^

julia> ^(x::Int, y::Int) = 7
^ (generic function with 52 methods)

julia> 2^0
1
  • We still maintain our Val type optimizations for HWNumbers, which I infer from the fact that it does repeated multiplications for powers 1, 2, 3:
julia> import Base: *

julia> *(::Float64, ::Float64) = 2.0
WARNING: Method definition *(Float64, Float64) in module Base at float.jl:375 overwritten in module Main at REPL[2]:1.
* (generic function with 177 methods)

julia> 2.0^3
2.0

however, I don't really understand why @code_lowered then reports:

julia> @code_lowered 2.0^3
CodeInfo(:(begin 
        nothing
        nothing
        return x ^ (Base.Math.Float64)(y)
    end))

which is clearly not what is happening.

  • To take advantage of the lowering operation for optimizations, you need to specialize Base.internal_pow, not Base.^. I don't consider this a problem personally, because only experts will need this specialization. I have not yet updated the tests to reflect this, so these lines need changing for the PR 20530 tests to pass. (edit: but otherwise, the tests should pass)

  • No clue if there is a performance penalty, but I don't think I've introduced any type instabilities.

@@ -2057,7 +2057,7 @@

((and (eq? f '^) (length= e 4) (integer? (cadddr e)))
(expand-forms
`(call ^ ,(caddr e) (call (core apply_type) (top Val) ,(cadddr e)))))
`(call (|.| Base (quote internal_pow)) ,(caddr e) (call (core apply_type) (top Val) ,(cadddr e)) ^)))
Copy link
Member

Choose a reason for hiding this comment

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

We should probably call it literal_pow in this case

Copy link
Member

Choose a reason for hiding this comment

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

Yes, that does seem like a better name now that it's used this way.

test/numbers.jl Outdated
@@ -2904,8 +2904,11 @@ end

import Base.^
immutable PR20530; end
immutable PR20889; x; end
Copy link
Contributor

Choose a reason for hiding this comment

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

these should be struct

@@ -2057,7 +2057,7 @@

((and (eq? f '^) (length= e 4) (integer? (cadddr e)))
(expand-forms
`(call ^ ,(caddr e) (call (core apply_type) (top Val) ,(cadddr e)))))
`(call (|.| Base (quote literal_pow)) ,(caddr e) (call (core apply_type) (top Val) ,(cadddr e)) ^)))
Copy link
Member

Choose a reason for hiding this comment

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

Use 'top' instead of 'Base.'

It'll essentially resolve to the same thing, but lets us avoid hard coding the name Bade in here, and let's someone override the default function (by calling a special function to declare their module also 'top')

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Great, thanks. I figured there might be some issue with the way I did that... I'll try it out.

@ajkeller34
Copy link
Contributor Author

ajkeller34 commented Mar 5, 2017 via email

@StefanKarpinski
Copy link
Member

I feel like literal_pow should probably take ^ as its first argument – by convention we have function arguments first and in this case it seems more intuitive to have ^ as the first argument. You could call it ^ locally as well, which might make some of the code clearer – or more confusing since that's a local that shadows the global ^, but since it's passed in anyway, you need to be aware that there can be multiple meanings of ^ from different contexts anyway.

@StefanKarpinski
Copy link
Member

Yes, that looks much better. Thanks!

@ajkeller34
Copy link
Contributor Author

I think the test failure may be unrelated? Also, there are a few spots in documentation where I say that we lower to Base.literal_pow(...) when we're now using top in julia-syntax.scm, but I'm not sure what the proper rewording is since I don't fully understand the top mechanism.

@stevengj
Copy link
Member

stevengj commented Mar 5, 2017

I think there are some ^(x, ::Val{p}) methods that can now be deleted (in irrationals.jl, mpfr.jl, and gmp.jl). (Actually, they could have been deleted after the HWNumber change.)

@StefanKarpinski
Copy link
Member

@stevengj: shall we just do that in a separate PR since it's unrelated to this?

@stevengj
Copy link
Member

stevengj commented Mar 6, 2017

sure

@ajkeller34 ajkeller34 changed the title WIP: address issue #20882 RFC: address issue #20882 Mar 6, 2017
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
Copy link
Contributor

Choose a reason for hiding this comment

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

There's no need for the Base.Test. here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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 Base.^. Since they are in a different module, I shouldn't get Base.Test.@test automatically, since I haven't done using Base.Test.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, right. Don't mind me then.

@ararslan ararslan added compiler:lowering Syntax lowering (compiler front end, 2nd stage) maths Mathematical functions labels Mar 6, 2017
@ajkeller34
Copy link
Contributor Author

This is good to merge as far as I'm concerned, though perhaps someone might want to retrigger CI or run benchmarks. (However, does anyone understand why @code_lowered reports what it does as described in the first comment in this thread?)

As we saw in #20882, some view the current behavior as odd enough to be considered a bug, so I hope this PR makes it into 0.6.0.

Copy link
Member

@StefanKarpinski StefanKarpinski left a comment

Choose a reason for hiding this comment

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

LGTM, this should have @stevengj's sign-off and be merged if it looks good.

@JeffBezanson
Copy link
Member

Nice. I think this is a really clever solution.

base/intfuncs.jl Outdated
# 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)
Copy link
Contributor

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)?

Copy link
Contributor Author

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 for literal_pow. Keep in mind that the ^ on the right-hand side is really the ^ passed as the first argument to literal_pow, not necessarily Base.:^.

Copy link
Contributor

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 ?

Copy link
Contributor

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.

Copy link
Contributor Author

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)

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@StefanKarpinski
Copy link
Member

Failure is the usual inability of AppVeyor to load packages from GitHub.

@StefanKarpinski StefanKarpinski merged commit 75c2e72 into JuliaLang:master Mar 10, 2017
@ajkeller34 ajkeller34 deleted the val_lowering branch March 10, 2017 17:27
Comment on lines +219 to 220
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{3}}) = x*x*x

Copy link
Member

Choose a reason for hiding this comment

The 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.

Copy link
Member

Choose a reason for hiding this comment

The 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 (x*x)^2 seems pretty well-compensated due to the symmetric shape, as these things go).

Copy link
Member

Choose a reason for hiding this comment

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

OK, thanks. So are you suggesting we should add a definition using (x*x)^2?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:lowering Syntax lowering (compiler front end, 2nd stage) maths Mathematical functions
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Issues with the new x^y Val lowering
9 participants