-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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: handle GlobalRef
based on assume_bindings_static
#54148
base: master
Are you sure you want to change the base?
Conversation
base/compiler/ssair/ir.jl
Outdated
@@ -428,22 +428,26 @@ struct IRCode | |||
cfg::CFG | |||
new_nodes::NewNodeStream | |||
meta::Vector{Expr} | |||
assume_bindings_static::Bool # XXX propagate `interp::AbstractInterpreter` here? |
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.
I don't think adding this field here is reasonable. We should find some other way to propagate this information.
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.
Yeah.. Would it be too hacky to temporarily keep Expr(:assume_bindings_static, true)
in meta::Vector{Expr}
?
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.
I think that would be fine.
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.
I'm not very happy with that solution.
Assuming that some implicit subset of InferenceParams have been copied into the meta
vector is very subtle and bakes implicit pipeline ordering into the construction of an IRCode.
For example, it looks like a caller of ssa_inlining_pass!
will have their Interpreter's parameters ignored when stmt_effect_flags
are computed internally (despite the fact that InliningState
contains an interp
which has params).
Is the caller supposed to provide a "meta-expanded" IRCode? How are they supposed to know which settings will be ignored?
cf8295d
to
f714901
Compare
@nanosoldier |
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. |
f714901
to
94d07a6
Compare
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.
I really don't like how this means that our optimization passes now pull some information from InferenceParams(interp)
and other information from secret/undocumented entries in the ir.meta
Vector.
I think this needs a more significant re-work, so that we always have a coherent view of the lattice + inference parameters within any given function/pass.
Indeed, this could be a great chance to introduce something similar to LLVM's optimization manager, which would centralize the management of optimization passes. I'll start thinking about the design. |
Following up #53750. External abstract interpreters with `assume_bindings_static` enabled assume nothrow-ness of defined `GlobalRef`s no matter if they are constant during abstract interpretation. However, post-#53750, the optimizer’s verification switched to stricter checks, causing errors in such interpreters. This commit makes `IRCode` hold `assume_bindings_static::Bool` info to ensure that inlining and verification behaviors are synced with abstract interpretation.
94d07a6
to
40fb628
Compare
Following up #53750.
External abstract interpreters with
assume_bindings_static
enabled assume nothrow-ness of definedGlobalRef
s no matter if they are constant during abstract interpretation. However, post-#53750, the optimizer’s verification switched to stricter checks, causing errors in such interpreters. This commit makesIRCode
holdassume_bindings_static::Bool
info to ensure that inlining and verification behaviors are synced with abstract interpretation.