`AbstractInterpreter`: implement `findsup` for `OverlayMethodTable` #44448

aviatesk · 2022-03-04T09:32:06Z

Previously findsup doesn't support OverlayMethodTable at all.
This hasn't been recognized since abstract_invoke didn't use
the method_table interface properly until #44389 ,
but now we need this support.

xref: JuliaGPU/GPUCompiler.jl#303 (comment)

base/compiler/methodtable.jl

base/reflection.jl

vtjnash · 2022-03-04T20:54:56Z

base/compiler/methodtable.jl

+        min_valid[] = typemin(UInt)
+        max_valid[] = typemax(UInt)


technically not legal to reset this, but is perfectly acceptable not to

Suggested change

min_valid[] = typemin(UInt)

max_valid[] = typemax(UInt)

I decided to make a duplicated call to findsup here.

ah, makes sense

though note it is mandatory to preserve the lookup min_valid/max_valid regardless of whether you get a result

note 2 that it is also always mandatory to execute the "fall back to the internal method table" code, unless you can prove that the results are fully covered

vtjnash · 2022-03-05T02:58:31Z

base/compiler/methodtable.jl

 end

 function findall(@nospecialize(sig::Type), table::OverlayMethodTable; limit::Int=typemax(Int))
+    result = _findall(sig, table.mt, table.world, limit)
+    result === missing || isempty(result) || return result


If this part of the result is missing, then the total result must also be missing

Though I find the change between false and missing to be somewhat odd for the API–you don't get less by adding more. Perhaps we should use something consistent such as nothing? (even true might be a better choice, since it means we found too many matches)

Yeah, I think nothing or missing would be better, since they are easier to be handled by Conditional? Given that such case means "too many matches", missing sounds a bit counter-intuitive.

Continuation from <#44448 (comment)>. Previously `ml_matches` (and its dependent utilities like `_methods_by_ftype`) returned `false` for cases when there are too many matching method, but its consumer like `findall(::Type, ::InternalMethodTable)` usually handles such case as `missing`. This commit does a refactor so that they all use the more consistent value `nothing` for representing that case.

Now `jl_gf_invoke_lookup` accepts an optional overlayed method table, but actual code execution (as done by JuliaInterpreter) doesn't need to care about it at this moment.

vtjnash · 2022-03-08T00:27:52Z

base/compiler/methodtable.jl

+    result = _findall(sig, table.mt, table.world, limit)
+    result === missing && return missing
+    if !isempty(result)
+        if all(match->match.fully_covers, result)


It only matters if result[end].fully_covers

Let me check my understanding.
We can just look at result[end].fully_covers since:

jl_matching_methods returns matching methods in order of speciality
(and so the last match is always the most general case)

if the last match fully_covers, it means this call is assured to be
dispatched to the last match when it's not dispatched with the other matches

vtjnash · 2022-03-08T00:28:50Z

base/compiler/methodtable.jl

+                WorldRange(min(result.valid_worlds.min_world, fallback_result.valid_worlds.min_world),
+                           max(result.valid_worlds.max_world, fallback_result.valid_worlds.max_world)),


you need the max of the min_worlds and the min of the max_worlds

vtjnash · 2022-03-08T00:30:01Z

base/compiler/methodtable.jl

+                result.ambig | fallback_result.ambig)
+        end
+    end
+    # fall back to the internal method table


Even if the result is empty, you are still required to return the valid_worlds data from the (failed) lookup

vtjnash · 2022-03-08T00:32:58Z

base/compiler/methodtable.jl

-    (result.method, WorldRange(min_valid[], max_valid[]))
+    result = ccall(:jl_gf_invoke_lookup_worlds, Any, (Any, Any, UInt, Ptr{Csize_t}, Ptr{Csize_t}),
+                   sig, mt, world, min_valid, max_valid)::Union{MethodMatch, Nothing}
+    return result === nothing ? result : (result, WorldRange(min_valid[], max_valid[]))


This is conservatively correct currently (since we return Any), but it would be more correct to return the tuple unconditionally, since the WorldRange applies to the lookup, regardless of the result.

- respect world range of failed lookup into `OverlayMethodTable`: <#44448 (comment)> - fix calculation of merged valid world range: <#44448 (comment)> - make `findsup` return valid `WorldRange` unconditionally, and enable more strict world validation within `abstract_invoke`: <#44448 (comment)> - fix the default `limit::Int` value of `findall`

…44448)

#44511) - respect world range of failed lookup into `OverlayMethodTable`: <#44448 (comment)> - fix calculation of merged valid world range: <#44448 (comment)> - make `findsup` return valid `WorldRange` unconditionally, and enable more strict world validation within `abstract_invoke`: <#44448 (comment)> - fix the default `limit::Int` value of `findall`

Continuation from <#44448 (comment)>. Previously `ml_matches` (and its dependent utilities like `_methods_by_ftype`) returned `false` for cases when there are too many matching method, but its consumer like `findall(::Type, ::InternalMethodTable)` usually handles such case as `missing`. This commit does a refactor so that they all use the more consistent value `nothing` for representing that case.

…44478) Continuation from <#44448 (comment)>. Previously `ml_matches` (and its dependent utilities like `_methods_by_ftype`) returned `false` for cases when there are too many matching method, but its consumer like `findall(::Type, ::InternalMethodTable)` usually handles such case as `missing`. This commit does a refactor so that they all use the more consistent value `nothing` for representing that case.

aviatesk requested a review from Keno March 4, 2022 09:32

aviatesk mentioned this pull request Mar 4, 2022

update to JuliaLang/julia#44389 JuliaGPU/GPUCompiler.jl#303

Merged

Keno reviewed Mar 4, 2022

View reviewed changes

base/compiler/methodtable.jl Outdated Show resolved Hide resolved

Keno reviewed Mar 4, 2022

View reviewed changes

base/reflection.jl Outdated Show resolved Hide resolved

vtjnash approved these changes Mar 4, 2022

View reviewed changes

vtjnash reviewed Mar 4, 2022

View reviewed changes

aviatesk force-pushed the avi/overlayedfindsup branch from 3716665 to 246b900 Compare March 4, 2022 23:49

vtjnash reviewed Mar 5, 2022

View reviewed changes

aviatesk force-pushed the avi/overlayedfindsup branch from 246b900 to 219f08d Compare March 6, 2022 04:02

aviatesk mentioned this pull request Mar 6, 2022

make ml_matches return nothing when there are too many matches #44478

Merged

AbstractInterpreter: implement findsup for OverlayMethodTable

c86d43a

aviatesk force-pushed the avi/overlayedfindsup branch from 219f08d to c86d43a Compare March 7, 2022 01:21

aviatesk merged commit 9a48dc1 into master Mar 7, 2022

aviatesk deleted the avi/overlayedfindsup branch March 7, 2022 04:16

maleadt mentioned this pull request Mar 7, 2022

A number of packages failed to precompile on Julia master (macOS Apple Silicon) #44496

Closed

vtjnash reviewed Mar 8, 2022

View reviewed changes

aviatesk mentioned this pull request Mar 8, 2022

follow up #44448, correct findall/findsup for OverlayMethodTable #44511

Merged

aviatesk mentioned this pull request Mar 9, 2022

Backports for julia 1.8.0-beta2 #44324

Merged

47 tasks

aviatesk added a commit that referenced this pull request Mar 9, 2022

AbstractInterpreter: implement findsup for OverlayMethodTable (#…

d15704a

…44448)

aviatesk added the backport 1.8 Change should be backported to release-1.8 label Mar 14, 2022

KristofferC removed the backport 1.8 Change should be backported to release-1.8 label Mar 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`AbstractInterpreter`: implement `findsup` for `OverlayMethodTable` #44448

`AbstractInterpreter`: implement `findsup` for `OverlayMethodTable` #44448

aviatesk commented Mar 4, 2022

vtjnash Mar 4, 2022

aviatesk Mar 5, 2022

vtjnash Mar 5, 2022

vtjnash Mar 5, 2022 •

edited

Loading

aviatesk Mar 6, 2022

aviatesk Mar 6, 2022

vtjnash Mar 8, 2022

aviatesk Mar 8, 2022

vtjnash Mar 8, 2022

vtjnash Mar 8, 2022

vtjnash Mar 8, 2022

		WorldRange(min(result.valid_worlds.min_world, fallback_result.valid_worlds.min_world),
		max(result.valid_worlds.max_world, fallback_result.valid_worlds.max_world)),

AbstractInterpreter: implement findsup for OverlayMethodTable #44448

AbstractInterpreter: implement findsup for OverlayMethodTable #44448

Conversation

aviatesk commented Mar 4, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vtjnash Mar 5, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

`AbstractInterpreter`: implement `findsup` for `OverlayMethodTable` #44448

`AbstractInterpreter`: implement `findsup` for `OverlayMethodTable` #44448

vtjnash Mar 5, 2022 •

edited

Loading