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

Make EnterNode save/restore dynamic scope #52309

Merged
merged 5 commits into from
Dec 13, 2023
Merged

Make EnterNode save/restore dynamic scope #52309

merged 5 commits into from
Dec 13, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Nov 26, 2023

As discussed in #51352, this gives EnterNode the ability to set (and restore on leave or catch edge) jl_current_task->scope. Manual modifications of the task field after the task has started are considered undefined behavior. In addition, we gain a new intrinsic to access current_task->scope and both inference and the optimizer will forward scopes from EnterNodes to this intrinsic (non-interprocedurally). Together with #51993 this is sufficient to fully optimize ScopedValues (non-interprocedurally at least).

Comment on lines 1835 to +1836
std::map<int, jl_varinfo_t> phic_slots;
std::map<int, std::pair<Value*, Value*> > scope_restore;
Copy link
Member

Choose a reason for hiding this comment

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

I thought we were preferring DenseMap since it has better performance and [] behavior, or maybe that does still need .at(), but at least has that operator?

Copy link
Member Author

Choose a reason for hiding this comment

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

I thought std::map was better for small maps, but I don't really care either way.

Copy link
Member

@gbaraldi gbaraldi Nov 27, 2023

Choose a reason for hiding this comment

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

For small maps we should probably use SmallDenseMap

std::map has similar characteristics to std::set: it uses a single allocation per pair inserted into the map, it offers log(n) lookup with an extremely large constant factor, imposes a space penalty of 3 pointers per pair in the map, etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like the gcc version that we have on CI has a false positive warning in SmallDenseMap. I really don't think it's worth micro-optimizing the datastructure here, so I'm just gonna switch this back to std::map

src/interpreter.c Outdated Show resolved Hide resolved
src/interpreter.c Outdated Show resolved Hide resolved
src/julia-syntax.scm Outdated Show resolved Hide resolved
base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
@@ -2488,6 +2488,12 @@ 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
length(argtypes) == 0 || return Effects(EFFECTS_THROWS; consistent=ALWAYS_FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

argtypes == Any[Vararg] is valid here too

Copy link
Member

Choose a reason for hiding this comment

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

And if length(argtypes) >= 1 && !isvarargtype(argtypes[1]), then isn't it just EFFECTS_THROWS?

ct.scope = $(Scope)(current_scope, $(exprs...))
$(Expr(:tryfinally, esc(ex), :(ct.scope = current_scope)))
end
Expr(:tryfinally, esc(ex), :(), :($(Scope)($(Core.current_scope)()::Union{Nothing, Scope}, $(exprs...))))
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
Expr(:tryfinally, esc(ex), :(), :($(Scope)($(Core.current_scope)()::Union{Nothing, Scope}, $(exprs...))))
Expr(:tryfinally, esc(ex), :(), :(Scope(Core.current_scope()::Union{Nothing, Scope}, $(exprs...))))

Copy link
Member

Choose a reason for hiding this comment

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

It appears that for this PR to work as intended, this call of Scope(...) needs to be constant-folded? And am I right in assuming that #51993 will make this happen? I'm considering if a standalone test case can be crafted for the basic functionality this PR implements.

Copy link
Member Author

Choose a reason for hiding this comment

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

No, constant-folding is not required. The inference support isn't really needed either for the base version of this, but I anticipate needing it downstream.

@@ -3259,6 +3259,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)
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

base/compiler/ssair/passes.jl Outdated Show resolved Hide resolved
@@ -2488,6 +2488,12 @@ 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
length(argtypes) == 0 || return Effects(EFFECTS_THROWS; consistent=ALWAYS_FALSE)
Copy link
Member

Choose a reason for hiding this comment

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

And if length(argtypes) >= 1 && !isvarargtype(argtypes[1]), then isn't it just EFFECTS_THROWS?

EnterNode(old::EnterNode, new_dest::Int) = isdefined(old, :scope) ?
EnterNode(new_dest, old.scope) : EnterNode(new_dest)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
EnterNode(old::EnterNode, new_dest::Int) = isdefined(old, :scope) ?
EnterNode(new_dest, old.scope) : EnterNode(new_dest)
function EnterNode(node::EnterNode;
dest::Int=node.dest,
scope)
@nospecialize scope
if !(@isdefined scope)
isdefined(node, :scope) || return EnterNode(dest)
scope = node.scope
end
return EnterNode(dest, scope)
end

Considering the possibility that EnterNode could get more complicated in the future (I'm not sure if it is likely though), defining it as Effects(effects::Effects; overrides...)-like constructor might be a better move?

function current_scope_tfunc(interp::AbstractInterpreter, sv::InferenceState)
pc = sv.currpc
while true
handleridx = sv.handler_at[pc][2]
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
handleridx = sv.handler_at[pc][2]
handleridx = sv.handler_at[pc][1]

Shouldn't this look at the active try region rather than the catch region?

Copy link
Member Author

Choose a reason for hiding this comment

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

Good catch

base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
ct.scope = $(Scope)(current_scope, $(exprs...))
$(Expr(:tryfinally, esc(ex), :(ct.scope = current_scope)))
end
Expr(:tryfinally, esc(ex), :(), :($(Scope)($(Core.current_scope)()::Union{Nothing, Scope}, $(exprs...))))
Copy link
Member

Choose a reason for hiding this comment

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

It appears that for this PR to work as intended, this call of Scope(...) needs to be constant-folded? And am I right in assuming that #51993 will make this happen? I'm considering if a standalone test case can be crafted for the basic functionality this PR implements.

@aviatesk
Copy link
Member

Manual modifications of the task field after the task has started are considered undefined behavior.

Maybe we should make it the only way to set a scope, and raise an explicit error if user manually modifies the scope field of a task?

@Keno
Copy link
Member Author

Keno commented Nov 27, 2023

Maybe we should make it the only way to set a scope, and raise an explicit error if user manually modifies the scope field of a task?

I do think we need to retain the ability to set the scope before a task is started. It's not used here, but I suspect we may want it in the future. However, we could certainly have a setproperty! that asserts that the task has not been started yet.

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[])
Copy link
Member

Choose a reason for hiding this comment

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

Can't we set scope_uses within compute_trycatch?

Copy link
Member Author

Choose a reason for hiding this comment

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

@brenhinkeller brenhinkeller added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Dec 7, 2023
Keno added 5 commits December 11, 2023 02:18
As discussed in #51352, this gives `EnterNode` the ability to set
(and restore on leave or catch edge) jl_current_task->scope. Manual
modifications of the task field after the task has started are
considered undefined behavior. In addition, we gain a new intrinsic
to access current_task->scope and both inference and the optimizer
will forward scopes from EnterNodes to this intrinsic (non-interprocedurally).
Together with #51993 this is sufficient to fully optimize ScopedValues
(non-interprocedurally at least).
@aviatesk aviatesk mentioned this pull request Dec 13, 2023
@Keno Keno merged commit 406f5b4 into master Dec 13, 2023
7 checks passed
@Keno Keno deleted the kf/enterscope branch December 13, 2023 04:17
aviatesk added a commit that referenced this pull request Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler:optimizer Optimization passes (mostly in base/compiler/ssair/)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants