Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow external lattice elements to properly union split #49030

Merged
merged 2 commits into from Mar 18, 2023

Conversation

Keno
Copy link
Member

@Keno Keno commented Mar 16, 2023

Currently MustAlias is the only lattice element that is allowed to widen to union types. However, there are others in external packages. Expand the support we have for this in order to allow union splitting of lattice elements.

Copy link
Sponsor Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

@nanosoldier runbenchmarks("inference", vs=":master")

@@ -231,8 +231,7 @@ function unionsplitcost(argtypes::Union{SimpleVector,Vector{Any}})
nu = 1
max = 2
for ti in argtypes
# TODO remove this to implement callsite refinement of MustAlias
if isa(ti, MustAlias) && isa(widenconst(ti), Union)
if !isvarargtype(ti)
ti = widenconst(ti)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

This widenconst would be wasteful in the native compilation pipeline since we don't enable MustAlias by default. Maybe we want to add a hook has_extended_unionsplit(𝕃::AbstractLattice) to configure this widening?

Copy link
Member Author

Choose a reason for hiding this comment

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

sure, seems reasonable

Currently `MustAlias` is the only lattice element that is allowed
to widen to union types. However, there are others in external
packages. Expand the support we have for this in order to allow
union splitting of lattice elements.
base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved
base/compiler/tfuncs.jl Outdated Show resolved Hide resolved
base/compiler/abstractinterpretation.jl Outdated Show resolved Hide resolved
Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
@Keno
Copy link
Member Author

Keno commented Mar 17, 2023

@nanosoldier runbenchmarks("inference")

@nanosoldier
Copy link
Collaborator

Your benchmark job has completed - successfully executed benchmarks. A full report can be found here.

@Keno
Copy link
Member Author

Keno commented Mar 17, 2023

@nanosoldier runbenchmarks("inference", vs=":master")

@nanosoldier
Copy link
Collaborator

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

@aviatesk aviatesk merged commit 0a9abc1 into master Mar 18, 2023
5 of 7 checks passed
@aviatesk aviatesk deleted the kf/extlatticeunionsplit branch March 18, 2023 04:56
aviatesk added a commit to aviatesk/JET.jl that referenced this pull request Mar 18, 2023
oscardssmith pushed a commit to oscardssmith/julia that referenced this pull request Mar 20, 2023
)

Currently `MustAlias` is the only lattice element that is allowed
to widen to union types. However, there are others in external
packages. Expand the support we have for this in order to allow
union splitting of lattice elements.

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
Xnartharax pushed a commit to Xnartharax/julia that referenced this pull request Apr 19, 2023
)

Currently `MustAlias` is the only lattice element that is allowed
to widen to union types. However, there are others in external
packages. Expand the support we have for this in order to allow
union splitting of lattice elements.

Co-authored-by: Shuhei Kadowaki <40514306+aviatesk@users.noreply.github.com>
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.

None yet

3 participants