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: remove ineffective handling for unmatched params #52092

Merged
merged 1 commit into from
Mar 13, 2024

Conversation

aviatesk
Copy link
Member

@aviatesk aviatesk commented Nov 9, 2023

The deleted branch was added in #45062, although it had not been tested.

I tried the following diff to find cases optimized by that, but I just found the handling proved to be in vain in all cases I tried.

diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl
index 318b21b09b..7e42a65aa4 100644
--- a/base/compiler/ssair/inlining.jl
+++ b/base/compiler/ssair/inlining.jl
@@ -1473,6 +1473,14 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt32, sig
         handle_any_const_result!(cases,
             result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=true)
         fully_covered = handled_all_cases = match.fully_covers
+        if length(cases) == 1 && fully_covered
+            println("first case: ", only_method)
+        elseif length(cases) == 1
+            atype = argtypes_to_type(sig.argtypes)
+            if atype isa DataType && cases[1].sig isa DataType
+                println("second case: ", only_method)
+            end
+        end
     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)

@aviatesk aviatesk requested a review from Keno November 9, 2023 07:11
@Keno
Copy link
Member

Keno commented Nov 9, 2023

This code is not supposed to be dead. Here's a test case that's supposed to trigger it:

function foo(x::Union{Int, Float64}, y::T) where {T}
    Base.donotdelete(T)
end
function bar(b::Bool, @nospecialize(y))
     foo(b ? 1 : 1.0, y)
end

I think this broke when we started relying more heavily on inference to push forward info objects.

@aviatesk
Copy link
Member Author

aviatesk commented Nov 13, 2023

I found this fails because there is no cached source to be inlined available for foo(:Union{Int, Float64}, y::T) (in common cases) since inference union-splits, although inlining will hit this branch in such a case:

if nsplit(info)::Int > 1
atype = argtypes_to_type(argtypes)
(metharg, methsp) = ccall(:jl_type_intersection_with_env, Any, (Any, Any), atype, only_method.sig)::SimpleVector
match = MethodMatch(metharg, methsp::SimpleVector, only_method, true)
result = nothing

@aviatesk
Copy link
Member Author

I don't think it's profitable to try union-split ininling with unmatched parameters in such a case, since SROA can't eliminate them anyway.

@aviatesk aviatesk force-pushed the avi/rm-dead-unmatched-param-handling branch from 91674b5 to 11c72c0 Compare November 13, 2023 10:29
The deleted branch was added in #45062, although it had not been tested.

I tried the following diff to find cases optimized by that, but I just
found the handling proved to be in vain in all cases I tried.
```diff
diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl
index 318b21b09b..7e42a65aa4 100644
--- a/base/compiler/ssair/inlining.jl
+++ b/base/compiler/ssair/inlining.jl
@@ -1473,6 +1473,14 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt32, sig
         handle_any_const_result!(cases,
             result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=true)
         fully_covered = handled_all_cases = match.fully_covers
+        if length(cases) == 1 && fully_covered
+            println("first case: ", only_method)
+        elseif length(cases) == 1
+            atype = argtypes_to_type(sig.argtypes)
+            if atype isa DataType && cases[1].sig isa DataType
+                println("second case: ", only_method)
+            end
+        end
     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)
```
@aviatesk aviatesk force-pushed the avi/rm-dead-unmatched-param-handling branch from 11c72c0 to 62cdb6e Compare March 13, 2024 07:43
@aviatesk
Copy link
Member Author

I'm thinking of moving this PR forward first to simplify the fix for #52644. Right now, the handling we're talking about isn't really turned on. If we decide to add similar features in the future, we'll likely need some extra help from the inference side of things. When that happens, we can bring this back with some effective test cases.

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 aviatesk changed the title inlining: remove (probably) dead handling for unmatched params inlining: remove ineffective handling for unmatched params Mar 13, 2024
Comment on lines +1378 to +1380
if handled_all_cases
if revisit_idx !== nothing
# we handled everything except one match with unmatched sparams,
Copy link
Member

Choose a reason for hiding this comment

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

Why do we set handled_all_cases if we didn't handle everything? This comment seems confusing here

Copy link
Member Author

Choose a reason for hiding this comment

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

revisit_idx might be effective even when there are other matches that have been handled properly. But I agree that this can be confusing. I'm working on some more clean up.

@vtjnash vtjnash merged commit 2e876fc into master Mar 13, 2024
8 checks passed
@vtjnash vtjnash deleted the avi/rm-dead-unmatched-param-handling branch March 13, 2024 16:23
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
The deleted branch was added in #45062, although it had not been tested.

I tried the following diff to find cases optimized by that, but I just
found the handling proved to be in vain in all cases I tried.
```diff
diff --git a/base/compiler/ssair/inlining.jl b/base/compiler/ssair/inlining.jl
index 318b21b09b..7e42a65aa4 100644
--- a/base/compiler/ssair/inlining.jl
+++ b/base/compiler/ssair/inlining.jl
@@ -1473,6 +1473,14 @@ function compute_inlining_cases(@nospecialize(info::CallInfo), flag::UInt32, sig
         handle_any_const_result!(cases,
             result, match, argtypes, info, flag, state; allow_abstract=true, allow_typevars=true)
         fully_covered = handled_all_cases = match.fully_covers
+        if length(cases) == 1 && fully_covered
+            println("first case: ", only_method)
+        elseif length(cases) == 1
+            atype = argtypes_to_type(sig.argtypes)
+            if atype isa DataType && cases[1].sig isa DataType
+                println("second case: ", only_method)
+            end
+        end
     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)
```
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 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.

3 participants