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

inlining: use method match signature for union-spliting #53600

Merged
merged 1 commit into from
Mar 7, 2024
Merged

Conversation

aviatesk
Copy link
Sponsor Member

@aviatesk aviatesk commented Mar 5, 2024

In cases where the results of constant inference, like concrete-eval, are used for union-split inlining, isa-blocks are generated using the result.edge.specTypes stored within each result. However, it's been found that the edge returned by abstract interpretation may have been widened by the new @nospecializeinfer, which can result in invalid union-splitting. To address this problem, this commit tweaks the inlining algorithm so that it performs union-split inlining using the original signatures that abstract interpretation used for union-split inference, by using match::MethodMatch.

In cases where the results of constant inference, like concrete-eval,
are used for union-split inlining, `isa`-blocks are generated using the
`result.edge.specTypes` stored within each `result`. However, it's been
found that the `edge` returned by abstract interpretation may have been
widened by the new `@nospecializeinfer`, which can result in invalid
union-splitting. To address this problem, this commit tweaks the
inlining algorithm so that it performs union-split inlining using the
original signatures that abstract interpretation used for union-split
inference, by using `match::MethodMatch`.

- fixes #53590
@aviatesk aviatesk added the backport 1.11 Change should be backported to release-1.11 label Mar 5, 2024
Copy link
Sponsor Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

LGTM. I am a bit concerned about what this means about the indiscriminate use of typeintersection results (spec_types) though. Are we being sufficiently careful to only use this result when we can prove that spec_types <: match.method.sig && spec_types <: argument to ml_matches (e.g. the exactness condition)? I would also guess that checking match.fully_covers is suitable in some cases as an alternative check, since it implies that argument <: match.method.sig && spec_types == argument, which is sufficient although not necessary.

@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 5, 2024

Also that once the exactness condition fails on one MethodMatch in the list and so we cannot generate an exact dispatch to it, then we can only accept future method match optimizations from the list that are disjoint from the one that failed (such as concrete methods where allow_abstract=false).

@KristofferC KristofferC mentioned this pull request Mar 6, 2024
60 tasks
@aviatesk
Copy link
Sponsor Member Author

aviatesk commented Mar 7, 2024

If it's not fully_covered, we don't inline dispatches that aren't concrete (

elseif !handled_all_cases
# if we've not seen all candidates, union split is valid only for dispatch tuples
filter!(case::InliningCase->isdispatchtuple(case.sig), cases)
) and keep the fallback dynamic dispatch (
# We're now in the fall through block, decide what to do
if !fully_covered
ssa = insert_node_here!(compact, NewInstruction(stmt, typ, line))
push!(pn.edges, bb)
push!(pn.values, ssa)
insert_node_here!(compact, NewInstruction(GotoNode(join_bb), Union{}, line))
finish_current_bb!(compact, 0)
end
), which should take care of the worries. There is a situation where we might inline an abstract dispatch even if it isn't fully_covered, but that's only if there's just one single dispatch candidate.

@aviatesk aviatesk merged commit d45581c into master Mar 7, 2024
6 of 8 checks passed
@aviatesk aviatesk deleted the avi/53590 branch March 7, 2024 16:15
aviatesk added a commit that referenced this pull request Mar 7, 2024
In cases where the results of constant inference, like concrete-eval,
are used for union-split inlining, `isa`-blocks are generated using the
`result.edge.specTypes` stored within each `result`. However, it's been
found that the `edge` returned by abstract interpretation may have been
widened by the new `@nospecializeinfer`, which can result in invalid
union-splitting. To address this problem, this commit tweaks the
inlining algorithm so that it performs union-split inlining using the
original signatures that abstract interpretation used for union-split
inference, by using `match::MethodMatch`.

- fixes #53590
aviatesk added a commit that referenced this pull request Mar 7, 2024
In cases where the results of constant inference, like concrete-eval,
are used for union-split inlining, `isa`-blocks are generated using the
`result.edge.specTypes` stored within each `result`. However, it's been
found that the `edge` returned by abstract interpretation may have been
widened by the new `@nospecializeinfer`, which can result in invalid
union-splitting. To address this problem, this commit tweaks the
inlining algorithm so that it performs union-split inlining using the
original signatures that abstract interpretation used for union-split
inference, by using `match::MethodMatch`.

- fixes #53590
@vtjnash
Copy link
Sponsor Member

vtjnash commented Mar 7, 2024

That is not relevant to the distinction I am making. Using spec_types derived from the MethodMatch is incorrect and insufficient to determine whether it is valid to inline a match. Only if match.spec_types <: match.method.sig is that legal, but even then you must be careful that if one case fails that validity check, then all of the ones after that must do more conservative matching. For example, say we decided for whatever reason to ignore f(::Int), then we cannot later choose to handle f(::Any). This can get even trickier if we did union-splitting, since we only sort the lists pre-splitting, so some thing in an earlier union-split list might be required to appear after something in a later list. For example, given the methods:
f(::Int64) = 1; f(::Any) = 2 and the call f(Union{Int32, Int64}), we know the result of union splitting will return the f(::Any) method first (for the Int32 case), then the f(::Int64) method. However it is only legal to use the Int32 if we can also show that the spec_types is safely also a subtype of the argument to ml_matches--otherwise we don't know if we are going to have widened that to f(::Any) (which is still a subtype of the sig, and therefore an expected legal result to have there), and then end up using that method instead of f(::Int64) for an Int64 call. Additionally, we also need to be extra cautious furthermore, since we know the cases may not be entirely disjoint (not just for the supertype Any): for example, the union elements might be Vector{<:Union{Int32,Nothing}} and Vector{<:Union{Int64,Nothing}}.

aviatesk added a commit that referenced this pull request Mar 7, 2024
In cases where the results of constant inference, like concrete-eval,
are used for union-split inlining, `isa`-blocks are generated using the
`result.edge.specTypes` stored within each `result`. However, it's been
found that the `edge` returned by abstract interpretation may have been
widened by the new `@nospecializeinfer`, which can result in invalid
union-splitting. To address this problem, this commit tweaks the
inlining algorithm so that it performs union-split inlining using the
original signatures that abstract interpretation used for union-split
inference, by using `match::MethodMatch`.

- fixes #53590
aviatesk added a commit that referenced this pull request Mar 13, 2024
As Jameson pointed out in the link below, while the union-split handles
cases when there are uncovered matches, sometimes the expected condition
`spec_types <: method.sig` that the union-split algorithm relies on
isn't met in such cases, which caused issues like #52644. This commit
fixes the problem by adding explicit checks for such cases. Note that
this is based on #52092. The extra handling for single method match
unmatched static parameters based on `only_method` that was removed in
JuliaLang/#52092 has been ineffective and would otherwise cause
problematic inlining on this PR. We'll likely need to come back to this
later and figure out a safe and effective way to deal with such cases
in the future when the handling for either case turns out to be
necessary.

- closes #52644
- xref: <#53600 (review)>
aviatesk added a commit that referenced this pull request Mar 14, 2024
As Jameson pointed out in the link below, while the union-split handles
cases when there are uncovered matches, sometimes the expected condition
`spec_types <: method.sig` that the union-split algorithm relies on
isn't met in such cases, which caused issues like #52644. This commit
fixes the problem by adding explicit checks for such cases. Note that
this is based on #52092. The extra handling for single method match
unmatched static parameters based on `only_method` that was removed in
JuliaLang/#52092 has been ineffective and would otherwise cause
problematic inlining on this PR. We'll likely need to come back to this
later and figure out a safe and effective way to deal with such cases
in the future when the handling for either case turns out to be
necessary.

- closes #52644
- xref: <#53600 (review)>
aviatesk added a commit that referenced this pull request Mar 15, 2024
)

As Jameson pointed out in the link below, while the union-split handles
cases when there are uncovered matches, sometimes the expected condition
`spec_types <: method.sig` that the union-split algorithm relies on
isn't met in such cases, which caused issues like #52644. This commit
fixes the problem by adding explicit checks for such cases. Note that
this is based on #52092. The extra handling for single method match
unmatched static parameters based on `only_method` that was removed in
JuliaLang/#52092 has been ineffective and would otherwise cause
problematic inlining on this PR. We'll likely need to come back to this
later and figure out a safe and effective way to deal with such cases in
the future when the handling for either case turns out to be necessary.

- closes #52644
- xref:
<#53600 (review)>
aviatesk added a commit that referenced this pull request Mar 15, 2024
)

As Jameson pointed out in the link below, while the union-split handles
cases when there are uncovered matches, sometimes the expected condition
`spec_types <: method.sig` that the union-split algorithm relies on
isn't met in such cases, which caused issues like #52644. This commit
fixes the problem by adding explicit checks for such cases. Note that
this is based on #52092. The extra handling for single method match
unmatched static parameters based on `only_method` that was removed in
JuliaLang/#52092 has been ineffective and would otherwise cause
problematic inlining on this PR. We'll likely need to come back to this
later and figure out a safe and effective way to deal with such cases in
the future when the handling for either case turns out to be necessary.

- closes #52644
- xref:
<#53600 (review)>
KristofferC pushed a commit that referenced this pull request Mar 18, 2024
)

As Jameson pointed out in the link below, while the union-split handles
cases when there are uncovered matches, sometimes the expected condition
`spec_types <: method.sig` that the union-split algorithm relies on
isn't met in such cases, which caused issues like #52644. This commit
fixes the problem by adding explicit checks for such cases. Note that
this is based on #52092. The extra handling for single method match
unmatched static parameters based on `only_method` that was removed in
JuliaLang/#52092 has been ineffective and would otherwise cause
problematic inlining on this PR. We'll likely need to come back to this
later and figure out a safe and effective way to deal with such cases in
the future when the handling for either case turns out to be necessary.

- closes #52644
- xref:
<#53600 (review)>

(cherry picked from commit b730d33)
@KristofferC KristofferC mentioned this pull request Mar 20, 2024
41 tasks
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 27, 2024
mkitti pushed a commit to mkitti/julia that referenced this pull request Apr 13, 2024
)

In cases where the results of constant inference, like concrete-eval,
are used for union-split inlining, `isa`-blocks are generated using the
`result.edge.specTypes` stored within each `result`. However, it's been
found that the `edge` returned by abstract interpretation may have been
widened by the new `@nospecializeinfer`, which can result in invalid
union-splitting. To address this problem, this commit tweaks the
inlining algorithm so that it performs union-split inlining using the
original signatures that abstract interpretation used for union-split
inference, by using `match::MethodMatch`.

- fixes JuliaLang#53590
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.

Incorrect optimization on typejoin in 1.11+
3 participants