Put inference parameters in a dedicated object #18496

maleadt · 2016-09-13T22:50:19Z

Part of #18338, based on #18396 for now because it uses kwargs in inference.

Entry-point function parameters to type inference (needtree, optimize, cached) are now saved in the InferenceState, which makes them available in more places. However, using cached that way breaks abstract_call_gf_by_type. Any reason this should always be saving its results?

Performance difference in loading base/inference.jl seems undetectable. Suggestions for better testing of type inference performance?

cc @JeffBezanson @vtjnash

JeffBezanson · 2016-09-14T01:40:56Z

Good tests of inference performance are the sys.so build, and some of the longer-running test suites like subarray.

JeffBezanson · 2016-09-14T01:42:32Z

base/inference.jl

+    cached::Bool
+
+    # parameters limiting potentially-infinite types
+    MAX_TYPEUNION_LEN::Int


I would just keep all of these as global constants unless you have a use case for changing them on some calls.

JeffBezanson · 2016-09-14T01:45:10Z

I don't really like that this makes a whole bunch of simple pure functions depend on an extra parameter that they usually don't use.

JeffBezanson · 2016-09-14T01:54:29Z

However, using cached that way breaks abstract_call_gf_by_type. Any reason this should always be saving its results?

The only reason we currently pass cached=false is to look at a single function e.g. without inlining. Any other functions recursively inferred during that process can still be optimized and cached as normal. In fact it is important not to propagate the parameters passed to inference, or we can end up caching non-optimized versions of functions. For example if I want to see the IR for f() = 1+1 without inlining, we will end up inferring +(::Int,::Int) in the process, so we might as well save the result. The cache also prevents recursive functions from being inferred infinitely.

maleadt · 2016-09-14T13:22:02Z

I don't really like that this makes a whole bunch of simple pure functions depend on an extra parameter that they usually don't use.

I agree, but I felt like keeping some const and others configurable was a half-assed job. I'll try to strike a better balance, IIRC most of the diff is caused byMAX_TUPLETYPE_LEN and MAX_TYPE_DEPTH anyway.

The cache also prevents recursive functions from being inferred infinitely.

Right, didn't realize this, that explains the hang.

In fact it is important not to propagate the parameters passed to inference, or we can end up caching non-optimized versions of functions.

OK, so I've changed that behavior in this PR, propagating all parameters but not caching the result instead.

The reason I wanted to propagate parameters for an entire inference "cycle", is because of the hooks I'm testing in #18338. Replacing a call applies to every function in a cycle, not only the top one. By propagating all parameters, including cached, we avoid re-using or polluting anything in the cache (except that it breaks recursion handling). This also side-steps the issue of needing multiple caches.

maleadt · 2016-10-12T19:04:44Z

Updated and rebased.

EDIT: also, no performance difference running subarray test.

I would still like to change the caching behavior, where inferring with cached=false adds called functions to the cache in order to break recursion. I don't fully grok the interplay between inference & the method tables, so any suggestions on how to do this @JeffBezanson @vtjnash?

vtjnash · 2016-10-12T20:17:24Z

base/inference.jl

@@ -3951,10 +3980,13 @@ function reindex_labels!(sv::InferenceState)
    end
 end

-function return_type(f::ANY, t::ANY)
+function return_type(f::ANY, t::ANY, params::InferenceParams=InferenceParams())
+    # TODO: are all local calls to return_type processed by pure_eval_call?


vtjnash · 2016-10-12T20:18:06Z

base/inference.jl

@@ -1590,7 +1618,7 @@ function typeinf_edge(method::Method, atypes::ANY, sparams::SimpleVector, caller
            end
        end
    end
-    frame = typeinf_frame(code, true, true, caller)
+    frame = typeinf_frame(code, caller, InferenceParams(caller.params; cached=true))


this is a hot path, it would be nice to avoid the kwargs dispatch and allocation here

OK, removed.

vtjnash · 2016-10-12T20:26:30Z

Can we leave out cached for now? I'll try to help you with making a selectable cache, but for now, I'm concerned it will just make #17057 trickier for me, since we're already overloading its meaning somewhat.

maleadt · 2016-10-12T20:33:29Z

Yeah sure, I almost did already but left it in not knowing what a better solution would look like.
Dropping cached also obviates the need for the ugly kwarg copy ctor.

I can also maintain this PR until after #17057, if you like.

vtjnash · 2016-10-12T20:52:09Z

the rest of it shouldn't conflict (other than superficially, and I already know I'll have to deal with rebasing other stuff anyways)

maleadt · 2016-10-12T21:21:11Z

I've also left out optimize, because without proper caching (or copying-and-modifying the InferenceParams when passing down) a nonoptimized method could end up in the cache.

tkelman · 2016-10-13T07:19:47Z

base/inference.jl

+    # reasonable defaults
+    InferenceParams(;inlining::Bool=inlining_enabled(),
+                    tupletype_len::Int=15, tuple_depth::Int=16,
+                    tuple_splat::Int=4, union_splitting::Int=4) =


this is swapping the defaults for depth and splat

That's bad. Nicely spotted, fixed.

This allows overriding parameters, while reducing global state and simplifying the API.

JeffBezanson · 2016-10-19T23:44:17Z

Is there a particular reason those 4 globals were moved into the struct, and the other 2 were kept global?

maleadt · 2016-10-20T00:12:12Z

I initially put them all in the struct, but that caused too much churn (as you mentioned above, making a whole bunch of simple pure functions depend on an extra parameter). So I took the ones causing the largest diff out.

andyferris · 2017-04-20T02:09:12Z

Out of curiosity, when we say "configurable" is e.g. MAX_TUPLE_SPLAT something I can specify on startup, when building a system image, or at the REPL?

maleadt · 2017-04-26T06:08:03Z

@andyferris: no, apart from changing the default value in inference.jl, or when calling into inference yourself (eg. this demo).

andyferris · 2017-04-26T09:19:10Z

Ok - thanks @maleadt

maleadt added the compiler:inference Type inference label Sep 13, 2016

StefanKarpinski added this to the 0.6.0 milestone Sep 14, 2016

StefanKarpinski assigned vtjnash and JeffBezanson Sep 14, 2016

JeffBezanson reviewed Sep 14, 2016
View reviewed changes

maleadt force-pushed the tb/inference_params branch from 673194b to f0a32b1 Compare September 14, 2016 14:30

maleadt force-pushed the tb/inference_params branch 3 times, most recently from 59c365c to 6f45396 Compare October 12, 2016 18:53

vtjnash reviewed Oct 12, 2016

View reviewed changes

maleadt force-pushed the tb/inference_params branch 2 times, most recently from 7048bc4 to a19f415 Compare October 12, 2016 21:17

maleadt mentioned this pull request Oct 12, 2016

META: CUDA external language implementation #18338

Closed

9 tasks

maleadt force-pushed the tb/inference_params branch from a19f415 to c32870c Compare October 13, 2016 00:19

tkelman reviewed Oct 13, 2016

View reviewed changes

maleadt force-pushed the tb/inference_params branch 2 times, most recently from 73f3de3 to 56e8b60 Compare October 17, 2016 14:04

Put inference parameters in a dedicated object.

dc2f727

This allows overriding parameters, while reducing global state and simplifying the API.

maleadt force-pushed the tb/inference_params branch from 56e8b60 to dc2f727 Compare October 18, 2016 17:14

maleadt merged commit d80ad6e into master Oct 19, 2016

tkelman deleted the tb/inference_params branch October 19, 2016 12:13

maleadt mentioned this pull request Oct 20, 2016

RFC: CodegenParams for external language implementations #19046

Merged

TotalVerb mentioned this pull request Apr 20, 2017

Add inlined methods for ntuple(f, Val{N}) for 0 ≤ N ≤ 15 #21446

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Put inference parameters in a dedicated object #18496

Put inference parameters in a dedicated object #18496

maleadt commented Sep 13, 2016

JeffBezanson commented Sep 14, 2016

JeffBezanson Sep 14, 2016

JeffBezanson commented Sep 14, 2016

JeffBezanson commented Sep 14, 2016

maleadt commented Sep 14, 2016

maleadt commented Oct 12, 2016 •

edited

Loading

vtjnash Oct 12, 2016

vtjnash Oct 12, 2016

maleadt Oct 12, 2016

vtjnash commented Oct 12, 2016

maleadt commented Oct 12, 2016

vtjnash commented Oct 12, 2016

maleadt commented Oct 12, 2016

tkelman Oct 13, 2016

maleadt Oct 13, 2016

JeffBezanson commented Oct 19, 2016

maleadt commented Oct 20, 2016

andyferris commented Apr 20, 2017

maleadt commented Apr 26, 2017

andyferris commented Apr 26, 2017

Put inference parameters in a dedicated object #18496

Put inference parameters in a dedicated object #18496

Conversation

maleadt commented Sep 13, 2016

JeffBezanson commented Sep 14, 2016

JeffBezanson Sep 14, 2016

Choose a reason for hiding this comment

JeffBezanson commented Sep 14, 2016

JeffBezanson commented Sep 14, 2016

maleadt commented Sep 14, 2016

maleadt commented Oct 12, 2016 • edited Loading

vtjnash Oct 12, 2016

Choose a reason for hiding this comment

vtjnash Oct 12, 2016

Choose a reason for hiding this comment

maleadt Oct 12, 2016

Choose a reason for hiding this comment

vtjnash commented Oct 12, 2016

maleadt commented Oct 12, 2016

vtjnash commented Oct 12, 2016

maleadt commented Oct 12, 2016

tkelman Oct 13, 2016

Choose a reason for hiding this comment

maleadt Oct 13, 2016

Choose a reason for hiding this comment

JeffBezanson commented Oct 19, 2016

maleadt commented Oct 20, 2016

andyferris commented Apr 20, 2017

maleadt commented Apr 26, 2017

andyferris commented Apr 26, 2017

maleadt commented Oct 12, 2016 •

edited

Loading