Remove inlining's dependence on method table lookups #39792

Keno · 2021-02-23T05:49:21Z

When we switched to stmtinfo, we mostly stopped relying on doing method lookups during inlining.
However, there were a few leftover exceptions, in particular

Special cases like the TypeVar constructor, which had special support in inference and thus did not look into the methods that are supposed to be inlined.
Inlining support for :invoke

This addresses these as well as a few corner cases that are arguable bugs. To address the first kind, we simply need a special purpose inliner that can understand how to properly inline the call despite not having method information. It turns out we already had this for a number of these cases, this PR just completes that. For invoke, we simply need to add a new statement info type and plumb the information through from inference.

Lastly, there is the use case of external optimization pipelines wanting to run multiple iterations of inlining after some more aggressive optimizations. However, I think that is better addressed by those external pipelines annotating the proper info object in a separate pass. One missing ingredient here is that we currently drop info objects after inlining in the optimizer, but I'm working on a separate PR that should address that as well.

I instrumented the fallback case and with this, they are dead code. There are a few cases that we might want to consider giving special purpose inliners in the future (e.g. typename and typejoin), but they weren't being inlined anyway because inference was never visiting them to populate any caches, so this is not a change of behavior.

JeffBezanson · 2021-02-23T16:13:27Z

base/compiler/abstractinterpretation.jl

@@ -981,8 +1003,16 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
            ft = argtype_by_index(argtypes, 3)
            (itft === Bottom || ft === Bottom) && return CallMeta(Bottom, false)
            return abstract_apply(interp, itft, ft, argtype_tail(argtypes, 4), sv, max_methods)
+        elseif f === invoke
+            ft = widenconst(argtype_by_index(argtypes, 2))


Should check argument count.

argtype_by_index will check and also handle the Vararg case correctly (similar to the apply case above).

aviatesk

Nice reduction of code :)

aviatesk · 2021-02-23T17:39:03Z

base/compiler/ssair/inlining.jl

-                new_info = info = nothing
+                new_info = info = false


Why do we prefer false over nothing here ?

nothing used to mean to try looking up the method match in inference, but we don't support that any more after this. Not that we actually ever inlined these cases.

Keno · 2021-02-27T01:22:54Z

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

nanosoldier · 2021-02-27T09:12:12Z

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

vtjnash · 2021-02-27T16:25:01Z

Looks like a couple cases allocate more, a couple less, and no changes in speed

Keno · 2021-03-01T18:16:29Z

I'm gonna take look to see if there's anything obvious wrong in the skipmissing cases, but I have seen those be a little all over the place in the past, since they're quite sensitive to the exact kinds of inlining that happen.

Rather than relying on inlining to perform a method lookup.

Removes another dependence of inlining on the method table and has the nice side benefit of allowing external consumers like Cthulhu to reason about `invoke`.

In some cases we can statically compute the result of a pure call, but we refuse to inline it into the IR because it would be too big. In these cases, we'd still like to be able to do regular inlining, so augment MethodResult pure to forward the call info for such calls rather than relying on inlining to recompute the method match.

This is now no longer used. A use case for external consumers was to re-run another pass of inlining, but that is better addressed by a separate pass that simply sets the appropriate info.

Keno · 2021-03-01T19:47:23Z

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

Keno · 2021-03-01T19:55:15Z

That turned out to be an actual bug that was masked by the previous fallback code: It didn't like the case where inference decided to union split, but only one of the cases actually had an applicable method. With that fixed, I think this'll be good to go, but I've triggered nanosoldier to reconfirm that test.

nanosoldier · 2021-03-01T20:42:44Z

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

Keno · 2021-03-01T20:44:02Z

Confirmed fixed. Will merge once all CI comes back green.

vtjnash · 2021-03-01T20:57:46Z

base/compiler/abstractinterpretation.jl

        elseif isa(rty, Const)
-            return CallMeta(Const(rty.val === false), nothing)
+            return CallMeta(Const(rty.val === false), MethodResultPure())


definitely not pure

because rty used to have actual provenance, but just threw it away?

ah, unfolding context in github further, I see contextually we're guessing that this is the only legal answer from !==?

Nope, I'm still confused. For builtins that are in _PURE_BUILTINS and return Const we just implicitly treat the result as pure. That doesn't apply here, because !== is not a builtin, but we are treating it as if it were one that simply had its result inverted. Why does that invalidate the purity of the call?

jinx with your previous comment, but yes, !== is special.

vtjnash · 2021-03-01T21:00:33Z

base/compiler/ssair/inlining.jl

-        intersect!(et, WorldRange(invoke_data.min_valid, invoke_data.max_valid))
+
+    if !info.match.fully_covers
+        # XXX: We could union split this


Doesn't seem like a bug (e.g. XXX)? Also not true after this PR?

Well, we don't have a standard notion of XXX. In this case it's just here to let people know that'd it'd be ok to do, but I didn't implement it. If can use TODO if you'd prefer. The union splitting that could be accomplished here is inlining the signature check, which isn't really a union split per-se, but the union split code kinda does as a side effect.

True, XXX just looks like flag of bad code, where this is fine

vtjnash · 2021-03-01T21:12:46Z

base/compiler/abstractinterpretation.jl

+    types = rewrap_unionall(Tuple{ft, unwrap_unionall(types).parameters...}, types)
+    nargtype = Tuple{ft, nargtype.parameters...}


argtype isa DataType, but types is not, so neither is nargtype

ah, I see now we bail early in that case

There is an nargtype isa DataType check above. But regardless, this code just moved :)

Keno requested review from vtjnash, JeffBezanson and aviatesk February 23, 2021 05:49

JeffBezanson reviewed Feb 23, 2021

View reviewed changes

aviatesk reviewed Feb 23, 2021

View reviewed changes

Keno added 5 commits March 1, 2021 13:22

Add special case inliner for TypeVar

7e5edad

Rather than relying on inlining to perform a method lookup.

Add statement info for invoke

6cbb3c0

Removes another dependence of inlining on the method table and has the nice side benefit of allowing external consumers like Cthulhu to reason about `invoke`.

Remove method_table from inlining state

7e345f9

This is now no longer used. A use case for external consumers was to re-run another pass of inlining, but that is better addressed by a separate pass that simply sets the appropriate info.

Fix case of inlining UnionSplit with a single applicable method

f166b20

Keno force-pushed the kf/inlinemtable branch from 9ee85d4 to f166b20 Compare March 1, 2021 19:43

vtjnash reviewed Mar 1, 2021

View reviewed changes

vtjnash approved these changes Mar 1, 2021

View reviewed changes

Keno merged commit 3ff44ea into master Mar 2, 2021

Keno deleted the kf/inlinemtable branch March 2, 2021 00:18

henriquebecker91 mentioned this pull request Apr 3, 2021

Write down what we know about Base.@pure #39954

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove inlining's dependence on method table lookups #39792

Remove inlining's dependence on method table lookups #39792

Keno commented Feb 23, 2021

JeffBezanson Feb 23, 2021

Keno Feb 23, 2021

aviatesk left a comment

aviatesk Feb 23, 2021

Keno Feb 23, 2021

Keno commented Feb 27, 2021

nanosoldier commented Feb 27, 2021

vtjnash commented Feb 27, 2021

Keno commented Mar 1, 2021

Keno commented Mar 1, 2021

Keno commented Mar 1, 2021

nanosoldier commented Mar 1, 2021

Keno commented Mar 1, 2021

vtjnash Mar 1, 2021

Keno Mar 1, 2021

vtjnash Mar 1, 2021

vtjnash Mar 1, 2021

Keno Mar 1, 2021

Keno Mar 1, 2021

vtjnash Mar 1, 2021

Keno Mar 1, 2021

vtjnash Mar 1, 2021

vtjnash Mar 1, 2021

vtjnash Mar 1, 2021

Keno Mar 1, 2021

		types = rewrap_unionall(Tuple{ft, unwrap_unionall(types).parameters...}, types)
		nargtype = Tuple{ft, nargtype.parameters...}

Remove inlining's dependence on method table lookups #39792

Remove inlining's dependence on method table lookups #39792

Conversation

Keno commented Feb 23, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aviatesk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Feb 27, 2021

nanosoldier commented Feb 27, 2021

vtjnash commented Feb 27, 2021

Keno commented Mar 1, 2021

Keno commented Mar 1, 2021

Keno commented Mar 1, 2021

nanosoldier commented Mar 1, 2021

Keno commented Mar 1, 2021

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

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