Improve inference SCC '[limited]' code computation #39116
Merged
Conversation
This file contains 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
vtjnash
added
compiler:inference
vtjnash
force-pushed
the
jn/inference-nolimited
branch
from
75181fb
to
bfc3b2a
Compare
|
I'm not sure why the status didn't update when those finished. I suppose it got confused. The only one waiting on is https://build.julialang.org/#/builders/39/builds/6682 |
Sacha0
reviewed
Jan 6, 2021
| @@ -56,6 +56,7 @@ end | |||
|
|
|||
| # Wraps a type and represents that the value may also be undef at this point. | |||
| # (only used in optimize, not abstractinterpret) | |||
| # N.B. in the lattice, this is epsilon bigger than `typ` (even Any) | |||
There was a problem hiding this comment.
nota bene (tr. note well!)
vtjnash
force-pushed
the
jn/inference-nolimited
branch
from
bfc3b2a
to
387903a
Compare
|
Don't know what the doctest machine segfaults because of this, since I can't reproduce and this otherwise is done. |
Sacha0
reviewed
Jan 12, 2021
| elseif typeb.causes ⊆ typea.causes | ||
| causes = typea.causes | ||
| else | ||
| causes = union!(copy(typea.causes), typeb.causes) |
There was a problem hiding this comment.
Any reason not to use plain union here?
There was a problem hiding this comment.
Yeah, it doesn’t make an IdDict. We should perhaps change that
There was a problem hiding this comment.
Ah, wait, that is a holdover from when this was a Dict and used merge, for union on IdSet, it would be okay.
julia> merge(IdDict(), IdDict())
Dict{Any, Any}()
387903a
to
b5bc366
Compare
This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary.
vtjnash
force-pushed
the
jn/inference-nolimited
branch
from
b5bc366
to
cf01a76
Compare
|
Running doctest with FORCE_ASSERTIONS, it seems to have turned out to be fixed by #39246. Let's see how this does now. |
|
Probably needs a rebase to catch #39263 to make doctests pass. |
|
That is acceptable, we know this would have passed. @JeffBezanson GTG? |
aviatesk
reviewed
Jan 16, 2021
| @@ -443,7 +454,8 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp | |||
| # (non-typically, this means that we lose the ability to detect a guaranteed StackOverflow in some cases) | |||
| return Any, true, nothing | |||
| end | |||
| poison_callstack(sv, topmost::InferenceState, true) | |||
| topmost = topmost::InferenceState | |||
| poison_callstack(sv, topmost.parent === nothing ? topmost : topmost.parent) | |||
There was a problem hiding this comment.
nit pick;
Suggested change
| poison_callstack(sv, topmost.parent === nothing ? topmost : topmost.parent) | |
| parent = topmost.parent | |
| poison_callstack(sv, parent === nothing ? topmost : parent) |
|
This is great, thanks Jameson! :) |
JeffBezanson
approved these changes
Jan 20, 2021
|
Hm, the doctest failure looks real. Perhaps Kristoffer's comment
is relevant, given it looks like the last rebase was before 39263? |
KristofferC
pushed a commit
that referenced
this pull request
Jan 21, 2021
This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary. (cherry picked from commit 5f10eb9)
vtjnash
added a commit
that referenced
this pull request
Jan 21, 2021
This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary. (cherry picked from commit 5f10eb9)
KristofferC
pushed a commit
that referenced
this pull request
Feb 1, 2021
This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary. (cherry picked from commit 5f10eb9)
ElOceanografo
pushed a commit
to ElOceanografo/julia
that referenced
this pull request
May 4, 2021
…39116) This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary.
antoine-levitt
pushed a commit
to antoine-levitt/julia
that referenced
this pull request
May 9, 2021
…39116) This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary.
staticfloat
pushed a commit
that referenced
this pull request
Dec 23, 2022
This helps refine our knowledge of the `[limited]` flag setting, which previously would always exclude a result from the cache when hitting a cycle. However, we really only need to exclude a result if the result might be dependent on that flag setting. That makes this formally part of the lattice, though can be annoying to work with yet another wrapper, so we try to add/remove it late/early to propagate it when necessary. (cherry picked from commit 5f10eb9)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
This implements the idea described in #38517, with other substantial additional refinements.