Inference annotations that can be read by an external tool #35845

Keno · 2020-05-12T02:43:24Z

This builds on top of #35831, letting inference emit a custom message
whenever it gives up on infering something. These messages are intended
to be displayed by external tools, either to debug what inference is doing
(e.g. for Cthulhu) or, if an external compiler needs to disallow certain
kinds of missing information (e.g. no dynamic dispatch on GPUs), can be
used to improve diagnostics. This is mostly a proof of concept, I think
these messages/hooks need to be a bit richer for a full solution, though
I think this can be already useful if we hook up something like Cthulhu.
As a proof of concept, I hacked up a 10 line function that reads these messagse.
It works something like the following:

function bar()
    sin = eval(:sin)
    sin(1)
end
foo() = bar()

julia> Compiler3.analyze_static(Tuple{typeof(foo)})
In function: bar()
ERROR: The called function was unknown
1| function bar()
2|     sin = eval(:sin)
=>     sin(1)
4| end

[1] In foo()

The reason this needs to sit on top of #35831 is that it needs to run
with a completely fresh cache, which #35831 provides the primitives for.
Otherwise, while you could still get the annotations out, that would
only work the first time something is inferred anywhere in the system.
With a fresh cache, everything is analyzed again, and any messages like
these that are opted in to can be collected.

cc @vchuravy for Cthulhu, @maleadt @jpsamaroo for GPU-related concerns

Keno · 2020-06-29T00:36:15Z

Can I go ahead with this? Not having it on master is causing me lots of rebase issues when I want to make patches against master from my branch.

vtjnash · 2020-06-29T02:22:55Z

base/compiler/typeinfer.jl

-        code_cache(interp)[result.linfo] = CodeInstance(result, min_valid, max_valid)
+        code_cache(interp)[result.linfo] = CodeInstance(result, min_valid, max_valid,
+            may_compress(interp), may_discard_trees(interp))


Seems like these got accidentally added from a different commit, but otherwise seems okay

Yeah, these are useful to have the remark emitting AbstractInterpreter force results to be kept such that it can correlate the remarks with the original source, but you're right, it's conceptually separate.

Ah, maybe just needs a separate commit then, or at least mention in the commit message, so we don't drop this later?

I'd also perhaps have preferred we just added these to the existing configuration set for this (InferenceParams), rather than creating a duplicate strategy with dispatch.

vtjnash · 2020-06-29T02:27:54Z

base/compiler/types.jl

+by the native interpreter, but can be used by external tooling to annotate
+inference results.
+"""
+mark_dynamic!(ni::NativeInterpreter, sv, s) = nothing


This naming seems rather poor, since there's not really a sense in which the places this got called is related to "when inference widens a result". The only place that happens is tmerge. Perhaps this should be called simply add_remark?

Fair enough. The idea here was that this answers the question "why did a dynamic dispatch get introduced here". I'm fine with just generically calling it add_remark though. Eventually we might want to have multiple classes of remarks, not just about inference precision, so that seems like a fine direction to go.

Yeah, but the places we're putting it (in inference) aren't introducing dynamic dispatch (that's always there), they're merely decision points where we prune our analysis of the call graph.

Depends on what you mean by dynamic dispatch ;). I meant calls to jl_apply_generic which is often what happens when we prune the analysis here. The reason to care about that is that that'd be what's disallowed in e.g. a GPU or XLA context. Anyway, add_remark! is fine.

Keno · 2020-06-29T03:20:57Z

Renamed to add_remark

vchuravy · 2020-06-29T18:12:31Z

base/compiler/abstractinterpretation.jl

@@ -327,6 +343,7 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
                # avoid widening when detecting self-recursion
                # TODO: merge call cycle and return right away
                if call_result_unused(sv)
+                    add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")


Suggested change

add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")

add_remark!(interp, sv, "Widening unused call result early to avoid unbounded self-recursion")

Would be good if the annotations are unique so that one can grep for them. Not sure if my recommendation is the best here.

As commented below, no widening is happening here (that's why we're okay with returning this early)

Suggested change

add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")

add_remark!(interp, sv, "Delaying call result analysis until after optimization due to bounded recursion")

Are we really delaying analysis, aren't we just skipping it entirely? When will we come back to do this analysis?

We're skipping this here because we know it's already being analyzed and we don't care about the result.

Ok, but we never go back to actually annotate that result, right? In that case, I'd like to inform the user that the return type they'd be seeing in Cthulhu may not match what inference knows about this callsite. Maybe something like: "Unbounded recursion detected with unused result. Annotated return type may be wider than true result".

This is bounded recursion (in the type domain), but yeah, we're putting 'Any' here specifically because we know it can't be used by the optimizer anyways, and thus want to avoid the re-work costs of putting the real type here later.

So "Unbounded recursion detected with unused result. Annotated return type may be wider than true result."? Or just leave off the remark entirely and have Cthulhu check when it seems a situation like this?

base/compiler/types.jl

base/compiler/abstractinterpretation.jl

vtjnash

Some alternative wording suggestions

vtjnash · 2020-06-29T19:52:51Z

base/compiler/abstractinterpretation.jl

-    isdefined(ftname, :mt) || return Any # not callable. should be Bottom, but can't track this backedge right now
+    if !isdefined(ftname, :mt)
+        # should be Bottom, but can't track this backedge right now
+        add_remark!(interp, sv, "The function is not callable")


Suggested change

add_remark!(interp, sv, "The function is not callable")

add_remark!(interp, sv, "Dispatch on this function type value is currently disallowed")

Alternatively, since it's probably not useful to attempt to maintain the distinction between these:

Suggested change

add_remark!(interp, sv, "The function is not callable")

add_remark!(interp, sv, "Could not identify method table for call")

I think it's worth having distinct messages for every call site, so people can grep the inference source and see what happened if necessary.

Yeah, but the only reason these are separate is that we're doing a bad job at reimplementing jl_method_table_for here. These reasons shouldn't actually be distinguishable.

Ok, fair enough. What's the reason we're not calling jl_method_table_for here? Or is there just not good reason?

Just got forgotten, I'm guessing

base/compiler/abstractinterpretation.jl

vtjnash · 2020-06-29T20:04:47Z

base/compiler/abstractinterpretation.jl

@@ -407,6 +424,7 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
            # continue inference, but note that we've limited parameter complexity
            # on this call (to ensure convergence), so that we don't cache this result
            if call_result_unused(sv)
+                add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")


This wording is misleading: there's never widening that happens here (except if the code-path would be discovered to stack overflow instead), since the result is known to be unused, though we also don't know what will happen next (though it's unlikely inlining would succeed regardless due to the graph cycle).

Suggested change

add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")

add_remark!(interp, sv, "Delaying call result analysis until after optimization due to unbounded recursion")

vtjnash · 2020-06-29T20:23:02Z

base/compiler/abstractinterpretation.jl

@@ -327,6 +343,7 @@ function abstract_call_method(interp::AbstractInterpreter, method::Method, @nosp
                # avoid widening when detecting self-recursion
                # TODO: merge call cycle and return right away
                if call_result_unused(sv)
+                    add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")


As commented below, no widening is happening here (that's why we're okay with returning this early)

Suggested change

add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")

add_remark!(interp, sv, "Delaying call result analysis until after optimization due to bounded recursion")

base/compiler/abstractinterpretation.jl

vtjnash · 2020-06-29T20:34:19Z

base/compiler/abstractinterpretation.jl

@@ -49,6 +57,7 @@ function abstract_call_gf_by_type(interp::AbstractInterpreter, @nospecialize(f),
        if !isa(tname, DataType)
            # can't track the backedge to the ctor right now
            # for things like Union
+            add_remark!(interp, sv, "The called constructor is too complicated")


Suggested change

add_remark!(interp, sv, "The called constructor is too complicated")

add_remark!(interp, sv, "Deciding to skip computing the result due to arbitrary reasons")

I have no clue what this ftname === _TYPE_NAME condition means (from 0b8f0b7), but looks really arbitrarily random these days to return from here.

Are you saying we should just remove this whole code path? The suggested message seems like it could cause frustration for people seeing it.

Yeah, I couldn't tell you why these specific conditions are so bad. "Too complicated" seemed like it was trying to be too PC and cause the same frustrations.

I'll put up a PR to remove it and see what breaks :)

Keno · 2020-06-30T00:34:31Z

I've applied most of the suggestions and asked two questions in others (will probably still apply those, but I'd like to understand better).

`@vtjnash` said in #35845 that he didn't know what this code was for and didn't think it was necessary. Since he's the one that added it and the comment says it's about backedges, I'm inclined to believe him :). In any case, removing it passes base tests, so let's try it.

In #35845, `@vtjnash` was saying that this function tried to re-implement jl_method_table_for, but badly. Try to just use it directly and see if anything breaks (Base tests seem to pass).

`@vtjnash` said in #35845 that he didn't know what this code was for and didn't think it was necessary. Since he's the one that added it and the comment says it's about backedges, I'm inclined to believe him :). In any case, removing it passes base tests, so let's try it.

In #35845, `@vtjnash` was saying that this function tried to re-implement jl_method_table_for, but badly. Try to just use it directly and see if anything breaks (Base tests seem to pass).

As well as turning off optimizations. This is useful for Cthulhu to be able to request all unoptimized, noncompressed CodeInfos and correlate them with inference remarks (see next commit).

This builds on top of #35831, letting inference emit a custom message whenever it gives up on infering something. These messages are intended to be displayed by external tools, either to debug what inference is doing (e.g. for Cthulhu) or, if an external compiler needs to disallow certain kinds of missing information (e.g. no dynamic dispatch on GPUs), can be used to improve diagnostics. This is mostly a proof of concept, I think these messages/hooks need to be a bit richer for a full solution, though I think this can be already useful if we hook up something like Cthulhu. As a proof of concept, I hacked up a 10 line function that reads these messagse. It works something like the following: ``` function bar() sin = eval(:sin) sin(1) end foo() = bar() ``` ``` julia> Compiler3.analyze_static(Tuple{typeof(foo)}) In function: bar() ERROR: The called function was unknown 1| function bar() 2| sin = eval(:sin) => sin(1) 4| end [1] In foo() ``` The reason this needs to sit on top of #35831 is that it needs to run with a completely fresh cache, which #35831 provides the primitives for. Otherwise, while you could still get the annotations out, that would only work the first time something is inferred anywhere in the system. With a fresh cache, everything is analyzed again, and any messages like these that are opted in to can be collected.

Keno · 2020-07-01T00:18:37Z

Alright, rebased on top of #36477 and split out the may_optimize stuff into a separate commit. Also adjusted the recursion message as discussed. Will plan to merge once #36477 is in.

`@vtjnash` said in JuliaLang#35845 that he didn't know what this code was for and didn't think it was necessary. Since he's the one that added it and the comment says it's about backedges, I'm inclined to believe him :). In any case, removing it passes base tests, so let's try it.

In JuliaLang#35845, `@vtjnash` was saying that this function tried to re-implement jl_method_table_for, but badly. Try to just use it directly and see if anything breaks (Base tests seem to pass).

Keno force-pushed the kf/cache_refactor branch from 33aa49a to c79044c Compare May 12, 2020 02:54

Base automatically changed from kf/cache_refactor to master June 9, 2020 03:10

Keno force-pushed the kf/static_errors branch from 0328dca to 57e5b5e Compare June 9, 2020 17:58

Keno changed the title ~~WIP: Inference annotations that can be read by an external tool~~ Inference annotations that can be read by an external tool Jun 9, 2020

Keno force-pushed the kf/static_errors branch from 57e5b5e to 01defb1 Compare June 9, 2020 18:22

Keno requested review from JeffBezanson, vtjnash and vchuravy and removed request for JeffBezanson June 29, 2020 00:36

vtjnash reviewed Jun 29, 2020

View reviewed changes

Keno force-pushed the kf/static_errors branch from 01defb1 to 13b7bb1 Compare June 29, 2020 03:19

vchuravy approved these changes Jun 29, 2020

View reviewed changes

vtjnash reviewed Jun 29, 2020

View reviewed changes

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

vtjnash reviewed Jun 29, 2020

View reviewed changes

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

vtjnash reviewed Jun 29, 2020

View reviewed changes

Keno mentioned this pull request Jun 30, 2020

Remove outdated restriction on Type{} calls in abstract interpretation #36477

Merged

Keno force-pushed the kf/static_errors branch from f82f5ba to b69f06e Compare July 1, 2020 00:15

Keno changed the base branch from master to kf/remwat July 1, 2020 00:16

Keno added 2 commits June 30, 2020 20:17

Allow AbstractInterpreters to request preserving trees

4bc394d

As well as turning off optimizations. This is useful for Cthulhu to be able to request all unoptimized, noncompressed CodeInfos and correlate them with inference remarks (see next commit).

Keno force-pushed the kf/static_errors branch from b69f06e to e0ed273 Compare July 1, 2020 00:17

Base automatically changed from kf/remwat to master July 1, 2020 18:57

Keno merged commit d5cf73f into master Jul 1, 2020

Keno deleted the kf/static_errors branch July 1, 2020 20:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Inference annotations that can be read by an external tool #35845

Inference annotations that can be read by an external tool #35845

Keno commented May 12, 2020

Keno commented Jun 29, 2020

vtjnash Jun 29, 2020

Keno Jun 29, 2020

vtjnash Jun 29, 2020

vtjnash Jun 29, 2020

Keno Jun 29, 2020

vtjnash Jun 30, 2020

Keno Jun 30, 2020

Keno commented Jun 29, 2020

vchuravy Jun 29, 2020

vtjnash Jun 29, 2020

Keno Jun 30, 2020

vtjnash Jun 30, 2020

Keno Jun 30, 2020

vtjnash Jun 30, 2020

Keno Jun 30, 2020

vtjnash left a comment

vtjnash Jun 29, 2020

vtjnash Jun 29, 2020

Keno Jun 30, 2020

vtjnash Jun 30, 2020

Keno Jun 30, 2020

vtjnash Jun 30, 2020

vtjnash Jun 29, 2020

vtjnash Jun 29, 2020

vtjnash Jun 29, 2020

Keno Jun 30, 2020

vtjnash Jun 30, 2020

Keno Jun 30, 2020

Keno commented Jun 30, 2020

Keno commented Jul 1, 2020

	add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")
	add_remark!(interp, sv, "Widening unused call result early to avoid unbounded self-recursion")

	add_remark!(interp, sv, "Widening unused call result early to avoid unbounded recursion")
	add_remark!(interp, sv, "Delaying call result analysis until after optimization due to bounded recursion")

	add_remark!(interp, sv, "The function is not callable")
	add_remark!(interp, sv, "Dispatch on this function type value is currently disallowed")

	add_remark!(interp, sv, "The function is not callable")
	add_remark!(interp, sv, "Could not identify method table for call")

	add_remark!(interp, sv, "The called constructor is too complicated")
	add_remark!(interp, sv, "Deciding to skip computing the result due to arbitrary reasons")

Inference annotations that can be read by an external tool #35845

Inference annotations that can be read by an external tool #35845

Conversation

Keno commented May 12, 2020

Keno commented Jun 29, 2020

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 Jun 29, 2020

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

vtjnash left a 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Keno commented Jun 30, 2020

Keno commented Jul 1, 2020