Fix a precision issue in `abstract_iteration` #41839

martinholters · 2021-08-09T13:14:53Z

The first loop is left before valtype and statetype are updated from the current stateordonet, so the second loop should first process it before obtaining a new one instead of vice versa.

Fixes #41022.

martinholters · 2021-08-09T15:14:42Z

Ah, no, this isn't it. We need to compute a new stateordonet based on the const-widened statetype first. We just need to be careful in case the first loop never sets a statetype to not use Union{} as the argument type then.

base/compiler/abstractinterpretation.jl

martinholters · 2021-08-12T11:20:22Z

Bump. @Keno, mind taking another look?

martinholters · 2021-08-17T08:03:31Z

Bump. Any objections against merging this?

base/compiler/abstractinterpretation.jl

vtjnash · 2021-08-17T16:27:51Z

Thanks for solving this. It looks like it is actually a bugfix so we should backport it, but that only the iterate function needs to be moved (the other changes just make it slower / less accurate)

martinholters · 2021-08-18T06:03:02Z

I don't think this is a bugfix in the sense that inference was unsound before. At least I could not come up with a case, although that may not be too strong an indicator... On the other hand, the fact that you proposed an alternative which did end up being unsound is a strong indicator that this is quite tricky to get right, so I'm not sure backporting is a good idea.

martinholters · 2021-08-18T06:18:56Z

but that only the iterate function needs to be moved (the other changes just make it slower / less accurate)

If you mean that we should not do the removal of statetype = widenconst(statetype) and use of ⊑ instead of <:, then no, these changes are needed for soundness (see explanation in response to your inline comment above).

base/compiler/abstractinterpretation.jl

martinholters · 2021-08-19T08:22:15Z

Prodding at this, I've found another case where we could consider improving precision:

julia> struct NoneOrFailOnSecond{T}
           itr::T
       end

julia> Base.iterate(itr::NoneOrFailOnSecond) = iterate(itr.itr)

julia> f_splat(x) = (x...,)
f_splat (generic function with 1 method)

julia> f_splat(NoneOrFailOnSecond([]))
()

julia> f_splat(NoneOrFailOnSecond([1]))
ERROR: MethodError: no method matching iterate(::NoneOrFailOnSecond{Vector{Int64}}, ::Int64)
[...]

julia> @code_typed f_splat(NoneOrFailOnSecond([1]))
CodeInfo(
1 ─ %1 = Core._apply_iterate(Base.iterate, Core.tuple, x)::Tuple
└──      return %1
) => Tuple

(Same for master and this PR in its current form.) Conceivably, this could be inferred as Tuple{}. It should also be possible to construct a case where it would be possible to infer that either a fixed (non-zero) number of elements is produced or the iteration fails. We would need to check nounion == Bottom and then act accordingly. But I doubt it's worth it. Such iterators look rather pathological and defensively producing too wide a result seems better than getting something wrong and returning too narrow a result, even if only for bizarre edge cases.

So I'd say leave that as is (for now, at least). Opinions?

vtjnash · 2021-08-19T16:11:59Z

I think we can easily handle that, by choosing not to widen valtype, and avoiding adding Vararg{Bottom} to the array

Another case we can improve is to model the implicit typeassert call, by adding a tmeet(nounion, Tuple{Any, Any}) call before the checks for whether we have an Tuple of 2 fields

test/compiler/inference.jl

base/compiler/abstractinterpretation.jl

martinholters · 2021-08-20T10:08:46Z

base/compiler/abstractinterpretation.jl

            valtype = Any
            break
        end
        if nounion.parameters[1] <: valtype && nounion.parameters[2] <: statetype
+            # reached a fixpoint or iterator failed/gave invalid answer
            if typeintersect(stateordonet, Nothing) === Union{}


Suggested change

if typeintersect(stateordonet, Nothing) === Union{}

if !(Nothing <: stateordonet)

As Nothing is a leaf type, this should be equal but more efficient?

It should be almost identical (since typeintersect can check that property too), so not worth worrying about the difference too much

martinholters · 2021-08-20T10:11:33Z

base/compiler/abstractinterpretation.jl

+        if nounion === Union{}
+            # handle iterator failing or giving an invalid answer below
+            nounion = Tuple{Union{}, Union{}}
+        elseif !isa(nounion, DataType) || !(nounion <: Tuple) || isvatuple(nounion) || length(nounion.parameters) != 2


Suggested change

elseif !isa(nounion, DataType) || !(nounion <: Tuple) || isvatuple(nounion) || length(nounion.parameters) != 2

elseif !isa(nounion, DataType)

Should be sufficient with the typeintersect above, but in corner cases it might give funnily too wide types which then might make these extra checks necessary?

base/compiler/abstractinterpretation.jl

If the first loop exits in the first iteration, the `statetype` is still `Bottom`. In that case, the new `stateordonet` needs to be determined with the two-arg version of `iterate` again. Fixes #41022.

Explicitly test that inference produces a sound (and reasonably precise) result when splatting an iterator (in this case a long range) that allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit.

Avoid recomputation of a `widenconst` and initialize `valtype` and `statetype` to `Bottom` before second loop to simplify a `⊑`to a `<:` there.

martinholters · 2021-08-30T05:27:29Z

@vtjnash anything left I should address or good to go?

If the first loop exits in the first iteration, the `statetype` is still `Bottom`. In that case, the new `stateordonet` needs to be determined with the two-arg version of `iterate` again. Explicitly test that inference produces a sound (and reasonably precise) result when splatting an iterator (in this case a long range) that allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit. Fixes #41022 Co-authored-by: Jameson Nash <vtjnash@gmail.com> (cherry picked from commit 92337b5)

If the first loop exits in the first iteration, the `statetype` is still `Bottom`. In that case, the new `stateordonet` needs to be determined with the two-arg version of `iterate` again. Explicitly test that inference produces a sound (and reasonably precise) result when splatting an iterator (in this case a long range) that allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit. Fixes JuliaLang#41022 Co-authored-by: Jameson Nash <vtjnash@gmail.com>

If the first loop exits in the first iteration, the `statetype` is still `Bottom`. In that case, the new `stateordonet` needs to be determined with the two-arg version of `iterate` again. Explicitly test that inference produces a sound (and reasonably precise) result when splatting an iterator (in this case a long range) that allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit. Fixes #41022 Co-authored-by: Jameson Nash <vtjnash@gmail.com> (cherry picked from commit 92337b5)

martinholters added the compiler:inference Type inference label Aug 9, 2021

martinholters force-pushed the mh/fix_abstract_iteration branch from 19d2f4c to 228e0fb Compare August 9, 2021 15:45

Keno reviewed Aug 9, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

vtjnash added backport 1.7 kind:bugfix This change fixes an existing bug labels Aug 17, 2021

vtjnash reviewed Aug 17, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Show resolved Hide resolved

vtjnash added the backport 1.6 Change should be backported to release-1.6 label Aug 17, 2021

martinholters force-pushed the mh/fix_abstract_iteration branch from defd0a7 to e60bcee Compare August 18, 2021 06:35

vtjnash reviewed Aug 18, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

martinholters force-pushed the mh/fix_abstract_iteration branch from e60bcee to 2666188 Compare August 19, 2021 07:25

martinholters commented Aug 20, 2021

View reviewed changes

test/compiler/inference.jl Show resolved Hide resolved

martinholters commented Aug 20, 2021

View reviewed changes

vtjnash reviewed Aug 20, 2021

View reviewed changes

base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved

martinholters added 5 commits August 24, 2021 07:54

Fix a precision issue in abstract_iteration

1d553b8

If the first loop exits in the first iteration, the `statetype` is still `Bottom`. In that case, the new `stateordonet` needs to be determined with the two-arg version of `iterate` again. Fixes #41022.

Slightly simplify abstract_iteration

423dc65

Add inference soundness test for splatting long range

bbfeb79

Explicitly test that inference produces a sound (and reasonably precise) result when splatting an iterator (in this case a long range) that allows constant-propagation up to the `MAX_TUPLE_SPLAT` limit.

Optimize abstract_iteration a little bit

75372b2

Avoid recomputation of a `widenconst` and initialize `valtype` and `statetype` to `Bottom` before second loop to simplify a `⊑`to a `<:` there.

Improve abstract_iteration for failing and infinte iterators

8669893

martinholters force-pushed the mh/fix_abstract_iteration branch from b200a62 to 8669893 Compare August 24, 2021 07:48

This was referenced Aug 25, 2021

release-1.7: Backports for 1.7-RC1 #41781

Merged

Backports for julia-1.6.3 #41554

Merged

vtjnash merged commit 92337b5 into master Aug 30, 2021

vtjnash deleted the mh/fix_abstract_iteration branch August 30, 2021 20:44

KristofferC removed backport 1.6 Change should be backported to release-1.6 backport 1.7 labels Sep 7, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix a precision issue in `abstract_iteration` #41839

Fix a precision issue in `abstract_iteration` #41839

martinholters commented Aug 9, 2021

martinholters commented Aug 9, 2021

martinholters commented Aug 12, 2021

martinholters commented Aug 17, 2021

vtjnash commented Aug 17, 2021 •

edited

Loading

martinholters commented Aug 18, 2021

martinholters commented Aug 18, 2021

martinholters commented Aug 19, 2021

vtjnash commented Aug 19, 2021

martinholters Aug 20, 2021

vtjnash Aug 20, 2021 •

edited

Loading

martinholters Aug 20, 2021

martinholters commented Aug 30, 2021

	if typeintersect(stateordonet, Nothing) === Union{}
	if !(Nothing <: stateordonet)

	elseif !isa(nounion, DataType) \|\| !(nounion <: Tuple) \|\| isvatuple(nounion) \|\| length(nounion.parameters) != 2
	elseif !isa(nounion, DataType)

Fix a precision issue in abstract_iteration #41839

Fix a precision issue in abstract_iteration #41839

Conversation

martinholters commented Aug 9, 2021

martinholters commented Aug 9, 2021

martinholters commented Aug 12, 2021

martinholters commented Aug 17, 2021

vtjnash commented Aug 17, 2021 • edited Loading

martinholters commented Aug 18, 2021

martinholters commented Aug 18, 2021

martinholters commented Aug 19, 2021

vtjnash commented Aug 19, 2021

martinholters Aug 20, 2021

Choose a reason for hiding this comment

vtjnash Aug 20, 2021 • edited Loading

Choose a reason for hiding this comment

martinholters Aug 20, 2021

Choose a reason for hiding this comment

martinholters commented Aug 30, 2021

Fix a precision issue in `abstract_iteration` #41839

Fix a precision issue in `abstract_iteration` #41839

vtjnash commented Aug 17, 2021 •

edited

Loading

vtjnash Aug 20, 2021 •

edited

Loading