Skip to content

Conversation

@vtjnash
Copy link
Member

@vtjnash vtjnash commented Sep 3, 2020

While in 1d08d70 we wanted to avoid combining two types, we did not intend to prevent a type from being just a pass-through value.

While in 1d08d70 we wanted to avoid
combining two types, we did not intend to prevent a type from being just
a pass-through value.
@vtjnash vtjnash merged commit a9743d8 into master Sep 3, 2020
@vtjnash vtjnash deleted the jn/34834regression branch September 3, 2020 15:15
@KristofferC KristofferC mentioned this pull request Sep 7, 2020
29 tasks
KristofferC pushed a commit that referenced this pull request Sep 7, 2020
While in 1d08d70 we wanted to avoid
combining two types, we did not intend to prevent a type from being just
a pass-through value.

(cherry picked from commit a9743d8)
@KristofferC
Copy link
Member

Backporting this doesn't pass the added test https://build.julialang.org/#/builders/71/builds/3148/steps/5/logs/stdio. Should we just not backport it then?

@martinholters
Copy link
Member

Should rather try to figure out how to make them pass, as they used to on 1.4, so this fixes a real regression.

@KristofferC
Copy link
Member

Help appreciated :P

@martinholters
Copy link
Member

On 1.5, we're getting a NOT_FOUND there. So using

    (typea === Union{} || typea === NOT_FOUND) && return typeb
    (typeb === Union{} || typeb === NOT_FOUND) && return typea

makes the test pass. But I'm not sure that is valid. I'm also not sure the NOT_FOUND is even supposed to propagate up to here. I have no idea what it is supposed to signify here and where it comes from.

@martinholters
Copy link
Member

Prior to 1d08d70 we had tmerge(NOT_FOUND, T) == tmerge(T, NOT_FOUND) == T for any T (including Union{}). So that would mean the corresponding change to backport would be

typea === NOT_FOUND && return typeb
typeb === NOT_FOUND && return typea
typea === Union{} && return typeb
typeb === Union{} && return typea

On master, we seem to no longer see a NOT_FOUND in tmerge. So alternatively, we could consider backporting whichever change is responsible for that.

@martinholters
Copy link
Member

Aha, alternatively I guess we could backport this part of dcc0696:

--- a/base/compiler/inferencestate.jl
+++ b/base/compiler/inferencestate.jl
@@ -210,7 +210,10 @@ update_valid_age!(edge::InferenceState, sv::InferenceState) = update_valid_age!(
 function record_ssa_assign(ssa_id::Int, @nospecialize(new), frame::InferenceState)
     old = frame.src.ssavaluetypes[ssa_id]
     if old === NOT_FOUND || !(new ⊑ old)
-        frame.src.ssavaluetypes[ssa_id] = tmerge(old, new)
+        # typically, we expect that old ⊑ new (that output information only
+        # gets less precise with worse input information), but to actually
+        # guarantee convergence we need to use tmerge here to ensure that is true
+        frame.src.ssavaluetypes[ssa_id] = old === NOT_FOUND ? new : tmerge(old, new)
         W = frame.ip
         s = frame.stmt_types
         for r in frame.ssavalue_uses[ssa_id]

KristofferC pushed a commit that referenced this pull request Sep 8, 2020
While in 1d08d70 we wanted to avoid
combining two types, we did not intend to prevent a type from being just
a pass-through value.

(cherry picked from commit a9743d8)
@KristofferC
Copy link
Member

Okay, I backported that part. Let's see how the tests do after that.

KristofferC pushed a commit that referenced this pull request Sep 10, 2020
While in 1d08d70 we wanted to avoid
combining two types, we did not intend to prevent a type from being just
a pass-through value.

(cherry picked from commit a9743d8)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants