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 cfg_simplify! for empty preds and succs #41693

Closed
wants to merge 1 commit into from

Conversation

tkf
Copy link
Member

@tkf tkf commented Jul 24, 2021

Before this patch, cfg_simplify! relied on the conversion for handling empty preds and succs

julia> Vector{Int}(Union{}[])
Int64[]

which is not available inside the compiler:

julia> Base.invoke_in_world(UInt(4526), Vector{Int}, Union{}[])
ERROR: MethodError: no method matching Vector{Int64}(::Vector{Union{}})
The applicable method may be too new: running in world age 4526, while current world is 31202.

(I get 4526 via gdb)

This PR fixes it and also calls cfg_simplify! in the compiler's world age during the test. I think it's also good for improving inferrability. This is extracted from #39773.

Comment on lines +140 to +141
const _COMPILER_WORLD_AGE = ccall(:jl_get_tls_world_age, UInt, ())
compiler_world_age() = _COMPILER_WORLD_AGE
Copy link
Member Author

Choose a reason for hiding this comment

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

Is this OK since jl_typeinf_world is serialized? Or should I add a C function jl_get_typeinf_world?

Copy link
Member

Choose a reason for hiding this comment

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

I don't think this will work. You'll need to ask C what the world age of inference is.

Copy link
Member Author

@tkf tkf Jul 25, 2021

Choose a reason for hiding this comment

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

jl_typeinf_world is set by jl_set_typeinf_func just above. IIUC, this jl_typeinf_world is baked in the sysimage. So, isn't this age correct (though off by one) unless jl_set_typeinf_func is called again?

Of course, I'm fine with adding a proper C function but I'm curious when/how it'd be wrong.

Copy link
Member

Choose a reason for hiding this comment

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

I suppose you're right. World ages do persist across sysimage serialization (but not incremental serialization), so this should be ok.

Copy link
Member

Choose a reason for hiding this comment

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

We could pontially replace jl_call_in_typeinf_world with this too. While normally preserving these values would be risky, this is Core.Compiler, and is loaded early enough it is often not too important, but Revise expects to be able to change this, so it might still be a bit of a nuisance.

So might be better to write this as:

function invoke_in_compiler(args...)
    return ccall(:jl_call_in_typeinf_world, Any, (Ptr{Ptr{Cvoid}}, Cint), Any[args...], length(args))
end

@vtjnash
Copy link
Member

vtjnash commented Nov 12, 2021

I fixed this bug already in a different PR, and I don't think the test change is particularly necessary here

@vtjnash vtjnash closed this Nov 12, 2021
@tkf
Copy link
Member Author

tkf commented Nov 12, 2021

The fix part of it was already in master after #42172.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants