Skip to content

Commit

Permalink
optimize: Handle path-excluded Core.ifelse arguments (#50312)
Browse files Browse the repository at this point in the history
It's possible for PiNodes to effectively imply statically the condition
of a Core.ifelse. For example:
```julia
    23 ─ %60  = Core.ifelse(%47, false, true)::Bool
    │    %61  = Core.ifelse(%47, %58, false)::Union{Missing, Bool}
    25 ─        goto #27 if not %60
    26 ─ %65  = π (%61, Bool)
    └───        ...
```

In basic block #26, the PiNode gives us enough information to conclude
that `%47 === false` if control flow reaches that point. The previous
code incorrectly assumed that this kind of pruning would only be done
for PhiNodes.

Resolves #50276
  • Loading branch information
topolarity committed Jun 29, 2023
1 parent 6d400e4 commit 5db930e
Show file tree
Hide file tree
Showing 2 changed files with 67 additions and 1 deletion.
17 changes: 16 additions & 1 deletion base/compiler/ssair/passes.jl
Original file line number Diff line number Diff line change
Expand Up @@ -730,7 +730,7 @@ function perform_lifting!(compact::IncrementalCompact,
new_node = Expr(:call, ifelse_func, condition) # Renamed then_result, else_result added below
new_inst = NewInstruction(new_node, result_t, NoCallInfo(), old_inst[:line], old_inst[:flag])

ssa = insert_node!(compact, old_ssa, new_inst)
ssa = insert_node!(compact, old_ssa, new_inst, #= attach_after =# true)
lifted_philikes[i] = LiftedPhilike(ssa, IfElseCall(new_node), true)
end
# lifting_cache[ckey] = ssa
Expand Down Expand Up @@ -767,6 +767,21 @@ function perform_lifting!(compact::IncrementalCompact,
else_result = lifted_value(compact, old_node_ssa, else_result,
lifted_philikes, lifted_leaves, reverse_mapping)

# In cases where the Core.ifelse condition is statically-known, e.g., thanks
# to a PiNode from a guarding conditional, replace with the remaining branch.
if then_result === SKIP_TOKEN || else_result === SKIP_TOKEN
only_result = (then_result === SKIP_TOKEN) ? else_result : then_result

# Replace Core.ifelse(%cond, %a, %b) with %a
compact[lf.ssa][:inst] = only_result
should_count && _count_added_node!(compact, only_result)

# Note: Core.ifelse(%cond, %a, %b) has observable effects (!nothrow), but since
# we have not deleted the preceding statement that this was derived from, this
# replacement is safe, i.e. it will not affect the effects observed.
continue
end

@assert then_result !== SKIP_TOKEN && then_result !== UNDEF_TOKEN
@assert else_result !== SKIP_TOKEN && else_result !== UNDEF_TOKEN

Expand Down
51 changes: 51 additions & 0 deletions test/compiler/irpasses.jl
Original file line number Diff line number Diff line change
Expand Up @@ -742,6 +742,57 @@ let m = Meta.@lower 1 + 1
@test Core.Compiler.verify_ir(ir) === nothing
end

# A lifted Core.ifelse with an eliminated branch (#50276)
let m = Meta.@lower 1 + 1
@assert Meta.isexpr(m, :thunk)
src = m.args[1]::CodeInfo
src.code = Any[
# block 1
#= %1: =# Core.Argument(2),
# block 2
#= %2: =# Expr(:call, Core.ifelse, SSAValue(1), true, missing),
#= %3: =# GotoIfNot(SSAValue(2), 11),
# block 3
#= %4: =# PiNode(SSAValue(2), Bool), # <-- This PiNode is the trigger of the bug, since it
# means that only one branch of the Core.ifelse
# is lifted.
#= %5: =# GotoIfNot(false, 8),
# block 2
#= %6: =# nothing,
#= %7: =# GotoNode(8),
# block 4
#= %8: =# PhiNode(Int32[5, 7], Any[SSAValue(4), SSAValue(6)]),
# ^-- N.B. This PhiNode also needs to have a Union{ ... } type in order
# for lifting to be performed (it is skipped for e.g. `Bool`)
#
#= %9: =# Expr(:call, isa, SSAValue(8), Missing),
#= %10: =# ReturnNode(SSAValue(9)),
# block 5
#= %11: =# ReturnNode(false),
]
src.ssavaluetypes = Any[
Any,
Union{Missing, Bool},
Any,
Bool,
Any,
Missing,
Any,
Union{Nothing, Bool},
Bool,
Any,
Any
]
nstmts = length(src.code)
src.codelocs = fill(one(Int32), nstmts)
src.ssaflags = fill(one(Int32), nstmts)
src.slotflags = fill(zero(UInt8), 3)
ir = Core.Compiler.inflate_ir(src)
@test Core.Compiler.verify_ir(ir) === nothing
ir = @test_nowarn Core.Compiler.sroa_pass!(ir)
@test Core.Compiler.verify_ir(ir) === nothing
end

# Issue #31546 - missing widenconst in SROA
function f_31546(x)
(a, b) = x == "r" ? (false, false) :
Expand Down

6 comments on commit 5db930e

@maleadt
Copy link
Member

Choose a reason for hiding this comment

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

Let's do this one instead:

@nanosoldier runtests(isdaily = true)

@vtjnash

This comment was marked as outdated.

@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 package evaluation job has completed - possible new issues were detected.
A full report can be found here.

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 5db930e Jul 6, 2023

Choose a reason for hiding this comment

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

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

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on 5db930e Jul 7, 2023

Choose a reason for hiding this comment

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

"inference" fared very poorly again

Please sign in to comment.