inference: Relax constprop recursion detection #40561
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Member
Author
|
@nanosoldier |
Collaborator
|
Something went wrong when running your job: Logs and partial data can be found here |
Member
Author
|
@nanosoldier |
Collaborator
|
Something went wrong when running your job: Logs and partial data can be found here |
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
Apr 27, 2021
Co-Authored-By: Keno Fisher <[email protected]>
Member
|
@nanosoldier runbenchmarks(ALL, vs=":master") |
Member
|
@nanosoldier |
Collaborator
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
vtjnash
approved these changes
Apr 27, 2021
Member
vtjnash
left a comment
vtjnash
left a comment
There was a problem hiding this comment.
Looks like we might need to fix a couple BaseBenchmarks.jl now, but that is a good problem to have 😂. AFAICT, it had no performance impact end-to-end (6h15 total run time sounds about typical currently), though most of that time is GC-scrubbing, so it may not be a useful measure.
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
Apr 27, 2021
Co-Authored-By: Keno Fisher <[email protected]>
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
Apr 27, 2021
Co-Authored-By: Keno Fisher <[email protected]>
Member
Author
|
Yeah I will inspect into those serious regressions. |
aviatesk
changed the title
Member
Author
|
@nanosoldier |
Collaborator
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
Member
|
Can't really reproduce the array regressions locally, so retriggering those again. @nanosoldier |
Collaborator
|
Your benchmark job has completed - possible performance regressions were detected. A full report can be found here. cc @christopher-dG |
At the moment, we restrict const prop whenever we see a cycle in methods being called. However, I think this condition can be relaxed slightly: In particular, if the type complexity limiting did not decide to limit the growth of the type in question, I think it should be fine to constant prop as long as there is no cycle in *method instances* (rather than just methods). Fixes JuliaLang#39915, replaces JuliaLang#39918 Co-Authored-By: Keno Fisher <[email protected]>
aviatesk
changed the title
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
May 27, 2021
aviatesk
added a commit
to aviatesk/JET.jl
that referenced
this pull request
May 27, 2021
simeonschaub
removed
the
merge me
aviatesk
added a commit
that referenced
this pull request
May 31, 2021
…_call_method_with_const_args` chain This PR refactors the `abstract_call_method` -> `abstract_call_method_with_const_args` chain, and simplifies the signature of `abstract_call_method_with_const_args`: the newly defined `MethodCallResult` struct wraps a result and context information of `abstract_method_call`, and is passed and consumed by the succeeding `abstract_call_method_with_const_args`. Although our constant-propagation heuristic will be likely to change in the future (as in #40561) and so the signature of `abstract_call_method_with_const_args` is very unstable, hopefully this PR makes it a bit more stable. As an additional benefit, now an external `AbstractInterpreter` can use the context information of `abstract_method_call` (especially `edge::MethodInstance`) within `maybe_get_const_prop_profitable`.
vtjnash
pushed a commit
that referenced
this pull request
May 31, 2021
…_call_method_with_const_args` chain (#41020) This PR refactors the `abstract_call_method` -> `abstract_call_method_with_const_args` chain, and simplifies the signature of `abstract_call_method_with_const_args`: the newly defined `MethodCallResult` struct wraps a result and context information of `abstract_method_call`, and is passed and consumed by the succeeding `abstract_call_method_with_const_args`. Although our constant-propagation heuristic will be likely to change in the future (as in #40561) and so the signature of `abstract_call_method_with_const_args` is very unstable, hopefully this PR makes it a bit more stable. As an additional benefit, now an external `AbstractInterpreter` can use the context information of `abstract_method_call` (especially `edge::MethodInstance`) within `maybe_get_const_prop_profitable`.
shirodkara
pushed a commit
to shirodkara/julia
that referenced
this pull request
Jun 9, 2021
At the moment, we restrict const prop whenever we see a cycle in methods being called. However, I think this condition can be relaxed slightly: In particular, if the type complexity limiting did not decide to limit the growth of the type in question, I think it should be fine to constant prop as long as there is no cycle in *method instances* (rather than just methods). Fixes JuliaLang#39915, replaces JuliaLang#39918 Co-authored-by: Keno Fisher <[email protected]>
shirodkara
pushed a commit
to shirodkara/julia
that referenced
this pull request
Jun 9, 2021
…_call_method_with_const_args` chain (JuliaLang#41020) This PR refactors the `abstract_call_method` -> `abstract_call_method_with_const_args` chain, and simplifies the signature of `abstract_call_method_with_const_args`: the newly defined `MethodCallResult` struct wraps a result and context information of `abstract_method_call`, and is passed and consumed by the succeeding `abstract_call_method_with_const_args`. Although our constant-propagation heuristic will be likely to change in the future (as in JuliaLang#40561) and so the signature of `abstract_call_method_with_const_args` is very unstable, hopefully this PR makes it a bit more stable. As an additional benefit, now an external `AbstractInterpreter` can use the context information of `abstract_method_call` (especially `edge::MethodInstance`) within `maybe_get_const_prop_profitable`.
johanmon
pushed a commit
to johanmon/julia
that referenced
this pull request
Jul 5, 2021
At the moment, we restrict const prop whenever we see a cycle in methods being called. However, I think this condition can be relaxed slightly: In particular, if the type complexity limiting did not decide to limit the growth of the type in question, I think it should be fine to constant prop as long as there is no cycle in *method instances* (rather than just methods). Fixes JuliaLang#39915, replaces JuliaLang#39918 Co-authored-by: Keno Fisher <[email protected]>
johanmon
pushed a commit
to johanmon/julia
that referenced
this pull request
Jul 5, 2021
…_call_method_with_const_args` chain (JuliaLang#41020) This PR refactors the `abstract_call_method` -> `abstract_call_method_with_const_args` chain, and simplifies the signature of `abstract_call_method_with_const_args`: the newly defined `MethodCallResult` struct wraps a result and context information of `abstract_method_call`, and is passed and consumed by the succeeding `abstract_call_method_with_const_args`. Although our constant-propagation heuristic will be likely to change in the future (as in JuliaLang#40561) and so the signature of `abstract_call_method_with_const_args` is very unstable, hopefully this PR makes it a bit more stable. As an additional benefit, now an external `AbstractInterpreter` can use the context information of `abstract_method_call` (especially `edge::MethodInstance`) within `maybe_get_const_prop_profitable`.
ffucci
pushed a commit
to ffucci/julia
that referenced
this pull request
Aug 11, 2022
In a similar spirit to JuliaLang#40561, we can relax the recursion detection to guarantee `:terminates` effect and allow the effects analysis to not taint `:terminates` effect when there are no cycles in `MethodInstance`s in a call graph. fix JuliaLang#45781
pcjentsch
pushed a commit
to pcjentsch/julia
that referenced
this pull request
Aug 18, 2022
In a similar spirit to JuliaLang#40561, we can relax the recursion detection to guarantee `:terminates` effect and allow the effects analysis to not taint `:terminates` effect when there are no cycles in `MethodInstance`s in a call graph. fix JuliaLang#45781
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
At the moment, we restrict const prop whenever we see a cycle in
methods being called. However, I think this condition can be relaxed
slightly: In particular, if the type complexity limiting did not
decide to limit the growth of the type in question, I think it
should be fine to constant prop as long as there is no cycle in
method instances (rather than just methods).
Fixes #39915, replaces #39918
Co-Authored-By: Keno Fisher [email protected]