-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
inference: concretize invoke
callsite correctly
#46743
Conversation
d5bf1e4
to
38d051d
Compare
@@ -885,6 +885,7 @@ function abstract_call_method_with_const_args(interp::AbstractInterpreter, resul | |||
add_backedge!(res.const_result.mi, sv, invoketypes) | |||
return res | |||
end | |||
f isa InvokeCall && (f = f.f) # unwrap `invoke`-ed function (the call will be virtualized) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't usually like wrapper objects in non-escaped contexts. E.g. one could probably end up with an InvokeCall
here, by doing something like const foo = CC.InvokeCall(...)
and then calling foo, but I suppose that wouldn't actually be a problem here, since that would still give the right answer, so I suppose it's not a problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay, I stopped passing the wrapper object through the f
argument, but rather pass the invoke
information as an additional argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, I like that better
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like that better too
It turns out that previously we didn't concretize `invoke` callsite correctly, as we didn't take into account whether or not the call being concretized is `invoke`-d or not, e.g.: ``` julia> invoke_concretized2(a::Int) = a > 0 ? :int : nothing invoke_concretized2 (generic function with 1 method) julia> invoke_concretized2(a::Integer) = a > 0 ? :integer : nothing invoke_concretized2 (generic function with 2 methods) julia> let Base.Experimental.@force_compile Base.@invoke invoke_concretized2(42::Integer) end :int # this should return `:integer` instead ``` This commit fixes that up by propagating information `invoke`-d callsite to `concrete_eval_call`. Now we should pass the following test cases: ```julia invoke_concretized1(a::Int) = a > 0 ? :int : nothing invoke_concretized1(a::Integer) = a > 0 ? "integer" : nothing @test Base.infer_effects((Int,)) do a @invoke invoke_concretized1(a::Integer) end |> Core.Compiler.is_foldable @test Base.return_types() do @invoke invoke_concretized1(42::Integer) end |> only === String invoke_concretized2(a::Int) = a > 0 ? :int : nothing invoke_concretized2(a::Integer) = a > 0 ? :integer : nothing @test Base.infer_effects((Int,)) do a @invoke invoke_concretized2(a::Integer) end |> Core.Compiler.is_foldable @test let Base.Experimental.@force_compile @invoke invoke_concretized2(42::Integer) end === :integer ```
38d051d
to
61ba571
Compare
The whitespace failure is fixed on master. |
It turns out that previously we didn't concretize `invoke` callsite correctly, as we didn't take into account whether the call being concretized is `invoke`-d or not, e.g.: ``` julia> invoke_concretized2(a::Int) = a > 0 ? :int : nothing invoke_concretized2 (generic function with 1 method) julia> invoke_concretized2(a::Integer) = a > 0 ? :integer : nothing invoke_concretized2 (generic function with 2 methods) julia> let Base.Experimental.@force_compile Base.@invoke invoke_concretized2(42::Integer) end :int # this should return `:integer` instead ``` This commit fixes that up by propagating information `invoke`-d callsite to `concrete_eval_call`. Now we should be able to pass the following test cases: ```julia invoke_concretized1(a::Int) = a > 0 ? :int : nothing invoke_concretized1(a::Integer) = a > 0 ? "integer" : nothing @test Base.infer_effects((Int,)) do a @invoke invoke_concretized1(a::Integer) end |> Core.Compiler.is_foldable @test Base.return_types() do @invoke invoke_concretized1(42::Integer) end |> only === String invoke_concretized2(a::Int) = a > 0 ? :int : nothing invoke_concretized2(a::Integer) = a > 0 ? :integer : nothing @test Base.infer_effects((Int,)) do a @invoke invoke_concretized2(a::Integer) end |> Core.Compiler.is_foldable @test let Base.Experimental.@force_compile @invoke invoke_concretized2(42::Integer) end === :integer ```
It turns out that previously we didn't concretize
invoke
callsite correctly, as we didn't take into account whether or not the call being concretized isinvoke
-d or not, e.g.:This commit fixes that up by propagating information
invoke
-d callsite toconcrete_eval_call
. Now we should pass the following test cases: