inference: fix too conservative effects for recursive cycles #54323
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
Keno
reviewed
May 2, 2024
| @@ -864,7 +864,8 @@ function typeinf_edge(interp::AbstractInterpreter, method::Method, @nospecialize | |||
| update_valid_age!(caller, frame.valid_worlds) | |||
| isinferred = is_inferred(frame) | |||
| edge = isinferred ? mi : nothing | |||
| effects = isinferred ? frame.result.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects | |||
| effects = isinferred ? frame.result.ipo_effects : # effects are adjusted already within `finish` for ipo_effects | |||
| adjust_effects(effects_for_cycle(frame.ipo_effects), method) | |||
There was a problem hiding this comment.
I don't believe this is legal unless we start cycle-converging effects (as we do for exct and rt), which I think is worth a try, but a much bigger change.
There was a problem hiding this comment.
So we probably need to introduce something similar to LimitedAccuracy for effects too?
I agree it's a necessary step but at the same time I don't foresee this PR causing any practical issues as is, so maybe we could merge it now and leave a TODO comment for future consideration?
There was a problem hiding this comment.
start cycle-converging effects (as we do for exct and rt)
Actually, I was reflecting on this for unrelated reasons, and realized that this is probably not actually true. For most of these bits, the results are not cycle-converged like exct and rt, but rather are like world-age min/max. The difference is very significant, as world-age-range is converged very cheaply in finish, as all items in the cycle must have exactly the same bits and simply is the intersection of each partial computation (without dependencies that can change other parts of the computation), unlike those others which have unique results which can have cyclic dependencies on each other.
So as long as you add a path to finish that unions together all of the effects (where we do the same for world age range), we should be able to merge and backport this this without the correctness regression.
There was a problem hiding this comment.
Thanks for your suggestion Jameson. Do you think 960da62 correctly implements your idea?
I generally agree with this direction, but there seem to be one difference in world-age-range and effects regarding cycles involving Core.Compiler._return_type. In such cycles, the world-age-range should be the same for all frames, but effects do not necessarily have to be, and especially even when there is a cycle in abstract interpretation (via _return_type_tfunc), it doesn't always need to taint the termination of runtime effects (#49119). As a result 960da62 breaks the test cases added in #49119.
59 tasks
|
Added a TODO comment, and I'm planning to merge this soon. |
|
How did you end up addressing the correctness problem Keno noted in his review? |
|
I plan to address that in the future. Practically I don't think any issues will arise with this commit as it is. |
KristofferC
added a commit
that referenced
this pull request
May 28, 2024
Backported PRs: - [x] #53665 <!-- use afoldl instead of tail recursion for tuples --> - [x] #53976 <!-- LinearAlgebra: LazyString in interpolated error messages --> - [x] #54005 <!-- make `view(::Memory, ::Colon)` produce a Vector --> - [x] #54010 <!-- Overload `Base.literal_pow` for `AbstractQ` --> - [x] #54069 <!-- Allow PrecompileTools to see MI's inferred by foreign abstract interpreters --> - [x] #53750 <!-- inference correctness: fields and globals can revert to undef --> - [x] #53984 <!-- Profile: fix heap snapshot is valid char check --> - [x] #54102 <!-- Explicitly compute stride in unaliascopy for SubArray --> - [x] #54070 <!-- Fix integer overflow in `skip(s::IOBuffer, typemax(Int64))` --> - [x] #54013 <!-- Support case-changes to Annotated{String,Char}s --> - [x] #53941 <!-- Fix writing of AnnotatedChars to AnnotatedIOBuffer --> - [x] #54137 <!-- Fix typo in docs for `partialsortperm` --> - [x] #54129 <!-- use correct size when creating output data from an IOBuffer --> - [x] #54153 <!-- Fixup IdSet docstring --> - [x] #54143 <!-- Fix `make install` from tarballs --> - [x] #54151 <!-- LinearAlgebra: Correct zero element in `_generic_matvecmul!` for block adj/trans --> - [x] #54213 <!-- Add `public` statement to `Base.GC` --> - [x] #54222 <!-- Utilize correct tbaa when emitting stores of unions. --> - [x] #54233 <!-- set MAX_OS_WRITE on unix --> - [x] #54255 <!-- fix `_checked_mul_dims` in the presence of 0s and overflow. --> - [x] #54259 <!-- Fix typo in `readuntil` --> - [x] #54251 <!-- fix typo in gc_mark_memory8 when chunking a large array --> - [x] #54276 <!-- Fix solve for complex `Hermitian` with non-vanishing imaginary part on diagonal --> - [x] #54248 <!-- ensure package callbacks are invoked when no valid precompile file exists for an "auto loaded" stdlib --> - [x] #54308 <!-- Implement eval-able AnnotatedString 2-arg show --> - [x] #54302 <!-- Specialised substring equality for annotated strs --> - [x] #54243 <!-- prevent `package_callbacks` to run multiple time for a single package --> - [x] #54350 <!-- add a precompile signature to Artifacts code that is used by JLLs --> - [x] #54331 <!-- correctly track freed bytes in jl_genericmemory_to_string --> - [x] #53509 <!-- revert moving "creating packages" from Pkg.jl --> - [x] #54335 <!-- When accessing the data pointer for an array, first decay it to a Derived Pointer --> - [x] #54239 <!-- Make sure `fieldcount` constant-folds for `Tuple{...}` --> - [x] #54288 - [x] #54067 - [x] #53715 <!-- Add read/write specialisation for IOContext{AnnIO} --> - [x] #54289 <!-- Rework annotation ordering/optimisations --> - [x] #53815 <!-- create phantom task for GC threads --> - [x] #54130 <!-- inference: handle `LimitedAccuracy` in `handle_global_assignment!` --> - [x] #54428 <!-- Move ConsoleLogging.jl into Base --> - [x] #54332 <!-- Revert "add unsetindex support to more copyto methods (#51760)" --> - [x] #53826 <!-- Make all command-line options documented in all related files --> - [x] #54465 <!-- typeintersect: conservative typevar subtitution during `finish_unionall` --> - [x] #54514 <!-- typeintersect: followup cleanup for the nothrow path of type instantiation --> - [x] #54499 <!-- make `@doc x` work without REPL loaded --> - [x] #54210 <!-- attach finalizer in `mmap` to the correct object --> - [x] #54359 <!-- Pkg REPL: cache `pkg_mode` lookup --> Non-merged PRs with backport label: - [ ] #54471 <!-- Actually setup jit targets when compiling packageimages instead of targeting only one --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #54323 <!-- inference: fix too conservative effects for recursive cycles --> - [ ] #54322 <!-- effects: add new `@consistent_overlay` macro --> - [ ] #54191 <!-- make `AbstractPipe` public --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #53882 <!-- Warn about cycles in extension precompilation --> - [ ] #53707 <!-- Make ScopedValue public --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [ ] #53286 <!-- Raise an error when using `include_dependency` with non-existent file or directory --> - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
60 tasks
aviatesk
force-pushed
the
avi/issue52938
branch
3 times, most recently
from
c303fed
to
dd692a8
Compare
|
This PR breaks one of the test cases in #49119, but that case might not be very important in reality. That is because if function |
aviatesk
force-pushed
the
avi/issue52938
branch
3 times, most recently
from
9f43a32
to
d548088
Compare
aviatesk
added a commit
that referenced
this pull request
Jun 5, 2024
#54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. This commit uses a somewhat hacky approach to resolve this. It identifies statements involved in the cycle comparing `stmt_edges` to `callers_in_cycle`, and updates `ssaflags` according to new cycle valid effects if necessary. This resolves the issue, but ideally, it should be implemented more safely with the new `edges` format that will be implemented in the future. For now, this approach should be okay.
aviatesk
added a commit
that referenced
this pull request
Jun 6, 2024
#54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. This commit uses a somewhat hacky approach to resolve this. It identifies statements involved in the cycle comparing `stmt_edges` to `callers_in_cycle`, and updates `ssaflags` according to new cycle valid effects if necessary. This resolves the issue, but ideally, it should be implemented more safely with the new `edges` format that will be implemented in the future. For now, this approach should be okay.
aviatesk
added a commit
that referenced
this pull request
Jun 6, 2024
#54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. This commit uses a somewhat hacky approach to resolve this. It identifies statements involved in the cycle comparing `stmt_edges` to `callers_in_cycle`, and updates `ssaflags` according to new cycle valid effects if necessary. This resolves the issue, but ideally, it should be implemented more safely with the new `edges` format that will be implemented in the future. For now, this approach should be okay.
aviatesk
added a commit
that referenced
this pull request
Jun 6, 2024
#54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. This commit uses a somewhat hacky approach to resolve this. It identifies statements involved in the cycle comparing `stmt_edges` to `callers_in_cycle`, and updates `ssaflags` according to new cycle valid effects if necessary. This resolves the issue, but ideally, it should be implemented more safely with the new `edges` format that will be implemented in the future. For now, this approach should be okay.
aviatesk
added a commit
that referenced
this pull request
Jun 6, 2024
#54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. To resolve this issue, this commit traverses `cycle_backedges` to visit statements involved in the cycle, and updates each `ssaflags` according to new cycle valid effects if necessary.
aviatesk
added a commit
that referenced
this pull request
Jun 7, 2024
…#54689) #54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. To resolve this issue, this commit traverses `cycle_backedges` to visit statements involved in the cycle, and updates each `ssaflags` according to new cycle valid effects if necessary.
aviatesk
added a commit
that referenced
this pull request
Jun 7, 2024
…#54689) #54323 ensures that all frames within a cycle have the same, cycle valid effects. However, `src.ssaflags` is calculated using partial effects, so when the effects of a `frame` within the cycle are updated, there would be an inconsistency between `frame.ipo_effects` and `frame.src.ssaflags`. Due to this inconsistency, #54323 breaks the test cases from #51092, when backported to v1.11. On the surface this is because #52999 hasn't been backported to v1.11, but the fundamental issue lies in this inconsistency between cycle effects and `ssaflags`. To resolve this issue, this commit traverses `cycle_backedges` to visit statements involved in the cycle, and updates each `ssaflags` according to new cycle valid effects if necessary.
aviatesk
pushed a commit
that referenced
this pull request
Jun 24, 2024
Follow-on from #54323 @aviatesk I think this should also be disabled on linux? It's been failing there On `test i686-linux-gnu` ``` math (7) | failed at 2024-06-23T13:58:26.792 Test Failed at /cache/build/tester-amdci5-8/julialang/julia-buildkite/julia-2d4ef8a238/share/julia/test/math.jl:1607 Expression: Core.Compiler.is_foldable(effects) Context: effects = (+c,+e,+n,!t,+s,+m,+u,+o) T = Float64 ```
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.
The
:terminateseffect bit must be conservatively tainted unless recursion cycle has been fully resolved. As for other effects, there's no need to taint them at this moment because they will be tainted as we try to resolve the cycle.