inference: fix bad effects for recursion #51092
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
kind:bugfix
|
Fixes #51090 |
aviatesk
reviewed
Aug 29, 2023
vtjnash
force-pushed
the
jn/51090
branch
2 times, most recently
from
259e7b1
to
39911bc
Compare
aviatesk
reviewed
Aug 31, 2023
aviatesk
reviewed
Aug 31, 2023
| return EdgeCallResult(frame.bestguess, edge, frame.ipo_effects) # effects are adjusted already within `finish` | ||
| isinferred = is_inferred(frame) | ||
| edge = isinferred ? mi : nothing | ||
| effects = isinferred ? frame.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects |
There was a problem hiding this comment.
I came up with this fix for typejoin accuracy for external AbstractInterpreter with overlay table:
Suggested change
| effects = isinferred ? frame.ipo_effects : adjust_effects(Effects(), method) # effects are adjusted already within `finish` for ipo_effects | |
| if isinferred | |
| effects = frame.ipo_effects # effects are adjusted already within `finish` | |
| else | |
| # if this method isn't overlayed, we don't need to taint `:nonoverlayed` | |
| effects = is_nonoverlayed(method) ? EFFECTS_UNKNOWN : Effects() | |
| effects = adjust_effects(effects, method) | |
| end |
But I just realized this isn't correct, because we can't guarantee that any overlay methods aren't involved within a recursive call graph..
And now I think this may be more problematic that I thought, because now we basically make external AbstractInterpreter with overlay table unable to perform concrete eval on any recursive methods.
I think there are two options:
- make inference keep track of a set of methods that appear within a call graph and check if there are any overlay methods here, which sounds a bit complicated but at the same time may be useful for the other analyses too
- move forward effects: allow override of
:nonoverlayedeffect bit (and rename it to:native_executable) #51080 and annotate:nonoverlayed typejoin, which seems awkward honestly, because it is obvious thattypejoinis not overlayed
There was a problem hiding this comment.
typejoin is probably considered to be :total, which would possibly already work. The only reason this method is a problem is that it is annotated :terminates, since otherwise inference couldn't derive that information itself anyways.
…pute_eltype
- Repair definition of Core._foldable_meta
- Handle Vararg better in eltype
- Mark has_free_typevars ccall as total
- Mark tailjoin foldable explicitly also, since it calls typejoin recursively
- Handle non-constructable Tuple{:a,2} and Tuple{T} types
- Prepare special code for inlining (improves generic effects too)
- Only optimize typejoin in concrete eval, not needlessly generating
specializations otherwise on specific types
The compiler seems better able to deal with this version of
_compute_eltype, and it does not seem that necessary to form the IdSet.
Effects are not converged, so they need to always be correct, and not a bestguess, even during recursion.
vtjnash
referenced
this pull request
Sep 7, 2023
- Add documentation to CodegenParams fields per request in #51123 - Fix compare_cgparams which hadn't been updated for recent additions - Remove unused and untested generic_context The latter was a codegen option that I added, but eventually ended up not using anywhere, so remove it for the time being. That said, I may end up in a situation where I need it again in the very near future, so I may end up eating my words here, but if I need to put it back, I'll include a test at least ;).
58 tasks
KristofferC
added a commit
that referenced
this pull request
Oct 2, 2023
Backported PRs: - [x] #48625 <!-- add replace(io, str, patterns...) --> - [x] #48387 <!-- Improve documentation of sort-related functions --> - [x] #48363 <!-- Revise sort.md and docstrings in sort.jl --> - [x] #48977 <!-- Update SparseArrays.jl stdlib for SuiteSparse 7 --> - [x] #50719 <!-- fix `CyclePadding(::DataType)` --> - [x] #50694 <!-- inference: permit recursive type traits --> - [x] #50860 <!-- Add `Base.get_extension` to docs/API --> - [x] #50594 <!-- Disallow non-index Integer types in isassigned --> - [x] #50802 <!-- Makes IntrusiveLinkedListSynchronized mutable to avoid allocation on poptask --> - [x] #50858 <!-- Add a `threadpool` parameter to `Channel` constructor --> - [x] #50874 <!-- Restrict COFF to a single thread when symbol count is high --> - [x] #50822 <!-- Add default method for setmodifiers! --> - [x] #50730 <!-- Fix integer overflow in `isapprox` --> - [x] #50850 <!-- Remove weird Rational dispatch and add pi functions to list --> - [x] #50809 <!-- Limit type-printing in MethodError --> - [x] #50915 <!-- Add note the `Task` about sticky bit --> - [x] #50929 <!-- when widening tuple types in tmerge, only widen the complex parts --> - [x] #50928 <!-- Bump JuliaSyntax to 0.4.6 --> - [x] #50959 <!-- Update libssh2 patches --> - [x] #50823 <!-- Make ranges more robust with unsigned indexes. --> - [x] #48542 <!-- Add docs on task-specific buffering using multithreading --> - [x] #50912 <!-- Separate foreign threads into a :foreign threadpool --> - [x] #51010 <!-- Add ORIGIN to SuiteSparse rpath on Linux/FreeBSD --> - [x] #50753 <!-- cat: remove unused promote_eltype methods that confuse inference --> - [x] #51027 <!-- Implement realloc accounting correctly --> - [x] #51019 <!-- fix a case of potentially use of undefined variable when handling error in distributed message processing --> - [x] #51039 <!-- Revert "Optimize findall(f, ::AbstractArray{Bool}) (#42202)" --> - [x] #51036 <!-- add missing invoke edge for nospecialize targets --> - [x] #51042 <!-- inference: fix return_type_tfunc modeling of concrete functions --> - [x] #51114 <!-- Workaround upstream FreeBSD issue #272992 --> - [x] #50892 <!-- Add `JL_DLLIMPORT` to `small_typeof` declaration --> - [x] #51154 <!-- broadcast: use recursion rather than ntuple to map over a tuple --> - [x] #51153 <!-- fix debug typo in "add missing invoke edge for nospecialize targets (#51036)" --> - [x] #51222 <!-- Check again if the tty is open inside the IO lock --> - [x] #51236 <!-- Add lock around uv_unref during init --> - [x] #51243 <!-- GMP: Gracefully handle more overflows. --> - [x] #51254 <!-- Ryu: make sure adding zeros does not overwrite trailing dot --> - [x] #51175 <!-- shorten stale_age for cachefile lock --> - [x] #51300 <!-- fix method definition error for bad vararg --> - [x] #51307 <!-- fix force-throw ctrl-C on Windows --> - [x] #51303 <!-- ensure revising structs is safe --> - [x] #51393 - [x] #51403 Need manual backport: - [x] #51009 <!-- fix #50562, regression in `in` of tuple of Symbols --> - [x] #51053 <!-- Bump Statistics stdlib --> - [x] #51013 <!-- fix #50709, type bound error due to free typevar in sparam env --> - [x] #51305 <!-- reduce test time for rounding and floatfuncs --> Contains multiple commits, manual intervention needed: - [ ] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> - [x] #51247 <!-- Check if malloc has succeeded before incrementing gc stats --> - [x] #51294 <!-- use LibGit2_jll for LibGit2 library --> Non-merged PRs with backport label: - [ ] #51132 <!-- Handle `AbstractQ` in concatenation --> - [x] #51029 <!-- testdefs: make sure that if a test set changes the active project, they change it back when they're done --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions --> - [x] #50385 <!-- Precompile pidlocks: add to NEWS and docs --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
KristofferC
added a commit
that referenced
this pull request
Nov 2, 2023
Backported PRs: - [x] #50932 <!-- types: fix hash values of Vararg --> - [x] #50975 <!-- Use rr-safe `nopl; rdtsc` sequence --> - [x] #50989 <!-- fix incorrect results in `expm1(::Union{Float16, Float32})` --> - [x] #51284 <!-- Avoid infinite loop when doing SIGTRAP in arm64-apple --> - [x] #51332 <!-- Add s4 field to Xoshiro --> - [x] #51397 <!-- call Pkg precompile hook in latest world --> - [x] #51405 <!-- Remove fallback that assigns a module to inlined frames. --> - [x] #51491 <!-- Throw clearer ArgumentError for strip with two string args --> - [x] #51531 <!-- fix `_tryonce_download_from_cache` (busybox.exe download error) --> - [x] #51541 <!-- Fix string index error in tab completion code --> - [x] #51530 <!-- Don't mark nonlocal symbols as hidden --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51512 <!-- avoid limiting Type{Any} to Type --> - [x] #51595 <!-- reset `maxprobe` on `empty!` --> - [x] #51582 <!-- Aggressive constprop in LinearAlgebra.wrap --> - [x] #51592 <!-- correctly track element pointer in heap snapshot --> - [x] #51326 <!-- complete false & true more generally as vals --> - [x] #51376 <!-- make `hash(::Xoshiro)` compatible with `==` --> - [x] #51557 <!-- Fix last startup & shutdown precompiles --> - [x] #51845 - [x] #51840 - [x] #50663 <!-- Fix Expr(:loopinfo) codegen --> - [x] #51863 <!-- LLVM 15.0.7-9 --> Contains multiple commits, manual intervention needed: - [ ] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [ ] #51092 <!-- inference: fix bad effects for recursion --> Non-merged PRs with backport label: - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia --> - [ ] #51414 <!-- improvements on GC scheduler shutdown --> - [ ] #51366 <!-- Handle infix operators in REPL completion --> - [ ] #50919 <!-- Code loading: do the "skipping mtime check for stdlib" check regardless of the value of `ispath(f)` --> - [ ] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions in Base --> - [ ] #49805 <!-- Limit TimeType subtraction to AbstractDateTime -->
|
As this breaks GPU codegen, see JuliaGPU/GPUCompiler.jl#506, I would propose not backporting this anymore so late in the 1.10 release process (cc @KristofferC). |
maleadt
removed
the
backport 1.10
|
Effects must be correct, otherwise we might eg tab-complete something that crashes the whole process or other bad side effects like that |
|
Sure, but the fix is flawed, or at least it regresses GPU compilation: JuliaGPU/GPUCompiler.jl#506. So unless you want to trade one issue for another, I'd rather we not break the GPU stack between 1.10-rc1 and the release. |
KristofferC
added a commit
that referenced
this pull request
Dec 2, 2023
Backported PRs: - [x] #51213 <!-- Wait for other threads to finish compiling before exiting --> - [x] #51520 <!-- Make allocopt respect the GC verifier rules with non usual address spaces --> - [x] #51598 <!-- Use a simple error when reporting sysimg load failures. --> - [x] #51757 <!-- fix parallel peakflop usage --> - [x] #51781 <!-- Don't make pkgimages global editable --> - [x] #51848 <!-- allow finalizers to take any locks and yield during exit --> - [x] #51847 <!-- add missing wait during Timer and AsyncCondition close --> - [x] #50824 <!-- Add some aliasing warnings to docstrings for mutating functions in Base --> - [x] #51885 <!-- remove chmodding the pkgimages --> - [x] #50207 <!-- [devdocs] Improve documentation about building external forks of LLVM --> - [x] #51967 <!-- further fix to the new promoting method for AbstractDateTime subtraction --> - [x] #51980 <!-- macroexpand: handle const/atomic struct fields correctly --> - [x] #51995 <!-- [Artifacts] Pass artifacts dictionary to `ensure_artifact_installed` dispatch --> - [x] #52098 <!-- Fix errors in `sort` docstring --> - [x] #52136 <!-- Bump JuliaSyntax to 0.4.7 --> - [x] #52140 <!-- Make c func `abspath` consistent on Windows. Fix tracking path conversion. --> - [x] #52009 <!-- fix completion that resulted in startpos of 0 for `\\ --> - [x] #52192 <!-- cap the number of GC threads to number of cpu cores --> - [x] #52206 <!-- Make have_fma consistent between interpreter and compiled --> - [x] #52027 <!-- fix Unicode.julia_chartransform for Julia 1.10 --> - [x] #52217 <!-- More helpful error message for empty `cpu_target` in `Base.julia_cmd` --> - [x] #51371 <!-- Memoize `cwstring` when used for env lookup / modification on Windows --> - [x] #52214 <!-- Turn Method Overwritten Error into a PrecompileError -- turning off caching --> - [x] #51895 <!-- Devdocs on fixing precompile hangs, take 2 --> - [x] #51596 <!-- Reland "Don't mark nonlocal symbols as hidden"" --> - [x] #51834 <!-- [REPLCompletions] allow symbol completions within incomplete macrocall expression --> - [x] #52010 <!-- Revert "Support sorting iterators (#46104)" --> - [x] #51430 <!-- add support for async backtraces of Tasks on any thread --> - [x] #51471 <!-- Fix segfault if root task is NULL --> - [x] #52194 <!-- Fix multiversioning issues caused by the parallel llvm work --> - [x] #51035 <!-- refactor GC scanning code to reflect jl_binding_t are now first class --> - [x] #52030 <!-- Bump Statistics --> - [x] #52189 <!-- codegen: ensure i1 bool is widened to i8 before storing --> - [x] #52228 <!-- Widen diagonal var during `Type` unwrapping in `instanceof_tfunc` --> - [x] #52182 <!-- jitlayers: replace sharedbytes intern pool with one that respects alignment --> Contains multiple commits, manual intervention needed: - [ ] #51092 <!-- inference: fix bad effects for recursion --> Non-merged PRs with backport label: - [ ] #52196 <!-- Fix creating custom log level macros --> - [ ] #52170 <!-- fix invalidations related to `ismutable` --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
KristofferC
removed
the
backport 1.10
This was referenced Jan 16, 2024
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.
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.
Effects are not converged, so they need to always be correct, and not a bestguess, even during recursion. This could be refined, but we don't really need to, and it might be unnecessarily costly to do so.