- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 5.6k
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
fix type instability in exp exp1m for Complex{Int} #18786
Conversation
@@ -424,7 +424,8 @@ function log2(z::Complex) | |||
a/log(oftype(real(a),2)) | |||
end | |||
|
|||
function exp(z::Complex) | |||
exp(z::Complex) = exp(float(z)) |
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.
Why can't type inference figure this out given isnan(::Int)
and isfinite(::Int)
are both constants (edit: pure)? I was thinking about doing this a while ago (before we had constant propagation in type inference) but this adds branches (isfinite
one in particular) that seems hard for LLVM to eliminate.
@@ -424,7 +424,8 @@ function log2(z::Complex) | |||
a/log(oftype(real(a),2)) | |||
end | |||
|
|||
function exp(z::Complex) | |||
exp(z::Complex) = exp(float(z)) |
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.
Ref #13539 for a better workaround to the same issue.
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.
And also the (probably) new preferred way of adding similar test one line above the one you added.
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 missunderstand you but the test above the one I added looks pretty similar to how I did 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.
I think you need to rebase
Line 945 in fe29cb5
@testset "issue #11839" 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.
Oh yeah, of course. Thanks.
Subsumed by #18793 |
Fixes #18785