inference: Don't confuse frames in different interpreters #53627
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
Diffractor's abstract interpreter sometimes needs to do side queries using the native interpreter. These are pushed onto the regular inference callstack in anticipation of a future where compiler plugins may want to recurse from the native interpreter back into the Diffractor abstract interpreter. However, this introduced a subtle challenge: When the native interpreter is looking at a frame that is currently on the inference stack, it would treat them as the same, incorrectly merging inference across the two abstract interpreters (which have different semantics and may not be confused). The caches for the two abstract interpreters were already different, so once things are inferred, there's no problem (likely because things were already inferred on the native interpreter), but if not, this could cause subtle and hard to debug problems.
vtjnash
reviewed
Mar 7, 2024
| @@ -689,6 +689,10 @@ function edge_matches_sv(interp::AbstractInterpreter, frame::AbsIntState, | |||
| if callee_method2 !== inf_method2 | |||
| return false | |||
| end | |||
| if isa(frame, InferenceState) && frame.interp !== interp | |||
There was a problem hiding this comment.
The isa check makes it seem like this is doing something incorrectly. Isn't this actually the is_same_frame query though on the line above here?
Anyways, SGTM as a hack, but I dislike that the NativeInterpreter seems like a gigantic stack object (136 bytes) that now needs to be memcmp on every frame against every other frame here.
There was a problem hiding this comment.
It's kind of is_same_frame, but on the method. Rather than comparing the interpreter, I think we could compare the cache_owner if you like that better.
There was a problem hiding this comment.
yeah, cache_owner does intuitively feel like the right level of abstraction to me
mkitti
pushed a commit
to mkitti/julia
that referenced
this pull request
Apr 13, 2024
…53627) Diffractor's abstract interpreter sometimes needs to do side queries using the native interpreter. These are pushed onto the regular inference callstack in anticipation of a future where compiler plugins may want to recurse from the native interpreter back into the Diffractor abstract interpreter. However, this introduced a subtle challenge: When the native interpreter is looking at a frame that is currently on the inference stack, it would treat them as the same, incorrectly merging inference across the two abstract interpreters (which have different semantics and may not be confused). The caches for the two abstract interpreters were already different, so once things are inferred, there's no problem (likely because things were already inferred on the native interpreter), but if not, this could cause subtle and hard to debug problems.
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.
Diffractor's abstract interpreter sometimes needs to do side queries using the native interpreter. These are pushed onto the regular inference callstack in anticipation of a future where compiler plugins may want to recurse from the native interpreter back into the Diffractor abstract interpreter. However, this introduced a subtle challenge: When the native interpreter is looking at a frame that is currently on the inference stack, it would treat them as the same, incorrectly merging inference across the two abstract interpreters (which have different semantics and may not be confused). The caches for the two abstract interpreters were already different, so once things are inferred, there's no problem (likely because things were already inferred on the native interpreter), but if not, this could cause subtle and hard to debug problems.