optimizer: Handle path-excluded Core.ifelse arguments
#50312
Conversation
I think that should be always legal. |
My main concern was about accidentally erasing an exception throw. What ensures that we preserve that effect? Is it that the original |
|
Ah, I guess make it conditional on |
|
|
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 JuliaLang#27 if not %60
26 ─ %65 = π (%61, Bool)
└─── ...
```
In basic block JuliaLang#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 JuliaLang#50276.
topolarity
added
the
status:merge me
| # 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), |
oscardssmith
added
performance
topolarity
added
kind:bugfix
| only_result = (then_result === SKIP_TOKEN) ? else_result : then_result | ||
|
|
||
| # Replace Core.ifelse(%cond, %a, %b) with %a | ||
| compact[lf.ssa][:inst] = only_result |
There was a problem hiding this comment.
On second look, this bypasses the count removal of the cond. This probably needs to be:
compact[lf.ssa] = nothing
compact[lf.ssa][:inst] = only_result
Possibly also consider turning on the oracle check for the tests, which would have caught this.
There are cases where a relevant PiNode implies that
Core.ifelse(%cond, %a, %b)never yields%b.I opted to take the basic path here and just lower toCore.ifelse(%cond, %a, %a). An improved solution would probably replace with%adirectly, but I haven't convinced myself of the legality conditions for that.Resolves #50276.