Add `PartialOpaque` lattice element for OpaqueClosure #39512

Keno · 2021-02-03T23:45:38Z

This adds a lattice element for tracking OpaqueClosures in inference,
but does not yet do anything with it. The reason I'm separating
this out is that just the introduction of the lattice element
raises some tricky issues. In particular, the lattice element
refers back to the OpaqueClosure method, which we currently
don't support in the serializer. I played with several ways
of adding support for that, but in the end it all ended up
super complicated for questionable benefit, so in this PR,
CodeInstances that get inferred to PartialOpaque get
omitted during serialization (i.e. they will be reinfered
upon loading the .ji).

base/compiler/abstractinterpretation.jl

base/compiler/typelattice.jl

vtjnash · 2021-02-04T19:31:37Z

base/compiler/typelattice.jl

+            return (widenconst(a) <: widenconst(b)) &&
+                ⊑(a.env, b.env)
+        end
+        return widenconst(a) <: widenconst(b)


Suggested change

return widenconst(a) <: widenconst(b)

return widenconst(a) ⊑ b

vtjnash · 2021-02-04T19:33:01Z

base/compiler/utilities.jl

+        if isdefined(linfo, :uninferred)
+            c = copy(linfo.uninferred::CodeInfo)
+        else
+            # user code might throw errors – ignore them
+            c = get_staged(linfo)
+            # For opaque closures, cache the generated code info to make sure


Since this is a correctness issue, and there's many places it gets called, get_staged must be handling this itself

I figured it was only a correctness issue if it escaped into inference, which should always happen here. Doing the copy in C is kinda annoying, but I guess I'll just move it there.

Actually, I guess I can just move this into the get_staged julia-side function, which avoids duplicating the copy logic on the C side. The other callers from C just use it to generate code directly, which doesn't have the correctness issues.

vtjnash · 2021-02-04T19:37:58Z

src/dump.c

+        jl_serialize_value(s, NULL);
+        jl_serialize_value(s, jl_any_type);


These are part of the ABI of the code pointers also in this struct. Dropping this means we also need to clear the jl_fptr_const_return flag.

This part isn't changed - it skips the entire codeinstance if the return type is PartialOpaque.

Ah, that would make more sense. Looks like you're missing a return above to actually implement that?

vtjnash · 2021-02-04T19:40:41Z

src/dump.c

@@ -2422,6 +2451,7 @@ static jl_method_t *jl_lookup_method(jl_methtable_t *mt, jl_datatype_t *sig, siz

 static jl_method_t *jl_recache_method(jl_method_t *m)
 {
+    assert(!m->is_for_opaque_closure);


Seems like this assert isn't handled on serialization? (there's nothing stopping the user from referencing this from other places via reflection or perhaps .roots from generated code)

I'll add an error case if you try to serialize it.

vtjnash · 2021-02-04T19:46:46Z

base/compiler/abstractinterpretation.jl

    else
        # non-constant function, but the number of arguments is known
        # and the ft is not a Builtin or IntrinsicFunction
-        if typeintersect(widenconst(ft), Builtin) != Union{}
+        if typeintersect(widenconst(ft), Union{Builtin, Core.OpaqueClosure}) != Union{}


Seems like we should consider making it a Builtin?

Potentially? It's kinda weird to call it a builtin though. Maybe add something above builtin?

I think it is perhaps accurate enough calling it a Builtin—there is no source code since that is in C and uses a JL_CALLABLE handle for the initial invoke, and there's otherwise about a dozen spots in compiler similar to this one that we need to audit. OTOH, in C we assume that Builtin must have exactly one entry in the method table cache (which will be false for the OpaqueClosure), so perhaps we need to audit each anyways.

Do we still need to audit these, or did you already look through the other places where we treat it specially?

I left this as is for the moment. It still seems wrong to be to call it a Builtin. We could perhaps insert a new abstract type above Builtin though.

I don't think that is necessarily useful. We might want to make an alias name for this Union in Core.Compiler, but currently this is the only usage site. I was inquiring instead whether any of the other places that we refer to Builtin though need to use this Union instead.

I looked through those and I didn't think any needed to be adjusted.

Ah, good. Thanks for confirming

JeffBezanson · 2021-02-05T22:26:14Z

base/compiler/abstractinterpretation.jl

+        rt = unwrapva(params[max(i-1, length(params))])
+        if closure.isva
+            if length(params) > i-1
+                for j = (i):length(params)


Suggested change

for j = (i):length(params)

for j = (i):length(params)-1

Am I missing something? I thought length(params) was correct.

JeffBezanson · 2021-02-05T22:30:35Z

src/dump.c

@@ -380,11 +380,11 @@ static void jl_serialize_module(jl_serializer_state *s, jl_module_t *m)
    write_uint8(s->s, m->infer);
 }

-static void jl_serialize_value_(jl_serializer_state *s, jl_value_t *v, int as_literal) JL_GC_DISABLED
+static inline int jl_serialize_generic(jl_serializer_state *s, jl_value_t *v) JL_GC_DISABLED


Why is this declared inline?

Also could use a comment saying what the return value means.

It grew after I added the inline annotation. It's probably too big now to be inline

vtjnash · 2021-02-10T04:19:25Z

base/compiler/utilities.jl

+        if has_opaque_closure(mi)
+            # For opaque closures, cache the generated code info to make sure
+            # that OpaqueClosure method identity is stable
+            mi.uninferred = copy(ci)


We already have code to do this in C (jl_copy_ast). Let's keep it there, since this would be unsound otherwise.

vtjnash · 2021-02-10T19:34:40Z

base/compiler/abstractinterpretation.jl

+    @assert isa(argt, DataType) && argt.name === typename(Tuple)
+    params = argt.parameters
+    for i = 2:closure.source.nargs
+        rt = unwrapva(params[max(i-1, length(params))])


whoops, thanks.

vtjnash · 2021-02-10T19:35:15Z

base/compiler/abstractinterpretation.jl

+    ret = Any[]
+    cc = widenconst(closure)
+    argt = unwrap_unionall(cc).parameters[1]
+    @assert isa(argt, DataType) && argt.name === typename(Tuple)


Is the idea we'll refuse to form the PartialOpaque if this condition doesn't hold?

I guess we could just do [Vararg{Any}] otherwise. Let me do that.

vtjnash · 2021-02-10T19:41:16Z

base/compiler/abstractinterpretation.jl

+    return CallMeta(Any, nothing)
+end
+
+function most_general_argtypes(closure::PartialOpaque)


This seems like incomplete copy of matching_cache_argtypes. Can we use that instead (refactoring as needed)?

vtjnash · 2021-02-10T21:33:03Z

base/compiler/inferenceresult.jl

@@ -3,7 +3,7 @@
 function is_argtype_match(@nospecialize(given_argtype),
                          @nospecialize(cache_argtype),
                          overridden_by_const::Bool)
-    if isa(given_argtype, Const) || isa(given_argtype, PartialStruct)
+    if isa(given_argtype, Const) || isa(given_argtype, PartialStruct) || isa(given_argtype, PartialOpaque)


Why would a PartialOpaque ever be equal with OpaqueClosure?

You're right, it shouldn't be

vtjnash · 2021-02-10T21:34:48Z

base/compiler/tfuncs.jl

+function _opaque_closure_tfunc(@nospecialize(arg), @nospecialize(isva),
+        @nospecialize(lb), @nospecialize(ub), @nospecialize(source), env::Vector{Any},
+        linfo::MethodInstance)


Alternatively, this could be written more compactly as. Thoughts?

Suggested change

function _opaque_closure_tfunc(@nospecialize(arg), @nospecialize(isva),

@nospecialize(lb), @nospecialize(ub), @nospecialize(source), env::Vector{Any},

linfo::MethodInstance)

function _opaque_closure_tfunc(arg::Any, isva::Any, lb::Any, ub::Any, source::Any,

env::Vector{Any}, linfo::MethodInstance)

@nospecialize

@nospecialized is declared at the top of this file, so I guess we even don't need these annotations in tfuncs.jl ?

I like the explicit nospecialize annotations. Otherwise I always forget to put them back

base/compiler/tfuncs.jl

vtjnash · 2021-02-10T21:46:44Z

base/compiler/typelattice.jl

+        a.source == b.source || return false
+        a.parent == b.parent || return false


Suggested change

a.source == b.source || return false

a.parent == b.parent || return false

a.source === b.source || return false

a.parent === b.parent || return false

vtjnash · 2021-02-10T21:48:40Z

src/dump.c

+    return 0;
+}
+
+static void jl_serialize_code_instance(jl_serializer_state *s, jl_code_instance_t *codeinst, int skip_partial_opaque) JL_GC_DISABLED


Could you add some comments to this function? It is a bit more complex now.

vtjnash · 2021-02-10T21:48:53Z

src/dump.c

+            jl_typeis(codeinst->rettype_const, jl_partial_opaque_type)) {
+        if (skip_partial_opaque) {
+            jl_serialize_code_instance(s, codeinst->next, skip_partial_opaque);
+        } else {


style nit:

Suggested change

} else {

}

else {

vtjnash · 2021-02-10T21:58:46Z

src/dump.c

+
+    int write_ret_type = validate || codeinst->min_world == 0;
+    if (write_ret_type && codeinst->rettype_const &&
+            jl_typeis(codeinst->rettype_const, jl_partial_opaque_type)) {


If this happens, I think we should also see this same rettype_const object in Method.roots and possibly elsewhere, from when we compressed slottypes? We might want to improve that eventually, but seems like we need to just make this valid to store? What's inhibiting that currently?

The PartialOpaque will have a reference to the Method object that we don't have a good identity for.

vtjnash · 2021-02-10T22:00:18Z

src/dump.c

+        if (jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure) {
+            jl_error("Cannot serialize MethodInstances for OpaqueClosure");
+        }


Seems like this means we also would be prohibited from inferring with them (since that would add backedges to them). Can we fix this?

Suggested change

if (jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure) {

jl_error("Cannot serialize MethodInstances for OpaqueClosure");

}

if (jl_is_method(mi->def.value) && mi->def.method->is_for_opaque_closure) {

jl_error("unimplemented: serialization of MethodInstances for OpaqueClosure");

}

vtjnash · 2021-02-10T22:01:59Z

src/jltypes.c

+    jl_partial_opaque_type = jl_new_datatype(jl_symbol("PartialOpaque"), core, jl_any_type, jl_emptysvec,
+                                       jl_perm_symsvec(5, "typ", "env", "isva", "parent", "source"),
+                                       jl_svec(5, jl_type_type, jl_any_type, jl_bool_type, jl_method_instance_type, jl_method_type), 0, 0, 5);


style nit:

Suggested change

jl_partial_opaque_type = jl_new_datatype(jl_symbol("PartialOpaque"), core, jl_any_type, jl_emptysvec,

jl_perm_symsvec(5, "typ", "env", "isva", "parent", "source"),

jl_svec(5, jl_type_type, jl_any_type, jl_bool_type, jl_method_instance_type, jl_method_type), 0, 0, 5);

jl_partial_opaque_type = jl_new_datatype(jl_symbol("PartialOpaque"), core, jl_any_type, jl_emptysvec,

jl_perm_symsvec(5, "typ", "env", "isva", "parent", "source"),

jl_svec(5, jl_type_type, jl_any_type, jl_bool_type, jl_method_instance_type, jl_method_type),

0, 0, 5);

vtjnash

A few comments inline. Do we want to also implement tmerge now? The fallback will already be correct (it falls back to tmerge on widenconst), but seems suboptimal?

This adds a lattice element for tracking OpaqueClosures in inference, but does not yet do anything with it. The reason I'm separating this out is that just the introduction of the lattice element raises some tricky issues. In particular, the lattice element refers back to the OpaqueClosure method, which we currently don't support in the serializer. I played with several ways of adding support for that, but in the end it all ended up super complicated for questionable benefit, so in this PR, CodeInstances that get inferred to `PartialOpaque` get omitted during serialization (i.e. they will be reinfered upon loading the .ji).

Keno · 2021-02-15T17:02:59Z

Alright, I think this is in a decent state to merge. There's much still to be done for inference/optimizations, so we can include any further tweaks in upcoming rounds.

vtjnash · 2021-02-15T17:46:11Z

base/compiler/utilities.jl

+function has_opaque_closure(c::CodeInfo)
+    for i = 1:length(c.code)
+        stmt = c.code[i]
+        (isa(stmt, Expr) && stmt.head === :new_opaque_closure) && return true
+    end
+    return false
+end


dead code

Suggested change

function has_opaque_closure(c::CodeInfo)

for i = 1:length(c.code)

stmt = c.code[i]

(isa(stmt, Expr) && stmt.head === :new_opaque_closure) && return true

end

return false

end

vtjnash · 2021-02-15T17:54:05Z

Glad to see this in! I wish you had merged #39606 first though, since that might need to be backported and has some minor conflicts with this (and one fix needed to the lattice here, for widenreturn, is in that PR).

Keno · 2021-02-15T18:05:51Z

I don't think #39606 is backportable as a whole anyway, since it touches 1.7 only code. I didn't think we had fully discussed the parameterization issue there yet, or has that been worked out (let's talk about that over there).

This adds a lattice element for tracking OpaqueClosures in inference, but does not yet do anything with it. The reason I'm separating this out is that just the introduction of the lattice element raises some tricky issues. In particular, the lattice element refers back to the OpaqueClosure method, which we currently don't support in the serializer. I played with several ways of adding support for that, but in the end it all ended up super complicated for questionable benefit, so in this PR, CodeInstances that get inferred to `PartialOpaque` get omitted during serialization (i.e. they will be reinfered upon loading the .ji).

Keno requested review from vtjnash and JeffBezanson February 3, 2021 23:45

Keno force-pushed the kf/partialopaque branch from 7ddb2be to d2ef8d0 Compare February 4, 2021 00:07

aviatesk reviewed Feb 4, 2021

View reviewed changes

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

base/compiler/typelattice.jl Show resolved Hide resolved

Keno force-pushed the kf/partialopaque branch from d2ef8d0 to 33929ec Compare February 4, 2021 19:13

vtjnash reviewed Feb 4, 2021

View reviewed changes

JeffBezanson reviewed Feb 5, 2021

View reviewed changes

JeffBezanson approved these changes Feb 5, 2021

View reviewed changes

Keno force-pushed the kf/partialopaque branch 2 times, most recently from cc4bb90 to 310ac38 Compare February 10, 2021 03:52

vtjnash reviewed Feb 10, 2021

View reviewed changes

base/compiler/tfuncs.jl Show resolved Hide resolved

vtjnash reviewed Feb 10, 2021

View reviewed changes

vtjnash mentioned this pull request Feb 10, 2021

Fixes some inference issues #39606

Merged

Keno added 5 commits February 12, 2021 20:27

Address two review comments

ce1e6f1

Some corrections

0657414

Move some code - fix some typos

5a240af

Address review

b4cdce7

Keno force-pushed the kf/partialopaque branch from 310ac38 to b4cdce7 Compare February 14, 2021 03:47

Keno merged commit 88c0e25 into master Feb 15, 2021

Keno deleted the kf/partialopaque branch February 15, 2021 17:03

vtjnash reviewed Feb 15, 2021

View reviewed changes

aviatesk mentioned this pull request Feb 16, 2021

inference: inter-procedural conditional constraint back-propagation #38905

Merged

maleadt mentioned this pull request Aug 17, 2022

Fix serialization of code instances. #46373

Merged

	return widenconst(a) <: widenconst(b)
	return widenconst(a) ⊑ b

		jl_serialize_value(s, NULL);
		jl_serialize_value(s, jl_any_type);

		a.source == b.source \|\| return false
		a.parent == b.parent \|\| return false

Add PartialOpaque lattice element for OpaqueClosure #39512

Add PartialOpaque lattice element for OpaqueClosure #39512

Conversation

Keno commented Feb 3, 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

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

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

Choose a reason for hiding this comment

vtjnash Feb 10, 2021 • edited Loading

Choose a reason for hiding this comment

vtjnash left a comment

Choose a reason for hiding this comment

Keno commented Feb 15, 2021

Choose a reason for hiding this comment

vtjnash commented Feb 15, 2021

Keno commented Feb 15, 2021

Add `PartialOpaque` lattice element for OpaqueClosure #39512

Add `PartialOpaque` lattice element for OpaqueClosure #39512

vtjnash Feb 10, 2021 •

edited

Loading