-
-
Notifications
You must be signed in to change notification settings - Fork 5.7k
Make EnterNode save/restore dynamic scope #52309
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
Changes from all commits
a583b92
6c8117b
1af57c5
8916dd3
8cf7e4f
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3265,6 +3265,19 @@ function typeinf_local(interp::AbstractInterpreter, frame::InferenceState) | |
| elseif isa(stmt, EnterNode) | ||
| ssavaluetypes[currpc] = Any | ||
| add_curr_ssaflag!(frame, IR_FLAG_NOTHROW) | ||
| if isdefined(stmt, :scope) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does inference / SROA need to understand that this is now a clobbering statement for the current_task scope field, since the scope field is still publicly visible? Or should we hide the field so that users can't break the invariants expected and proven of it?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think either of them can optimize these fields on the current task. I would be open to hiding the scope field further, but I didn't think it was that necessary. |
||
| scopet = abstract_eval_value(interp, stmt.scope, currstate, frame) | ||
| handler = frame.handlers[frame.handler_at[frame.currpc+1][1]] | ||
| @assert handler.scopet !== nothing | ||
| if !⊑(𝕃ᵢ, scopet, handler.scopet) | ||
| handler.scopet = tmerge(𝕃ᵢ, scopet, handler.scopet) | ||
| if isdefined(handler, :scope_uses) | ||
| for bb in handler.scope_uses | ||
| push!(W, bb) | ||
| end | ||
| end | ||
| end | ||
| end | ||
| @goto fallthrough | ||
| elseif isexpr(stmt, :leave) | ||
| ssavaluetypes[currpc] = Any | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2488,6 +2488,19 @@ function builtin_effects(𝕃::AbstractLattice, @nospecialize(f::Builtin), argty | |
| return Effects(EFFECTS_TOTAL; | ||
| consistent = (isa(setting, Const) && setting.val === :conditional) ? ALWAYS_TRUE : ALWAYS_FALSE, | ||
| nothrow = compilerbarrier_nothrow(setting, nothing)) | ||
| elseif f === Core.current_scope | ||
| nothrow = true | ||
| if length(argtypes) != 0 | ||
| if length(argtypes) != 1 || !isvarargtype(argtypes[1]) | ||
| return EFFECTS_THROWS | ||
| end | ||
| nothrow = false | ||
| end | ||
| return Effects(EFFECTS_TOTAL; | ||
| consistent = ALWAYS_FALSE, | ||
| notaskstate = false, | ||
| nothrow | ||
| ) | ||
| else | ||
| if contains_is(_CONSISTENT_BUILTINS, f) | ||
| consistent = ALWAYS_TRUE | ||
|
|
@@ -2554,6 +2567,32 @@ function memoryop_noub(@nospecialize(f), argtypes::Vector{Any}) | |
| return false | ||
| end | ||
|
|
||
| function current_scope_tfunc(interp::AbstractInterpreter, sv::InferenceState) | ||
| pc = sv.currpc | ||
| while true | ||
| handleridx = sv.handler_at[pc][1] | ||
| if handleridx == 0 | ||
| # No local scope available - inherited from the outside | ||
| return Any | ||
| end | ||
| pchandler = sv.handlers[handleridx] | ||
| # Remember that we looked at this handler, so we get re-scheduled | ||
| # if the scope information changes | ||
| isdefined(pchandler, :scope_uses) || (pchandler.scope_uses = Int[]) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can't we set
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We cannot, because we don't necessarily know all the callsites that will infer to a call to current_scope. We could make current_scope an expr to make it obvious, but I don't think it's required. |
||
| pcbb = block_for_inst(sv.cfg, pc) | ||
| if findfirst(==(pcbb), pchandler.scope_uses) === nothing | ||
| push!(pchandler.scope_uses, pcbb) | ||
| end | ||
| scope = pchandler.scopet | ||
| if scope !== nothing | ||
| # Found the scope - forward it | ||
| return scope | ||
| end | ||
| pc = pchandler.enter_idx | ||
| end | ||
| end | ||
| current_scope_tfunc(interp::AbstractInterpreter, sv) = Any | ||
|
|
||
| """ | ||
| builtin_nothrow(𝕃::AbstractLattice, f::Builtin, argtypes::Vector{Any}, rt) -> Bool | ||
|
|
||
|
|
@@ -2568,9 +2607,6 @@ end | |
| function builtin_tfunction(interp::AbstractInterpreter, @nospecialize(f), argtypes::Vector{Any}, | ||
| sv::Union{AbsIntState, Nothing}) | ||
| 𝕃ᵢ = typeinf_lattice(interp) | ||
| if f === tuple | ||
| return tuple_tfunc(𝕃ᵢ, argtypes) | ||
| end | ||
| if isa(f, IntrinsicFunction) | ||
| if is_pure_intrinsic_infer(f) && all(@nospecialize(a) -> isa(a, Const), argtypes) | ||
| argvals = anymap(@nospecialize(a) -> (a::Const).val, argtypes) | ||
|
|
@@ -2596,6 +2632,16 @@ function builtin_tfunction(interp::AbstractInterpreter, @nospecialize(f), argtyp | |
| end | ||
| tf = T_IFUNC[iidx] | ||
| else | ||
| if f === tuple | ||
| return tuple_tfunc(𝕃ᵢ, argtypes) | ||
| elseif f === Core.current_scope | ||
| if length(argtypes) != 0 | ||
| if length(argtypes) != 1 || !isvarargtype(argtypes[1]) | ||
| return Bottom | ||
| end | ||
| end | ||
| return current_scope_tfunc(interp, sv) | ||
| end | ||
| fidx = find_tfunc(f) | ||
| if fidx === nothing | ||
| # unknown/unhandled builtin function | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering the possibility that
EnterNodecould get more complicated in the future (I'm not sure if it is likely though), defining it asEffects(effects::Effects; overrides...)-like constructor might be a better move?