inference: allocate CodeInstance with inference results ASAP #54709
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
force-pushed
the
jn/early-codeinstance
branch
from
a117769
to
cb7c95d
Compare
aviatesk
reviewed
Jun 6, 2024
Update the CodeInstance after optimizations with code (if possible), but construct it early so that there will be the option of using it for edges and invoke during optimizations, without problems with cycles.
vtjnash
force-pushed
the
jn/early-codeinstance
branch
from
cb7c95d
to
7c05e1a
Compare
Keno
reviewed
Jun 7, 2024
vtjnash
force-pushed
the
jn/early-codeinstance
branch
from
7c05e1a
to
5cc5640
Compare
aviatesk
reviewed
Jun 10, 2024
Comment on lines
+456
to
+461
| # we might have introduced a limit marker, but we should know it must be sv and other callers_in_cycle | ||
| #@assert !isempty(sv.callers_in_cycle) | ||
| # FIXME: this assert fails, appearing to indicate there is a bug in filtering this list earlier. | ||
| # In particular (during doctests for example), during inference of | ||
| # show(Base.IOContext{Base.GenericIOBuffer{Memory{UInt8}}}, Base.Multimedia.MIME{:var"text/plain"}, LinearAlgebra.BunchKaufman{Float64, Array{Float64, 2}, Array{Int64, 1}}) | ||
| # we observed one of the ssavaluetypes here to be Core.Compiler.LimitedAccuracy(typ=Any, causes=Core.Compiler.IdSet(getproperty(LinearAlgebra.BunchKaufman{Float64, Array{Float64, 2}, Array{Int64, 1}}, Symbol))) |
There was a problem hiding this comment.
AFAIU, this assertion should be failing because of the removal of push!(frames, frame) on L239, i.e. it fails when LimitedAccuracy is introduced in cases of self-recursion. Was this an intentional change?
There was a problem hiding this comment.
The code at L239 seemed to have been making this accidentally pass and hiding this bug. In particular, there didn't seem to be self-recursion here (and LimitedAccuracy should not occur if there is self-recursion, typically)?
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.
Update the CodeInstance after optimizations with the code (if possible), but construct it early so that there will be the option of using it for edges and invoke during optimizations, without problems with cycles.
@Keno the intent is that this will let us use CodeInstance as targets for
:invokecalls, as discussed in https://hackmd.io/bpLnKm48TbWP_U_PGWgEVg. It complicates the life-cycle of a CodeInstance a little bit, so I don't think we have yet fully specified the thread-safety of this and which are valid transitions here (in general, it is only legal to narrow these properties or widen the worlds), but I think that is okay to postpone.