Inline statically known method errors.

[gbaraldi]

This replaces the Expr(:call, ...) with a call of a new builtin Core.throw_methoderror

This is useful because it makes very clear if something is a static method error or a plain dynamic dispatch that always errors.
Tools such as AllocCheck or juliac can notice that this is not a genuine dynamic dispatch, and prevent it from becoming a false positive compile-time error.

Dependent on #55705

[vtjnash]

I generally dislike optimizing codepaths that aren't performance critical, as it bloats the code, slows the compiler, and introduces more possibilities for subtle discrepancies in what error is generated and thrown

[topolarity]

I generally dislike optimizing codepaths that aren't performance critical, as it bloats the code, slows the compiler, and introduces more possibilities for subtle discrepancies in what error is generated and thrown

I share that concern, but in this case the transformation is pretty minimal I think.

It turns:

Expr(:call, foo, args...)::Union{}

into:

%world = Expr(:foreigncall, :(:jl_get_tls_world_age), UInt64, svec(), 0, :(:ccall))::UInt64
%argtuple = Expr(:call, tuple, args...)
%err = Expr(:new, MethodError, farg, %argtuple, %world)
Expr(:call, throw, %err)

(note that since #54537 the %world computation is lifted to function entry, at least once it reaches codegen)

Those 2-3 extra instructions don't seem too bad to me.

[aviatesk]

Suggested change

matches::UnionSplitMethodMatches

[aviatesk]

This change does not seem to bloat up the sysimg, but I have the same concern and would prefer to make it an optional feature enabled by a new optimization parameter.

[topolarity]

This change does not seem to bloat up the sysimg, but I have the same concern and would prefer to make it an optional feature enabled by a new optimization parameter.

We can do that, but it puts a tool like AllocCheck in an awkward position since it would now want to run with custom inference for precision purposes - but that can now diverge from the codegen/analysis for the standard inference settings (even if the "dynamic" dispatch itself is harmless in both cases) so @check_allocs in your tests may not give you the guarantees you want at runtime when you don't use it

That guarantee is already weak (since inference can be noticeably stateful in, e.g., recursive cycles), but I'd prefer not to make it worse if we can avoid it

[topolarity]

To avoid IR bloat, would it preferable to lower this to a new apply-like builtin instead?

Something like Expr(:call, Core.throw_method_error, f, args...) or Expr(:method_error, ...)

[aviatesk]

To avoid IR bloat, would it preferable to lower this to a new apply-like builtin instead?

Something like Expr(:call, Core.throw_method_error, f, args...) or Expr(:method_error, ...)

This seems like one possible approach, but adding it solely for this purpose feels a bit hesitant. This is because inlining the code to raise a MethodError in this case offers (almost) no performance or latency benefits. That said, we do add new intrinsics quite frequently, so it might not be a big issue though.
Instead, I believe it would be cleaner to leave the fallback :call in such cases, but also set its :type to Union{}, and make the next statement :unreachable? The ultimate goal of this PR is, if I understand correctly, to make the "must-throw" information for such :call visible at the LLVM level. And inserting :unreachable seems to be the cleanest solution to achieve that.

[gbaraldi]

The issue is that a call that does something and then errors vs something that immediately errors is different IMO. i.e I have a dispatch to something that does a while true loop and the only way it exits is by erroring.

[aviatesk]

I see. But if we assume that allocations on throw code path are not performance-critical, wouldn't it be okay for AllocCheck to ignore such calls?
Since the current implementation of inference is based on that assumption, it seems natural to reflect that in AllocCheck as well.

[topolarity]

AllocCheck also supports analysis including throw paths, it's just not turned on by default.

[Keno]

Unrelated?

[topolarity]

This was a race condition in the test that started failing after these changes, for some reason

[topolarity]

Alright this now turns the call into:

Core.throw_methoderror(f, args...)
unreachable

which should hopefully resolve the IR bloat concerns

[aviatesk]

LGTM modulo the comment posted above.

[gbaraldi]

Should we run benchmarks/pkgeval?

[topolarity]

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

PkgEval is probably unnecessary - this shouldn't have any observable semantic differences

[nanosoldier]

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

[aviatesk]

With this default implementation, for any arbitrary custom CallInfo, as long as it correctly implements getsplit_impl, the method error case inlining enabled by this PR will occur. However, if that CallInfo does not implement add_uncovered_edges_impl, there could theoretically be cases where the necessary edges are not added. In the version I proposed, the getmatchinfo(::CallInfo, ::Int) interface is enforced to be implemented for all CallInfos, so I think such interface requirement errors are easy to detect. That said, this is a very rare case, so I think it’s just fine to revisit it later when there’s more time.

[topolarity]

Yeah, I agree that it'd be better to make this error if unimplemented (TBH I don't know why we provide a default implementation for any of these)

The getmatchinfo(::CallInfo, ::Int) isn't quite right, because it treats uncovered edges as being associated with split indices, but they are the edge(s) not covered by the split cases (e.g. you might have 10 split cases, and then an uncovered atype that needs an edge to 2 method tables)

[aviatesk]

The getmatchinfo(::CallInfo, ::Int) isn't quite right, because it treats uncovered edges as being associated with split indices, but they are the edge(s) not covered by the split cases (e.g. you might have 10 split cases, and then an uncovered atype that needs an edge to 2 method tables)

Right... So maybe getuncoveredmt(::CallInfo) -> iterator of method tables with !fullmatch?

[aviatesk]

why we provide a default implementation for any of these

Since they are optional. If CallInfo implements nsplit, then the inlining for the call is enabled and the CallInfo is required to implement getsplit. And getresult is optional, and if it's implemented, then the inliner may use a result from extended lattice inference (a.k.a. const-prop and semi-concrete interpretation).

[topolarity]

That's helpful - sounds like this one should be:

add_uncovered_edges_impl(edges::Vector{Any}, info::CallInfo, @nospecialize(atype)) = error("unexpected call into add_uncovered_edges!")

since we also only do this if you implement nsplit

That said, getuncoveredmt would also be OK w/ me - the API shape seems orthogonal to the optionality, etc.

[vtjnash]

I intend for us to eliminate all this backwards-designed tracking code for better correctness, as soon as someone can help finish #54894

[aviatesk]

Would it be okay to revert back to storing Vector{MethodMatchInfo} like before? The current design, where matches/mts/fullmatches can have different lengths, feels a bit tricky to handle. I understand it's meant to avoid adding duplicate edges to the same mt in add_uncovered_edges_impl, but in practice, adding duplicate edges isn't really an issue. From a complexity standpoint, I think the previous approach is preferable.

[topolarity]

How would you feel about:

Suggested change

	matches::Vector{MethodMatchInfo}
	matches::Vector{MethodLookupResult}
	mts::Vector{MethodTable}
	fullmatches::Vector{Bool}
	matches::Vector{MethodLookupResult}
	mt_edges::Vector{@NamedTuple{mt::MethodTable, fullmatch::Bool}}

or similar?

That's more consistent with what UnionSplitMethodMatches carried before, which always had these edges de-duplicated.

[aviatesk]

Right, the previous design of UnionSplitMethodMatches also avoided adding duplicated edges. But I still feel that it's better for UnionSplitInfo or UnionSplitMethodMatches to have a simpler data structure. We can avoid duplication when adding edges.

[topolarity]

Sure, feel free to open a PR