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: bail out unless match.spec_types <: match.method.sig #53720

Merged
merged 1 commit into from
Mar 15, 2024

Conversation

aviatesk
Copy link
Sponsor Member

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.

@aviatesk aviatesk added the backport 1.11 Change should be backported to release-1.11 label Mar 13, 2024
@aviatesk aviatesk requested a review from vtjnash March 13, 2024 08:41
@aviatesk
Copy link
Sponsor Member Author

@nanosoldier runbenchmarks(!"scalar", vs=":master")

@nanosoldier
Copy link
Collaborator

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

Base automatically changed from avi/rm-dead-unmatched-param-handling to master March 13, 2024 16:23
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)>
end
elseif !(match.spec_types <: match.method.sig) # the requirement for correct union-split
handled_all_cases = false
else
handled_all_cases &= handle_any_const_result!(cases,
result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=false)
Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Suggested change
result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=false)
result, match, argtypes, info, flag, state; allow_abstract=handled_all_cases, allow_typevars=false)

This can only permit later abstract calls if we proved that all previous calls were disjoint from this. One conservative way to express that requirement is to only allow isdispatchtuple here.

Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Isn't this filtering enough for this?

elseif !isempty(cases)
# if we've not seen all candidates, union split is valid only for dispatch tuples
filter!(case::InliningCase->isdispatchtuple(case.sig), cases)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Additionally, if nunion != 1, then the list of matches cannot be assumed to be sorted, so we can only allow the abstract calls that we can prove are disjoint from the other union elements, since we do not attempt to merge-sort them to combine them (the merge sort might fail) but we simply concat the lists.

Suggested change
result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=false)
result, match, argtypes, info, flag, state; allow_abstract=handled_all_cases && nunion == 1, allow_typevars=false)

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Ah, okay, I wasn't expecting we would over-estimate this then filter it later. That late filter is actually a bit more strict than necessary then, since it doesn't consider the ordering. But that does at least seem valid.

@aviatesk aviatesk merged commit b730d33 into master Mar 15, 2024
7 checks passed
@aviatesk aviatesk deleted the avi/52644 branch March 15, 2024 00:39
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 aviatesk mentioned this pull request Mar 15, 2024
60 tasks
KristofferC added a commit that referenced this pull request Mar 17, 2024
Backported PRs:
- [x] #39071 <!-- Add a lazy `logrange` function and `LogRange` type -->
- [x] #51802 <!-- Allow AnnotatedStrings in log messages -->
- [x] #53369 <!-- Orthogonalize re-indexing for FastSubArrays -->
- [x] #48050 <!-- improve `--heap-size-hint` arg handling -->
- [x] #53482 <!-- add IR encoding for EnterNode -->
- [x] #53499 <!-- Avoid compiler warning about redefining jl_globalref_t
-->
- [x] #53507 <!-- update staled `Core.Compiler.Effects` documentation
-->
- [x] #53408 <!-- task splitting: change additive accumulation to
multiplicative -->
- [x] #53523 <!-- add back an alias for `check_top_bit` -->
- [x] #53377 <!-- add _readdirx for returning more object info gathered
during dir scan -->
- [x] #53525 <!-- fix InteractiveUtils call in Base.runtests on failure
-->
- [x] #53540 <!-- use more efficient `_readdirx` for tab completion -->
- [x] #53545 <!-- use `_readdirx` for `walkdir` -->
- [x] #53551 <!-- revert "Add @create_log_macro for making custom styled
logging macros (#52196)" -->
- [x] #53554 <!-- Always return a value in 1-d circshift! of
abstractarray.jl -->
- [x] #53424 <!-- yet more atomics & cache-line fixes on work-stealing
queue -->
- [x] #53571 <!-- Update Documenter to v1.3 for inventory writing -->
- [x] #53403 <!-- Move parallel precompilation to Base -->
- [x] #53589 <!-- add back `unsafe_convert` to pointer for arrays -->
- [x] #53596 <!-- build: remove extra .a file -->
- [x] #53606 <!-- fix error path in `precompilepkgs` -->
- [x] #53004 <!-- Unexport with, at_with, and ScopedValue from Base -->
- [x] #53629 <!-- typo fix in scoped values docs -->
- [x] #53630 <!-- sroa: Fix incorrect scope counting -->
- [x] #53598 <!-- Use Base parallel precompilation to build stdlibs -->
- [x] #53649 <!-- precompilepkgs: package in boths deps and weakdeps are
in fact only weak -->
- [x] #53671 <!-- Fix bootstrap Base precompile in cross compile
configuration -->
- [x] #52125 <!-- Load Pkg if not already to reinstate missing package
add prompt -->
- [x] #53602 <!-- Handle zero on arrays of unions of number types and
missings -->
- [x] #53516 <!-- permit NamedTuple{<:Any, Union{}} to be created -->
- [x] #53643 <!-- Bump CSL to 1.1.1 to fix libgomp bug -->
- [x] #53679 <!-- move precompile workload back from Base -->
- [x] #53663 <!-- add isassigned methods for reinterpretarray -->
- [x] #53662 <!-- [REPL] fix incorrectly cleared line after completions
accepted -->
- [x] #53611 <!-- Linalg: matprod_dest for Diagonal and adjvec -->
- [x] #53659 <!-- fix #52025, re-allow all implicit pointer casts in
cconvert for Array -->
- [x] #53631 <!-- LAPACK: validate input parameters to throw informative
errors -->
- [x] #53628 <!-- Make some improvements to the Scoped Values
documentation. -->
- [x] #53655 <!-- Change tbaa of ptr_phi to tbaa_value  -->
- [x] #53391 <!-- Default to the medium code model in x86 linux -->
- [x] #53699 <!-- Move `isexecutable, isreadable, iswritable` to
`filesystem.jl` -->
- [x] #41232 <!-- Fix linear indexing for ReshapedArray if the parent
has offset axes -->
- [x] #53527 <!-- Enable analyzegc checks for try catch and fix found
issues -->
- [x] #52092 
- [x] #53682 <!-- Increase build precompilation -->
- [x] #53720 
- [x] #53553 <!-- typeintersect: fix `UnionAll` unaliasing bug caused by
innervars. -->

Contains multiple commits, manual intervention needed:
- [ ] #53305 <!-- Propagate inbounds in isassigned with CartesianIndex
indices -->

Non-merged PRs with backport label:
- [ ] #53736 <!-- fix literal-pow to return the right type when the base
is -1 -->
- [ ] #53707 <!-- Make ScopedValue public -->
- [ ] #53696 <!-- add invokelatest to on_done callback in bracketed
paste -->
- [ ] #53660 <!-- put Logging back in default sysimage -->
- [ ] #53509 <!-- revert moving "creating packages" from Pkg.jl -->
- [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} -->
- [ ] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` -->
- [ ] #52694 <!-- Reinstate similar for AbstractQ for backward
compatibility -->
- [ ] #51928 <!-- Styled markdown, with a few tweaks -->
- [ ] #51816 <!-- User-themable stacktraces -->
- [ ] #51811 <!-- Make banner size depend on terminal size -->
- [ ] #51479 <!-- prevent code loading from lookin in the versioned
environment when building Julia -->
@KristofferC KristofferC removed the backport 1.11 Change should be backported to release-1.11 label Mar 18, 2024
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)
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.

Wrong call of DataType method with UnionAll argument causes segfault
4 participants