Widen diagonal var during Type unwrapping in instanceof_tfunc
#52228
Conversation
N5N3
added
compiler:inference
N5N3
force-pushed
the
instanceof_fix
branch
7 times, most recently
from
49672a4
to
1fc9d54
Compare
|
@nanosoldier |
| } | ||
| if (!any_concrete) | ||
| return t; // no diagonal | ||
| return insert_nondiagonal(t, troot, 0); |
There was a problem hiding this comment.
Since you already checked occurs_inv, it may be already fine to do wident2ub always?
| return insert_nondiagonal(t, troot, 0); | |
| return insert_nondiagonal(t, troot, 1); |
In effect though, that may be the same as implementing it as a map of rewrap_unionall over the fields (but recursively) roughly doing map(rewrap_unionall, t.parameters, Repeated(u))?
I think the tricky cases I was thinking of were possibly something like:
Tuple{Val{S}, T, T} where {S, T<:S} or perhaps
Tuple{S, T, T} where {S, T<:S} or perhaps
Tuple{S, Vararg{T}} where {S, T<:S}
These could be safely widened to Tuple{Val, Any, Any}, Tuple{Any, Any, Any}, and Tuple{Any, Vararg{Any}} respectively, but I was hoping to instead convert them to the more precise results of Tuple{Val{S}, T1, T2} where {S, T1<:S, T2<:S}, Tuple{S, T1, T2} where {S, T1<:S, T2<:S}, and Tuple{S, Vararg{Union{T1,T2} where T1<:S where T2<:S}} where S
(though normally allocation normalization will have incorrectly broken that last type anyways, since it quickly converts that to Tuple{S, Vararg{T where T<:S}} where S which it thinks is equal to Tuple{S, Vararg{S}} where S even though it is not necessarily correct to replace T.ub with T in the expression T where T there–as that also implies a new constraint on the Vararg: that it is a diagonal Vararg when it was not intended to be)
There was a problem hiding this comment.
It seems ok that
julia> CC.instanceof_tfunc(Type{Tuple{S, Vararg{T}}} where {S, T<:S})[1]
Tuple{S, Vararg{S}} where SAs S should be always concrete at runtime and Tuple{S, Vararg{S}} where S should cover all possible types.
Edit: Well we are doing inference, so that might be bad.
The current code fails for this case though
julia> CC.instanceof_tfunc(Type{Tuple{S, S, Vararg{T}}} where {S, T<:S})[1]
Tuple{S2, S1, Vararg{S}} where {S, S1<:S, S2<:S}It shows that the widen2ub branch should be recursive, and return Tuple{S2, S1, Vararg{Any}} where {S, S1<:S, S2<:S} instead.
As for the first two examples, the current code with widen2ub = 0 should be precise, though with worse normalization.
|
The package evaluation job you requested has completed - possible new issues were detected. |
N5N3
force-pushed
the
instanceof_fix
branch
3 times, most recently
from
3a1a626
to
95113d8
Compare
src/subtype.c
Outdated
| newa = insert_nondiagonal(a, troot, widen2ub); | ||
| newb = insert_nondiagonal(b, troot, widen2ub); | ||
| if (newa != a || newb != b) | ||
| type = (jl_value_t *)jl_new_struct(jl_uniontype_type, newa, newb); |
There was a problem hiding this comment.
Should this use the jl_union_type normalization? I think newa and newb might have common elements after this. E.g. Union{Tuple{S, T, T}, Tuple{T, T, S}} where {S, T}
There was a problem hiding this comment.
I think newa and newb would have no common elements if there's no widened Vararg, as we always insert a unique free typevar.
But for Vararg, there might be common elements as different TypeVar might have the same ub.
We can fuse simple_union here.
There was a problem hiding this comment.
Right. simple_union was the one that I was actually thinking of
Otherwise `type->ub` might still contains TypeVar that should be nondiagonal.
add add check before innervar wrapping.
vtjnash
added
status:merge me
|
Will this fix get backported for v1.10? Just curious. |
N5N3
added
backport 1.10
|
Oh, didn't notice the Compiler side change. Will take a look tonight. @KristofferC |
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 -->
Backported PRs: - [x] #51095 <!-- Fix edge cases where inexact conversions to UInt don't throw --> - [x] #52583 <!-- Don't access parent of triangular matrix in powm --> - [x] #52645 <!-- update --gcthreads section in command line options --> - [x] #52423 <!-- update nthreads info in versioninfo --> - [x] #52721 <!-- inference: Guard TypeVar special case against vararg --> - [x] #52637 <!-- fix finding bundled stdlibs even if they are e.g. devved in an environment higher in the load path --> - [x] #52752 <!-- staticdata: handle cycles in datatypes --> - [x] #52758 <!-- use a Dict instead of an IdDict for caching of the `cwstring` for Windows env variables --> - [x] #51375 <!-- Insert hardcoded backlinks to stdlib doc pages --> - [x] #52994 <!-- place work-stealing queue indices on different cache lines to avoid false-sharing --> - [x] #53015 <!-- Add type assertion in iterate for logicalindex --> - [x] #53032 <!-- Fix a list in GC devdocs --> - [x] #52748 - [x] #52856 - [x] #52878 - [x] #52754 - [x] #52228 - [x] #52924 - [x] #52569 <!-- Fix GC rooting during rehashing of iddict --> - [x] #52605 <!-- Default uplo in symmetric/hermitian --> - [x] #52618 <!-- heap snapshot: add gc roots and gc finalist roots to fix unrooted nodes --> - [x] #52781 <!-- fix type-stability bugs in Ryu code --> - [x] #53055 <!-- Profile: use full terminal cols to show function name --> - [x] #53096 - [x] #53076 - [x] #52841 <!-- Extensions: make loading of extensions independent of what packages are in the sysimage --> - [x] #52078 <!-- Replace `⇔` by `↔` in documentation --> - [x] #53035 <!-- use proper cache-line size variable in work-stealing queue --> - [x] #53066 <!-- doc: replace harr HTML entity by unicode --> - [x] #52996 <!-- Apple silicon has 128 byte alignment so fix our defines to match --> - [x] #53121 Non-merged PRs with backport label: - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
close #52168
close #27031