[NewOptimizer] Union Split during Inlining #26900

Keno · 2018-04-25T14:42:58Z

This adds union splitting optimizations to the inliner.
This works a bit differently from the old optimizer, which
did union splitting on the bail out path. Instead, we try
to run the full inliner on any union split case that's a
dispatch tuple (and record what to do in that case). Doing
that allows an effective resolution of #23338, though this
PR doesn't quite do that yet, since some further (minor) fixes
are needed.

vtjnash · 2018-04-25T16:01:25Z

base/compiler/utilities.jl

+    opt_check_bounds == 0 && return :default
+    opt_check_bounds == 1 && return :on
+    return :off
+end


editor issue?

vtjnash · 2018-04-25T16:05:03Z

base/compiler/ssair/inlining2.jl

+        case === nothing
+        @assert !isa(metharg, UnionAll)
+        cond = true
+        for (i, (a, m)) in Iterators.drop(enumerate(zip(atype.parameters, metharg.parameters)), 1)


don't put Types in Tuples

vtjnash · 2018-04-25T16:06:55Z

base/compiler/ssair/inlining2.jl

+    return_value
+end
+
+function ir_inline_unionsplit!(compact, idx, argexprs, linetable, item, boundscheck, todo_bbs)


annotate your argument types

vtjnash · 2018-04-25T16:09:19Z

base/compiler/ssair/inlining2.jl

+                cond = insert_node_here!(compact, and_expr, Bool, line)
+            end
+        end
+        atype = typesubtract(atype, metharg)


attempting this operation is wrong, although since atype is a DataType, it's at least a no-op

vtjnash · 2018-04-25T16:15:18Z

base/compiler/ssair/inlining2.jl

+    has_generic = false
+    @assert length(item.bbs) > length(item.cases)
+    for ((metharg, case), next_cond_bb) in zip(item.cases, item.bbs)
+        case === nothing


vtjnash · 2018-04-25T16:23:53Z

base/compiler/ssair/inlining2.jl

+            val = ir_inline_item!(compact, idx, argexprs, linetable, case, boundscheck, todo_bbs)
+        elseif isa(case, MethodInstance)
+            # Make it an Expr(:invoke)
+            error()


need to fix error() this

vtjnash · 2018-04-25T16:24:05Z

base/compiler/ssair/inlining2.jl

+        elseif isa(case, ConstantCase)
+            val = case.val
+        else
+            error()


need to fix this

Keno · 2018-05-01T20:05:40Z

I've been running this branch locally and it seems to work fine, so I'm planning to merge this in a bit. As always comments are still open afterwards.

vtjnash · 2018-05-01T20:17:39Z

Seems like this design will probably regress our ability to do union-splitting a bit (this can only handle cases that intersect to create dispatch tuples, while the approach of the existing code can handle more general cases with fewer isa tests), but I understand most review is being deferred for later

vtjnash · 2018-05-01T20:19:22Z

base/compiler/ssair/inlining2.jl

+        isa(methsp[i], TypeVar) && return nothing
+    end
+
+    # Find the linfo for this methods (Generated functions are expanded here if necessary)


I hope not: the last parameter (true) means "do not expand generated functions"

Right, we discussed that on a past review. I'll separately fix the comment.

vtjnash · 2018-05-01T20:19:58Z

base/compiler/ssair/inlining2.jl

+        ast = copy_exprargs(inferred.code)
+    else
+        src = ccall(:jl_uncompress_ast, Any, (Any, Any), method, inferred::Vector{UInt8})::CodeInfo
+        # TODO: It seems like PhiNodes are shared between compressed codeinfos, making this copy necessary


This is a serious bug with the serializer, if (still) true

I suspect this got fixed.

vtjnash · 2018-05-01T20:27:35Z

base/compiler/ssair/inlining2.jl

+                found_any = false
+                for (i, match) in enumerate(meth)
+                    (metharg, methsp, method) = (match[1]::Type, match[2]::SimpleVector, match[3]::Method)
+                    metharg′ <: metharg || continue


type-intersect

Isn't that equivalent, since metharg′ is a dispatch tuple?

ah yes, although note that metharg is not relevant to whether the method will actually match at runtime

Could you elaborate that statement?

metharg is typeintersect(method.sig, atypes), so it's unpredictable whether it's method.sig, atypes, or something wider or narrower. basically, just assume that it's Tuple and you should be OK.

Ok, so should that be method.sig on the RHS of this check then?

yeah, that should work

Keno · 2018-05-01T20:28:12Z

Seems like this design will probably regress our ability to do union-splitting a bit (this can only handle cases that intersect to create dispatch tuples, while the approach of the existing code can handle more general cases with fewer isa tests), but I understand most review is being deferred for later

Examples of which cases you don't like with the new behavior would be good. Last time we discussed this you and @JeffBezanson told me that you'd prefer to go through the dynamic dispatch for everything that's not a dispatchtuple. In any case, we can do all of that later.

vtjnash · 2018-05-01T20:50:10Z

We'd like to also be able to generate direct calls in the presence of @nospecialize cases (predicated on whether inlining succeeds instead – there's a partial predicate for this "jl_is_cacheable_sig" in src).

This adds union splitting optimizations to the inliner. This works a bit differently from the old optimizer, which did union splitting on the bail out path. Instead, we try to run the full inliner on any union split case that's a dispatch tuple (and record what to do in that case). Doing that allows an effective resolution of #23338, though this PR doesn't quite do that yet, since some further (minor) fixes are needed.

Keno mentioned this pull request Apr 25, 2018

Benchmark new optimizer #26795

Closed

18 tasks

vtjnash reviewed Apr 25, 2018

View reviewed changes

base/compiler/ssair/inlining2.jl Outdated

elseif isa(case, ConstantCase)

val = case.val

else

error()

Copy link

Member

vtjnash Apr 25, 2018

Choose a reason for hiding this comment

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

need to fix this

ararslan added the compiler:optimizer Optimization passes (mostly in base/compiler/ssair/) label Apr 25, 2018

Keno force-pushed the kf/unionsplit branch 4 times, most recently from 2cfee6c to 6a6b96e Compare May 1, 2018 15:25

vtjnash reviewed May 1, 2018

View reviewed changes

Keno added 3 commits May 1, 2018 17:19

Widen element type for processed array

3305ef2

Correct comment

8f4ff91

Keno force-pushed the kf/unionsplit branch from 6a6b96e to 8f4ff91 Compare May 1, 2018 21:27

Keno merged commit 7d48ecb into master May 2, 2018

martinholters deleted the kf/unionsplit branch May 2, 2018 07:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NewOptimizer] Union Split during Inlining #26900

[NewOptimizer] Union Split during Inlining #26900

Keno commented Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

vtjnash Apr 25, 2018

Keno commented May 1, 2018

vtjnash commented May 1, 2018

vtjnash May 1, 2018

Keno May 1, 2018

vtjnash May 1, 2018

Keno May 1, 2018

vtjnash May 1, 2018

Keno May 1, 2018

vtjnash May 1, 2018 •

edited

Loading

Keno May 1, 2018

vtjnash May 1, 2018

Keno May 1, 2018

vtjnash May 1, 2018

Keno commented May 1, 2018

vtjnash commented May 1, 2018 •

edited

Loading

[NewOptimizer] Union Split during Inlining #26900

[NewOptimizer] Union Split during Inlining #26900

Conversation

Keno commented Apr 25, 2018

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

Keno commented May 1, 2018

vtjnash commented May 1, 2018

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

vtjnash May 1, 2018 • 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

Keno commented May 1, 2018

vtjnash commented May 1, 2018 • edited Loading

vtjnash May 1, 2018 •

edited

Loading

vtjnash commented May 1, 2018 •

edited

Loading