Skip to content

Commit

Permalink
fix #29306, teach effect_free that GotoIfNot with non-Bool can throw (#…
Browse files Browse the repository at this point in the history
  • Loading branch information
JeffBezanson authored and Keno committed Sep 22, 2018
1 parent f8b52da commit a854139
Show file tree
Hide file tree
Showing 2 changed files with 14 additions and 3 deletions.
10 changes: 7 additions & 3 deletions base/compiler/optimize.jl
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,14 @@ end
# These affect control flow within the function (so may not be removed
# if there is no usage within the function), but don't affect the purity
# of the function as a whole.
function stmt_affects_purity(@nospecialize stmt)
if isa(stmt, GotoIfNot) || isa(stmt, GotoNode) || isa(stmt, ReturnNode)
function stmt_affects_purity(@nospecialize(stmt), ir)
if isa(stmt, GotoNode) || isa(stmt, ReturnNode)
return false
end
if isa(stmt, GotoIfNot)
t = argextype(stmt.cond, ir, ir.spvals)
return !(t Bool)
end
if isa(stmt, Expr)
return stmt.head != :simdloop && stmt.head != :enter
end
Expand All @@ -173,7 +177,7 @@ function optimize(opt::OptimizationState, @nospecialize(result))
proven_pure = true
for i in 1:length(ir.stmts)
stmt = ir.stmts[i]
if stmt_affects_purity(stmt) && !stmt_effect_free(stmt, ir.types[i], ir, ir.spvals)
if stmt_affects_purity(stmt, ir) && !stmt_effect_free(stmt, ir.types[i], ir, ir.spvals)
proven_pure = false
break
end
Expand Down
7 changes: 7 additions & 0 deletions test/core.jl
Original file line number Diff line number Diff line change
Expand Up @@ -6723,3 +6723,10 @@ function f29175(tuple::T) where {T<:Tuple}
return prefix
end
@test f29175((1,2,3)) === (1,2)

# issue #29306
let a = [1,2,3,4,missing,6,7]
@test_throws TypeError [ (x>6 ? missing : x) for x in a]
foo(x) = x > 0 ? x : missing
@test_throws TypeError foo(missing)
end

4 comments on commit a854139

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@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 @ararslan

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

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

Executing the daily benchmark build, I will reply here when finished:

@nanosoldier runbenchmarks(ALL, isdaily = true)

@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 @ararslan

Please sign in to comment.