Skip to content

Commit

Permalink
Fix a few more cases of missing lattice arguments (#46645)
Browse files Browse the repository at this point in the history 
These were missed in the original lattice PR.
  • Loading branch information
@Keno
Keno authored Sep 6, 2022
1 parent d6cbfab commit fa3981b
Show file tree
Hide file tree
Showing 3 changed files with 12 additions and 12 deletions.
2 changes: 1 addition & 1 deletion base/compiler/typelattice.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -362,7 +362,7 @@ function is_lattice_equal(lattice::PartialsLattice, @nospecialize(a), @nospecial
widenconst(a) == widenconst(b) || return false
a.fields === b.fields && return true # fast path
for i in 1:length(a.fields)
is_lattice_equal(a.fields[i], b.fields[i]) || return false
is_lattice_equal(lattice, a.fields[i], b.fields[i]) || return false
end
return true
end
Expand Down
20 changes: 10 additions & 10 deletions base/compiler/typelimits.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -304,7 +304,7 @@ end

# A simplified type_more_complex query over the extended lattice
# (assumes typeb ⊑ typea)
function issimplertype(@nospecialize(typea), @nospecialize(typeb))
function issimplertype(lattice::AbstractLattice, @nospecialize(typea), @nospecialize(typeb))
typea = ignorelimited(typea)
typeb = ignorelimited(typeb)
typea isa MaybeUndef && (typea = typea.typ) # n.b. does not appear in inference
Expand All @@ -315,14 +315,14 @@ function issimplertype(@nospecialize(typea), @nospecialize(typeb))
for i = 1:length(typea.fields)
ai = unwrapva(typea.fields[i])
bi = fieldtype(aty, i)
is_lattice_equal(ai, bi) && continue
is_lattice_equal(lattice, ai, bi) && continue
tni = _typename(widenconst(ai))
if tni isa Const
bi = (tni.val::Core.TypeName).wrapper
is_lattice_equal(ai, bi) && continue
is_lattice_equal(lattice, ai, bi) && continue
end
bi = getfield_tfunc(typeb, Const(i))
is_lattice_equal(ai, bi) && continue
is_lattice_equal(lattice, ai, bi) && continue
# It is not enough for ai to be simpler than bi: it must exactly equal
# (for this, an invariant struct field, by contrast to
# type_more_complex above which handles covariant tuples).
Expand All @@ -335,14 +335,14 @@ function issimplertype(@nospecialize(typea), @nospecialize(typeb))
typeb isa Const && return true
typeb isa Conditional || return false
is_same_conditionals(typea, typeb) || return false
issimplertype(typea.thentype, typeb.thentype) || return false
issimplertype(typea.elsetype, typeb.elsetype) || return false
issimplertype(lattice, typea.thentype, typeb.thentype) || return false
issimplertype(lattice, typea.elsetype, typeb.elsetype) || return false
elseif typea isa InterConditional # ibid
typeb isa Const && return true
typeb isa InterConditional || return false
is_same_conditionals(typea, typeb) || return false
issimplertype(typea.thentype, typeb.thentype) || return false
issimplertype(typea.elsetype, typeb.elsetype) || return false
issimplertype(lattice, typea.thentype, typeb.thentype) || return false
issimplertype(lattice, typea.elsetype, typeb.elsetype) || return false
elseif typea isa PartialOpaque
# TODO
end
Expand All @@ -356,10 +356,10 @@ end
typea === typeb && return typea

suba = ⊑(lattice, typea, typeb)
suba && issimplertype(typeb, typea) && return typeb
suba && issimplertype(lattice, typeb, typea) && return typeb
subb = ⊑(lattice, typeb, typea)
suba && subb && return typea
subb && issimplertype(typea, typeb) && return typea
subb && issimplertype(lattice, typea, typeb) && return typea
return nothing
end

Expand Down
2 changes: 1 addition & 1 deletion test/compiler/inference.jl
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4097,7 +4097,7 @@ end == Rational
# vararg-tuple comparison within `PartialStruct`
# https://github.com/JuliaLang/julia/issues/44965
let t = Core.Compiler.tuple_tfunc(Any[Core.Const(42), Vararg{Any}])
@test Core.Compiler.issimplertype(t, t)
@test Core.Compiler.issimplertype(Core.Compiler.fallback_lattice, t, t)
end

# check the inference convergence with an empty vartable:
Expand Down

8 comments on commit fa3981b

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Executing the daily package evaluation, I will reply here when finished:

@nanosoldier runtests(ALL, isdaily = true)

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on fa3981b Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanosoldier runbenchmarks(ALL, isdaily = true)

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your package evaluation job has completed - possible new issues were detected. A full report can be found here.

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on fa3981b Sep 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like a few large regressions and no significant improvements this week. Probably need to rerun to filter out some of the noisy ones, then bisect to figure out the source of the new regressions.

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on fa3981b Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nanosoldier runbenchmarks("array"||"broadcast"||"collection"||"find"||"inference"||"misc"||"sparse"||"string"||"tuple", vs="@5c5af1fffd1bd0a9124415689a4664ab934e79f1")

@nanosoldier
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Your benchmark job has completed - possible performance regressions were detected. A full report can be found here.

@vtjnash
Copy link
Member

@vtjnash vtjnash commented on fa3981b Sep 9, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems to narrow it down a lot to being just one real regression and lots of noise, though still no significant improvements detected.

["string", "==(::AbstractString, ::AbstractString)", "different length"]	1.45 (5%) ❌	1.00 (1%)
["string", "==(::AbstractString, ::AbstractString)", "different"]	400.98 (5%) ❌	1.00 (1%)
["string", "==(::AbstractString, ::AbstractString)", "equal"]	1.45 (5%) ❌	1.00 (1%)

Please sign in to comment.