-
-
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
Conversation
src/julia-syntax.scm
Outdated
@@ -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)) ^))) |
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.
We should probably call it literal_pow
in this case
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.
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 |
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.
these should be struct
src/julia-syntax.scm
Outdated
@@ -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)) ^))) |
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.
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')
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.
Great, thanks. I figured there might be some issue with the way I did that... I'll try it out.
That’s right. By passing ^, we now opt out of the specializations for powers 0, 1, 2, 3 if we find that ^ != Base.^. See my comment beginning “The optimizations for raising `HWNumber`…” in the Github conversation for this PR. It is now considerably harder to come up with surprising behavior.
I can’t find your comments on Github, even though I see everyone else’s, so I hope this reply by e-mail gets to you.
… On Mar 4, 2017, at 6:39 PM, Steven G. Johnson ***@***.***> wrote:
@stevengj commented on this pull request.
In NEWS.md <#20889 (comment)>:
> @@ -74,8 +74,8 @@ Language changes
`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]).
+ or `x^-3`) is now lowered to `Base.literal_pow(x, Val{n}, ^)`, to enable
I guess for cases where ^ != Base.^?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub <#20889 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AG0I-XX1EwnOBTLPj1zB52kYyPhL8p6jks5riiBTgaJpZM4MTA33>.
|
I feel like |
Yes, that looks much better. Thanks! |
I think the test failure may be unrelated? Also, there are a few spots in documentation where I say that we lower to |
I think there are some |
@stevengj: shall we just do that in a separate PR since it's unrelated to this? |
sure |
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 comment
The reason will be displayed to describe this comment to others. Learn more.
There's no need for the Base.Test.
here.
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 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
.
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.
Oh, right. Don't mind me then.
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 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. |
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.
LGTM, this should have @stevengj's sign-off and be merged if it looks good.
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) |
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 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.:^
.
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.
Failure is the usual inability of AppVeyor to load packages from GitHub. |
@inline literal_pow(::typeof(^), x::HWNumber, ::Type{Val{3}}) = x*x*x | ||
|
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.
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 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).
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.
OK, thanks. So are you suggesting we should add a definition using (x*x)^2
?
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:
and if you import
Base.^
: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 beBase.^
or a different function named^
defined in a module that did not importBase.^
.The optimizations for raising
HWNumber
to integer literal powers 0, 1, 2, 3 are only applied ifBase.^
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 forHWNumber
types using a literal integer power: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:Val
type optimizations forHWNumber
s, which I infer from the fact that it does repeated multiplications for powers 1, 2, 3:however, I don't really understand why
@code_lowered
then reports:which is clearly not what is happening.
To take advantage of the lowering operation for optimizations, you need to specialize
Base.internal_pow
, notBase.^
. 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.