Skip to content
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

optimizer: ssaflags validation after optimization ? #42258

Open
aviatesk opened this issue Sep 15, 2021 · 5 comments · Fixed by #42260
Open

optimizer: ssaflags validation after optimization ? #42258

aviatesk opened this issue Sep 15, 2021 · 5 comments · Fixed by #42260
Labels
compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)

Comments

@aviatesk
Copy link
Member

It seems that (ci::CodeInfo).ssaflags is sometimes processed badly during optimization
(and it will confuse the inlining cost model).

Given the following definition of Core.Compiler.setindex!(::Core.Compiler.UseRef, ::Any):

function setindex!(x::UseRef, @nospecialize(v))
stmt = x.stmt
if isa(stmt, Expr) && stmt.head === :(=)
rhs = stmt.args[2]
if isa(rhs, Expr)
if is_relevant_expr(rhs)
x.op > length(rhs.args) && throw(BoundsError())
rhs.args[x.op] = v
return v
end
end
x.op == 1 || throw(BoundsError())
stmt.args[2] = v
elseif isa(stmt, Expr) # @assert is_relevant_expr(stmt)
x.op > length(stmt.args) && throw(BoundsError())
stmt.args[x.op] = v
elseif isa(stmt, GotoIfNot)
x.op == 1 || throw(BoundsError())
x.stmt = GotoIfNot(v, stmt.dest)
elseif isa(stmt, ReturnNode)
x.op == 1 || throw(BoundsError())
x.stmt = typeof(stmt)(v)
elseif isa(stmt, UpsilonNode)
x.op == 1 || throw(BoundsError())
x.stmt = typeof(stmt)(v)
elseif isa(stmt, PiNode)
x.op == 1 || throw(BoundsError())
x.stmt = typeof(stmt)(v, stmt.typ)
elseif isa(stmt, PhiNode)
x.op > length(stmt.values) && throw(BoundsError())
isassigned(stmt.values, x.op) || throw(BoundsError())
stmt.values[x.op] = v
elseif isa(stmt, PhiCNode)
x.op > length(stmt.values) && throw(BoundsError())
isassigned(stmt.values, x.op) || throw(BoundsError())
stmt.values[x.op] = v
else
throw(BoundsError())
end
return x
end

I found ssaflags is set correctly before optimization (every "throw block" gets marked with IR_FLAG_THROW_BLOCK = 0x01 << 3:

julia> function show_with_flags(io::IO, @nospecialize x)
           src, rt = x
           buf = IOBuffer()
           show(IOContext(buf, :color => true), src => rt)
           s = String(take!(buf))
           for (i, line) in enumerate(split(s, '\n'))
               i -= 1
               if checkbounds(Bool, src.code, i)
                   println(io, repr(src.ssaflags[i]), " ", line)
               else
                   println(line)
               end
           end
       end;

julia> show_with_flags(@nospecialize x) = show_with_flags(stdout::IO, x);

julia> code_typed(Core.Compiler.setindex!, (Core.Compiler.UseRef, Core.Compiler.NewSSAValue);
                  optimize=false) |> 
                  only |>
                  show_with_flags
CodeInfo(
0x00 1 ──        nothing::Core.Const(nothing)
0x00 │           Core.NewvarNode(:(rhs))::Any
0x00 │           (stmt = Core.Compiler.getproperty(x, :stmt))::Any
0x00%4   = (stmt isa Core.Compiler.Expr)::Bool
0x00 └───        goto #12 if not %4
...
0x00 │           Core.Compiler.setindex!(%137, v, %138)::Any
0x00 └───        goto #51
0x08 50%141 = Core.Compiler.BoundsError()::Any
0x08 └───        Core.Compiler.throw(%141)::Union{}
0x00 51return x
) => Union{Core.Compiler.NewSSAValue, Core.Compiler.UseRef}

julia> # extracts 0x08 statments
       let (ci, rt) = code_typed(Core.Compiler.setindex!, (Core.Compiler.UseRef, Core.Compiler.NewSSAValue);
                  optimize=false) |> 
                  only
           filter(collect(zip(ci.ssaflags, ci.code))) do (flag, _)
               (flag & Core.Compiler.IR_FLAG_THROW_BLOCK)  0
           end
       end
24-element Vector{Tuple{UInt8, Any}}:
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%20)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%31)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%43)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%56)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%68)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%80)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%92)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%106)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%114)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%127)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%135)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%141)))

But after optimization some 0x08 statements no longer correspond to "throw block", and rather they're scattered around arbitrary places:

julia> code_typed(Core.Compiler.setindex!, (Core.Compiler.UseRef, Core.Compiler.NewSSAValue);
                  optimize=true) |> 
                  only |>
                  show_with_flags
CodeInfo(
0x10 1 ── %1   = Core.Compiler.getproperty(x, :stmt)::Any
0x10%2   = (%1 isa Core.Compiler.Expr)::Bool
0x00 └───        goto #25 if not %2
0x10 2 ── %4   = π (%1, Expr)
0x10%5   = Core.Compiler.getproperty(%4, :head)::Symbol
...
0x00 └───        unreachable
0x00 67%221 = Core.Compiler.BoundsError()::Any
0x00 │           Core.Compiler.throw(%221)::Union{}
0x00 └───        unreachable
0x00 68return x
) => Union{Core.Compiler.NewSSAValue, Core.Compiler.UseRef}

julia> # extracts 0x08 statments
       let (ci, rt) = code_typed(Core.Compiler.setindex!, (Core.Compiler.UseRef, Core.Compiler.NewSSAValue);
                  optimize=true) |> 
                  only
           filter(collect(zip(ci.ssaflags, ci.code))) do (flag, _)
               (flag & Core.Compiler.IR_FLAG_THROW_BLOCK)  0
           end
       end
22-element Vector{Tuple{UInt8, Any}}:
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%50)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%66)))
 (0x08, :(goto %80 if not %75))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x18, nothing)
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(goto %108 if not %102))
 (0x18, nothing)
 (0x08, :(goto %120 if not %114))
 (0x18, :(Core.Compiler.getproperty(_2, :op)))
 (0x18, :(Core.Compiler.getproperty(_2, :op)))
 (0x18, :(Core.Compiler.getproperty(%140, :values)))
 (0x18, :(Core.Compiler.getproperty(%148, :values)))
 (0x18, :(Core.Compiler.getproperty(_2, :op)))
 (0x08, :(goto %221 if not %179))
 (0x18, :(Core.Compiler.getproperty(_2, :op)))
 (0x08, :(unreachable))
 (0x18, :(Core.Compiler.getproperty(%190, :values)))
 (0x08, :(Core.Compiler.BoundsError()))
 (0x08, :(Core.Compiler.throw(%218)))

There are several points where new statement can be inserted in the optimization, and AFAIU we basically need to check if each construction of NewInstruction is valid and correctly inherits the statement flags:

struct NewInstruction
stmt::Any
type::Any
info::Any
# If nothing, copy the line from previous statement
# in the insertion location
line::Union{Int32, Nothing}
flag::UInt8
## Insertion options
# The IR_FLAG_EFFECT_FREE flag has already been computed (or forced).
# Don't bother redoing so on insertion.
effect_free_computed::Bool
NewInstruction(@nospecialize(stmt), @nospecialize(type), @nospecialize(info),
line::Union{Int32, Nothing}, flag::UInt8, effect_free_computed::Bool) =
new(stmt, type, info, line, flag, effect_free_computed)
end
NewInstruction(@nospecialize(stmt), @nospecialize(type)) =
NewInstruction(stmt, type, nothing)
NewInstruction(@nospecialize(stmt), @nospecialize(type), line::Union{Nothing, Int32}) =
NewInstruction(stmt, type, nothing, line, 0x00, false)

This issue might also confuse propagate_inbounds and callsite inlining stuff, because they also use ssaflags set before optimization.

As a final target, the following case should pass once this issue gets resolved:

let 
    ci, rt = code_typed(Core.Compiler.setindex!, (Core.Compiler.UseRef, Core.Compiler.NewSSAValue);
               optimize=true) |> 
               only
    idx = filter(1:length(ci.code)) do i
        stmt = ci.code[i]
        if Meta.isexpr(stmt, :call)
            t = Core.Compiler.argextype(stmt.args[1], ci, Any[])
            ft = Core.Compiler.widenconst(t)
            return ft === typeof(BoundsError) || ft === typeof(throw)
        end
        false
    end
    @test all(ci.ssaflags[idx]) do flag
        flag & Core.Compiler.IR_FLAG_THROW_BLOCK  0
    end
end
@aviatesk aviatesk added compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) good first issue Indicates a good issue for first-time contributors to Julia labels Sep 15, 2021
aviatesk added a commit that referenced this issue Sep 15, 2021
…ge statements

It seems that this change still doesn't resolve #42258 though.
aviatesk added a commit that referenced this issue Sep 15, 2021
…ge statements

It seems that this change still doesn't resolve #42258 though.
aviatesk added a commit that referenced this issue Sep 15, 2021
…ge statements

It seems that this change still doesn't resolve #42258 though.
aviatesk added a commit that referenced this issue Sep 15, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
@vtjnash
Copy link
Member

vtjnash commented Sep 15, 2021

It sounds like our flags are incorrectly defined. Dropping statement flags is supposed to always be legal, so noinline should be inverted (permitinline), for example.

@aviatesk
Copy link
Member Author

Hmm, I can't follow you.

It turns out that this issue is caused by that type_annotate! missed to delete corresponding statement flag when dropping a statement (, which should be fixed by #42262 ).
I'm not sure where and why we need to invert a flag, as far as we make sure to keep the correspondence between statements and flags.

@vtjnash
Copy link
Member

vtjnash commented Sep 15, 2021

Sometimes we'll insert or move statements, and (like LLVM) need to drop flags since we won't know if they are still applicable

aviatesk added a commit that referenced this issue Sep 18, 2021
…ge statements

It seems that this change still doesn't resolve #42258 though.
aviatesk added a commit that referenced this issue Sep 18, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
@aviatesk
Copy link
Member Author

Okay, so we need to validate statement flags on two levels ?

  1. does a flag corresponds to the appropriate statement
  2. is a flag valid after some transformation

I think #42260 and #42262 are necessary for 1., and we can consider about 2. later.
Especially, we need to care about IR_FLAG_THROW_BLOCK is valid after optimizations ? We may want to go back to the previous implementation where we recomputed throw blocks after optimization.

aviatesk added a commit that referenced this issue Sep 20, 2021
…ge statements (#42260)

It seems that this change still doesn't resolve #42258 though.
@aviatesk aviatesk reopened this Sep 20, 2021
aviatesk added a commit that referenced this issue Sep 20, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
aviatesk added a commit that referenced this issue Sep 21, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
aviatesk added a commit that referenced this issue Sep 22, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
@aviatesk aviatesk removed the good first issue Indicates a good issue for first-time contributors to Julia label Sep 22, 2021
@aviatesk aviatesk reopened this Sep 22, 2021
@aviatesk aviatesk changed the title optimizer: ssaflags can be strange after optimization optimizer: ssaflags validation after optimization ? Sep 22, 2021
@aviatesk
Copy link
Member Author

The first flag skew should be fixed and will be kept being asserted by #42262 .
I'd keep this issue open unless we settle on the second skew.

Probably the next step would be to find a concrete case where ssaflags actually becomes strange after optimization.

KristofferC pushed a commit that referenced this issue Sep 28, 2021
…ge statements (#42260)

It seems that this change still doesn't resolve #42258 though.
KristofferC pushed a commit that referenced this issue Sep 28, 2021
Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of #42260.
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
…ng coverage statements (JuliaLang#42260)

It seems that this change still doesn't resolve JuliaLang#42258 though.
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Feb 22, 2022
…JuliaLang#42262)

Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of JuliaLang#42260.
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
…ng coverage statements (JuliaLang#42260)

It seems that this change still doesn't resolve JuliaLang#42258 though.
LilithHafner pushed a commit to LilithHafner/julia that referenced this issue Mar 8, 2022
…JuliaLang#42262)

Essentially, this PR adds missing `ssaflags` deletion of `type_annotate!`.
Built on top of JuliaLang#42260.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:inference Type inference compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants