Skip to content

Commit

Permalink
Revert "Fix miscellaneous type instabilities"
Browse files Browse the repository at this point in the history
  • Loading branch information
KristofferC authored Jul 25, 2016
1 parent 8f99882 commit 6bcb99c
Show file tree
Hide file tree
Showing 6 changed files with 32 additions and 16 deletions.
14 changes: 8 additions & 6 deletions base/bool.jl
Original file line number Diff line number Diff line change
Expand Up @@ -51,16 +51,18 @@ abs2(x::Bool) = x
^(x::Bool, y::Bool) = x | !y
^(x::Integer, y::Bool) = ifelse(y, x, one(x))

function +{T<:AbstractFloat}(x::Bool, y::T)::promote_type(Bool,T)
return ifelse(x, one(y) + y, y)
function +{T<:AbstractFloat}(x::Bool, y::T)
ifelse(x, one(promote_type(Bool,T)) + convert(promote_type(Bool,T),y),
convert(promote_type(Bool,T),y))
end
+(y::AbstractFloat, x::Bool) = x + y

function *{T<:Number}(x::Bool, y::T)::promote_type(Bool,T)
return ifelse(x, y, copysign(zero(y), y))
function *{T<:Number}(x::Bool, y::T)
ifelse(x, convert(promote_type(Bool,T),y),
ifelse(signbit(y), -zero(promote_type(Bool,T)), zero(promote_type(Bool,T))))
end
function *{T<:Unsigned}(x::Bool, y::T)::promote_type(Bool,T)
return ifelse(x, y, zero(y))
function *{T<:Unsigned}(x::Bool, y::T)
ifelse(x, convert(promote_type(Bool,T),y), zero(promote_type(Bool,T)))
end
*(y::Number, x::Bool) = x * y

Expand Down
6 changes: 3 additions & 3 deletions base/complex.jl
Original file line number Diff line number Diff line change
Expand Up @@ -456,11 +456,11 @@ function log1p{T}(z::Complex{T})
end
end

function ^{T<:AbstractFloat}(z::Complex{T}, p::Complex{T})::Complex{T}
if p == 2 #square
function ^{T<:AbstractFloat}(z::Complex{T}, p::Complex{T})
if p==2 #square
zr, zi = reim(z)
x = (zr-zi)*(zr+zi)
y = 2*zr*zi
y = T(2*zr*zi)
if isnan(x)
if isinf(y)
x = copysign(zero(T),zr)
Expand Down
1 change: 0 additions & 1 deletion base/irrationals.jl
Original file line number Diff line number Diff line change
Expand Up @@ -94,7 +94,6 @@ hash(x::Irrational, h::UInt) = 3*object_id(x) - h
for op in Symbol[:+, :-, :*, :/, :^]
@eval $op(x::Irrational, y::Irrational) = $op(Float64(x),Float64(y))
end
*(x::Bool, y::Irrational) = ifelse(x, Float64(y), 0.0)

macro irrational(sym, val, def)
esym = esc(sym)
Expand Down
4 changes: 2 additions & 2 deletions base/rational.jl
Original file line number Diff line number Diff line change
Expand Up @@ -355,8 +355,8 @@ function ^(x::Rational, n::Integer)
end

^(x::Number, y::Rational) = x^(y.num/y.den)
^{T<:AbstractFloat}(x::T, y::Rational) = x^(convert(T, y.num / y.den))
^{T<:AbstractFloat}(x::Complex{T}, y::Rational) = x^(convert(T, y.num / y.den))
^{T<:AbstractFloat}(x::T, y::Rational) = x^(convert(T,y.num)/y.den)
^{T<:AbstractFloat}(x::Complex{T}, y::Rational) = x^(convert(T,y.num)/y.den)

^{T<:Rational}(z::Complex{T}, n::Bool) = n ? z : one(z) # to resolve ambiguity
function ^{T<:Rational}(z::Complex{T}, n::Integer)
Expand Down
2 changes: 0 additions & 2 deletions base/strings/io.jl
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ function print(io::IO, x)
finally
unlock(io)
end
return nothing
end

function print(io::IO, xs...)
Expand All @@ -21,7 +20,6 @@ function print(io::IO, xs...)
finally
unlock(io)
end
return nothing
end

println(io::IO, xs...) = print(io, xs..., '\n')
Expand Down
21 changes: 19 additions & 2 deletions test/numbers.jl
Original file line number Diff line number Diff line change
Expand Up @@ -2790,7 +2790,7 @@ let types = (Base.BitInteger_types..., BigInt, Bool,
Complex{Int}, Complex{UInt}, Complex32, Complex64, Complex128)
for S in types
for op in (+, -)
if S === Float16
if S === Float16 # type instability here?
# broken, fixme then remove this branch
@test_throws ErrorException @inferred(Base.promote_op(op, S))
T = Base.promote_op(op, S)
Expand All @@ -2810,10 +2810,27 @@ let types = (Base.BitInteger_types..., BigInt, Bool,
# broken, fixme then remove this branch
@test_throws ErrorException @inferred(Base.promote_op(op, R, S))
T = Base.promote_op(op, R, S)
if ((R === Bool || S === Bool) && op in (+, *)) ||
((S in (Rational{Int}, Complex{Float16})) && op === ^) ||
(R === Complex{Float16} && op === ^)
@test_throws ErrorException @inferred(op(one(R), one(S)))
t = op(one(R), one(S))
else
t = @inferred op(one(R), one(S))
end
elseif (R === Complex{Float16} || S === Complex{Float16}) &&
((R === Bool && op in (+, *, /, ^)) ||
(S === Bool && op in (+, *)) ||
(S in (R, Rational{Int}) && op === ^))
# broken, fixme then remove this branch too
@test_throws ErrorException @inferred(Base.promote_op(op, R, S))
T = Base.promote_op(op, R, S)
@test_throws ErrorException @inferred(op(one(R), one(S)))
t = op(one(R), one(S))
else
T = @inferred Base.promote_op(op, R, S)
t = @inferred op(one(R), one(S))
end
t = @inferred op(one(R), one(S))
@test T === typeof(t)
end
end
Expand Down

8 comments on commit 6bcb99c

@KristofferC
Copy link
Member Author

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks(ALL, vs = ":master")

@jrevels
Copy link
Member

@jrevels jrevels commented on 6bcb99c Jul 25, 2016

Choose a reason for hiding this comment

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

Hmm Nanosoldier didn't seem to pick this one up. You didn't edit in quotes to your comment or something like that did you? Nanosoldier doesn't pick up on comment edits anymore. Let's try again:

@nanosoldier runbenchmarks(ALL, vs = ":master")

EDIT: Turns out one of the server nodes fell over and I had to reboot. I resubmitted the job and it's running now.

@KristofferC
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I wrote it wrong first but instead of editing I deleted and made a new comment. Thanks for fixing it.

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @jrevels

@jrevels
Copy link
Member

Choose a reason for hiding this comment

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

ID time ratio memory ratio
["simd",("conditional_loop!","Int32",4095)] 0.00 (20%) ✅ 1.00 (1%)
["simd",("conditional_loop!","Int32",4096)] 0.00 (20%) ✅ 1.00 (1%)

woah, is there really a >100x improvement here or did something go wonky?

@Sacha0
Copy link
Member

@Sacha0 Sacha0 commented on 6bcb99c Jul 25, 2016

Choose a reason for hiding this comment

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

Those improvements should be real: The regressions were ~250x if memory serves.

@KristofferC
Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, there are some inlining bugs with function return type syntax so I guess these isn't getting inlined anymore.

@tkelman
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm going to delete this branch for the sake of cleanliness, but leave a cross-reference to #17530 so it's possible to find the conversation from there later

Please sign in to comment.