Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix inference with const opaque closure #42725

Merged
merged 8 commits into from
Oct 30, 2021
Merged

Conversation

tkf
Copy link
Member

@tkf tkf commented Oct 21, 2021

Currently, using constant opaque closure (e.g., a global const or @benchmark #40409) introduces an illegal instruction error. MWE:

const oc = Base.Experimental.@opaque () -> 0
f() = oc()
f()

This is because of the incorrect inference:

julia> @code_typed optimize=false f()
CodeInfo(
1 ─     Main.oc()::Union{}
└──     Core.Const(:(return %1))::Union{}
) => Union{}

This seems to be simply due to a missing widenconst in inference.

fix #40409

@tkf tkf requested review from aviatesk and Keno October 21, 2021 01:55
@tkf tkf added backport 1.7 bugfix This change fixes an existing bug labels Oct 21, 2021
@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

If this fixes it, it seems like there is also a deeper underlying bug too that must be fixed also.

Copy link
Member

@aviatesk aviatesk left a comment

Choose a reason for hiding this comment

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

LGTM, since this pass is supposed to handle a case when only the data type of an opaque closure is known, and we should use widenconst when working on Type objects.

@oscardssmith
Copy link
Member

What is the tester_linuxaarch64 failure? It definitely seems unrelated, but maybe worth rebasing?

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

Yes, but it is also unacceptable for it to return an incorrect answer just because this case couldn't be optimized

@tkf
Copy link
Member Author

tkf commented Oct 21, 2021

Do you mean we need to fix something between the inference and codegen? But it looks like a correct codegen to me given the inference Main.oc()::Union{}? In Julia 1.7, I get

julia> @code_llvm f()
;  @ REPL[2]:1 within `f`
; Function Attrs: noreturn
define nonnull {}* @julia_f_184() #0 {
top:
  %0 = call nonnull {}* @jl_apply_generic({}* inttoptr (i64 140356399876112 to {}*), {}** null, i32 0)
  call void @llvm.trap()
  unreachable
}

@tkf tkf requested a review from vtjnash October 21, 2021 18:59
@tkf
Copy link
Member Author

tkf commented Oct 21, 2021

Do we need other actions before merging this? Or should we close this PR and wait for a real fix?

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

Yes, Main.oc()::Union{} is wrong from inference

@tkf
Copy link
Member Author

tkf commented Oct 21, 2021

Hmm... I'm a bit confused. This PR fixes the inference Main.oc()::Union{} to Main.oc()::Any, right? (or Int once @opaque starts filling the return type parameter.)

Do you mean we need to fix this in abstract_call_known?

@vtjnash
Copy link
Member

vtjnash commented Oct 21, 2021

It is supposed to be handled by the check on line 1378, for example, but it isn't reaching there apparently.

@tkf
Copy link
Member Author

tkf commented Oct 21, 2021

Quoting the code in question:

f = singleton_type(ft)
if isa(ft, PartialOpaque)
return abstract_call_opaque_closure(interp, ft, argtypes[2:end], sv)
elseif (uft = unwrap_unionall(widenconst(ft)); isa(uft, DataType) && uft.name === typename(Core.OpaqueClosure))
return CallMeta(rewrap_unionall((uft::DataType).parameters[2], ft), false)
elseif f === nothing
# non-constant function, but the number of arguments is known
# and the ft is not a Builtin or IntrinsicFunction
if typeintersect(widenconst(ft), Union{Builtin, Core.OpaqueClosure}) != Union{}
add_remark!(interp, sv, "Could not identify method table for call")
return CallMeta(Any, false)
end
return abstract_call_gf_by_type(interp, nothing, fargs, argtypes, argtypes_to_type(argtypes), sv, max_methods)
end
return abstract_call_known(interp, f, fargs, argtypes, sv, max_methods)

But we don't hit 1378 since singleton_type(ft) === Main.oc !== nothing, right? Main.oc is not a singleton in the sense of issingletontype. But from the definition of singleton_type, it seems intentional that it behaves this way (unwraps Const)

function singleton_type(@nospecialize(ft))
if isa(ft, Const)
return ft.val
elseif isconstType(ft)
return ft.parameters[1]
elseif ft isa DataType && isdefined(ft, :instance)
return ft.instance
end
return nothing
end

(TBH, singleton_type is a confusing name for me maybe because I'm not familiar with the inference code. It sounds like known_value or const_value seems to reflect what it returns. It also seems to be tricky to use for cases where you could have Cons(nothing).)

@aviatesk
Copy link
Member

When oc::OpaqueClosure is wrapped in Const, f === oc and won't hit the line 1378. And then we can't find any method table in abstract_call_gf_by_type so the return type had to be annotated as Bottom.
Since we can't form PartialOpaque in such circumstances, the line 1373 should handle that case instead.

TBH, singleton_type is a confusing name

Yeah, at this moment all the usages of singleton_type(x) are supposed to do what could be called as argtype_to_function(x). So a case like singleton_type(Const(nothing)) isn't a problem at this moment, but I agree with renaming it to avoid wrong uages in the future.

@JeffBezanson JeffBezanson added the compiler:inference Type inference label Oct 22, 2021
@@ -1370,7 +1370,7 @@ function abstract_call(interp::AbstractInterpreter, fargs::Union{Nothing,Vector{
f = singleton_type(ft)
if isa(ft, PartialOpaque)
return abstract_call_opaque_closure(interp, ft, argtypes[2:end], sv)
elseif (uft = unwrap_unionall(ft); isa(uft, DataType) && uft.name === typename(Core.OpaqueClosure))
elseif (uft = unwrap_unionall(widenconst(ft)); isa(uft, DataType) && uft.name === typename(Core.OpaqueClosure))
Copy link
Member

Choose a reason for hiding this comment

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

Maybe not directly related, but this kind of structural-only check causes a non-monotonicity:

julia> f() = Union{OC{Tuple{},Int8},OC{Tuple{},Int16}}[][1]()
julia> @code_typed f()
...
) => Any

julia> f() = OC{Tuple{},<:Integer}[][1]()
julia> @code_typed f()
...
) => Integer

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it exist before this patch but I guess it'd be nice to fix it? I don't know what Keno's intention was, though.

What is the standard way to implement this? Would something like

elseif typeintersect(widenconst(ft), OpaqueClosure) <: OpaqueClosure

work? But I guess it still doesn't support Union{OC{Tuple{},Int8},Returns{Int16}}?

I can open a follow-up PR to fix this once I understand how to do it.

Copy link
Member

Choose a reason for hiding this comment

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

Looked into this closer, and I found this appear to be the other missing piece. This PR makes this unreachable by-and-large, but still the intended behavior of each function to do.

diff --git a/base/compiler/abstractinterpretation.jl b/base/compiler/abstractinterpretation.jl
index ff674f2d91..83cc37cb50 100644
--- a/base/compiler/abstractinterpretation.jl
+++ b/base/compiler/abstractinterpretation.jl
@@ -1279,6 +1279,9 @@ function abstract_call_known(interp::AbstractInterpreter, @nospecialize(f),
             return abstract_modifyfield!(interp, argtypes, sv)
         end
         return CallMeta(abstract_call_builtin(interp, f, arginfo, sv, max_methods), false)
+    elseif isa(f, OpaqueClosure)
+        # calling an OpaqueClosure about which we have no information returns no information
+        return CallMeta(Any, false)
     elseif f === Core.kwfunc
         if la == 2
             ft = widenconst(argtypes[2])

tkf and others added 3 commits October 25, 2021 19:41
This reverts the bugfix part of commit 07f06d5
but keeps the test.
Co-authored-by: Jameson Nash <vtjnash@gmail.com>
@tkf
Copy link
Member Author

tkf commented Oct 26, 2021

@vtjnash Thanks, I was wondering if abstract_call_known was the better place to fix it. I swapped the patch to your implementation #42725 (comment)

Is it good to go now?

Copy link
Member

@vtjnash vtjnash left a comment

Choose a reason for hiding this comment

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

actual correctness requires both patches

@aviatesk aviatesk added the merge me PR is reviewed. Merge when all tests are passing label Oct 29, 2021
@tkf
Copy link
Member Author

tkf commented Oct 29, 2021

/buildkite rerun failed

@aviatesk
Copy link
Member

The failures seem unrelated. Maybe we can fix them by rebasing ?

@DilumAluthge
Copy link
Member

@tkf Can you rebase this on the latest master? That should fix the Downloads test failures.

@DilumAluthge
Copy link
Member

I am eventually going to implement a new Buildkite command that will automatically rerun the entire Buildkite job with the updated merge commit. Once I've done that, you won't need to rebase; you'll be able to run this new command. But it'll be a while before that is ready.

@tkf
Copy link
Member Author

tkf commented Oct 29, 2021

I'm pretty sure this PR will be squash-merged. So it doesn't have to be rebased, I suppose? It's good to leave edit and CI history in the PR, which will be hard to reconstruct with rebase.

@aviatesk aviatesk merged commit 60e3e55 into JuliaLang:master Oct 30, 2021
@DilumAluthge DilumAluthge removed the merge me PR is reviewed. Merge when all tests are passing label Oct 30, 2021
@tkf tkf deleted the oc-const branch November 10, 2021 21:22
@KristofferC
Copy link
Member

Needs a manual backport.

@aviatesk
Copy link
Member

done backport

LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Feb 22, 2022
LilithHafner pushed a commit to LilithHafner/julia that referenced this pull request Mar 8, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix This change fixes an existing bug compiler:inference Type inference
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Illegal instruction interpolating an opaque closure into at-btime
7 participants