Skip to content

Commit 2870d7f

Browse files
authored
bpart: Redesign representation of implicit imports (JuliaLang#57755)
# Current Design When we explicitly import a binding (via either `using M: x` or `import`), the corresponding bpart representation is a direct pointer to the imported binding. This is necessary, because that is the precise semantic representation of the import. However, for implicit imports (those created via `using M` without explicit symbol name), the situation is a bit different. Here, there is no semantic content to the particular binding being imported. Worse, the binding is not necessarily unique. Our current semantics are essentially that we walk the entire import graph (both explicit and implicit edges) and if all reachable terminal nodes are either (i) the same binding or (ii) constant bindings with the same value, the import is allowed. In this, we are supposed to ignore cycles, although the current implementation has some trouble with this (JuliaLang#57638, JuliaLang#57699). If the import succeeds, in the current implementation, we then record an arbitrary implicit import edge as in the BindingPartition. In essence, we are creating a spanning tree of the import graph (formed from both the implicit and explicit import edges). However, dynamic algorithms for spanning tree maintenance are complicated and we didn't implement any. As a result, it is possible for later edge additions to accidentally introduce cycles (causing JuliaLang#57699). An additional problem, even if we could keep a consistent spanning tree, is that the spanning tree is not unique. In practice, this is not supposed to be observable, but it is still odd to have a non-unique representation for a particular set of imports. That said, we don't really need the spanning tree. The only place that actually uses it is `which(::Module, ::Symbol)` which is not performance sensitive and arguably a bad API for the above reason that the answer is ill-defined. # This PR With all these problems, let's just get rid of the spanning tree all together - as mentioned we don't really need it. Instead, we split the PARTITION_KIND_IMPLICIT into two, one for each of the two cases (importing a global binding, or importing a particular constant value). This is actually a more efficient implementation for the common case of needing to follow a lookup - we no longer need to follow all the import edges. In exchange, more work needs to be done during binding replacement to re-scan the entire import graph. This is probably the right trade-off though, since binding replacement is already a slow path. Fixes JuliaLang#57638, fixes JuliaLang#57699, fixes JuliaLang#57641, fixes JuliaLang#57700, fixes JuliaLang#57343
1 parent aad26e5 commit 2870d7f

File tree

4 files changed

+30
-9
lines changed

4 files changed

+30
-9
lines changed

src/Compiler.jl

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,6 @@ else
3535

3636
@eval baremodule Compiler
3737

38-
# Needs to match UUID defined in Project.toml
39-
ccall(:jl_set_module_uuid, Cvoid, (Any, NTuple{2, UInt64}), Compiler,
40-
(0x807dbc54_b67e_4c79, 0x8afb_eafe4df6f2e1))
41-
4238
using Core.Intrinsics, Core.IR
4339

4440
using Core: ABIOverride, Builtin, CodeInstance, IntrinsicFunction, MethodInstance, MethodMatch,
@@ -61,7 +57,7 @@ using Base: @_foldable_meta, @_gc_preserve_begin, @_gc_preserve_end, @nospeciali
6157
generating_output, get_nospecializeinfer_sig, get_world_counter, has_free_typevars,
6258
hasgenerator, hasintersect, indexed_iterate, isType, is_file_tracked, is_function_def,
6359
is_meta_expr, is_meta_expr_head, is_nospecialized, is_nospecializeinfer, is_defined_const_binding,
64-
is_some_const_binding, is_some_guard, is_some_imported, is_valid_intrinsic_elptr,
60+
is_some_const_binding, is_some_guard, is_some_imported, is_some_explicit_imported, is_some_binding_imported, is_valid_intrinsic_elptr,
6561
isbitsunion, isconcretedispatch, isdispatchelem, isexpr, isfieldatomic, isidentityfree,
6662
iskindtype, ismutabletypename, ismutationfree, issingletontype, isvarargtype, isvatuple,
6763
kwerr, lookup_binding_partition, may_invoke_generator, methods, midpoint, moduleroot,
@@ -76,6 +72,10 @@ import Base: ==, _topmod, append!, convert, copy, copy!, findall, first, get, ge
7672
getindex, haskey, in, isempty, isready, iterate, iterate, last, length, max_world,
7773
min_world, popfirst!, push!, resize!, setindex!, size, intersect
7874

75+
# Needs to match UUID defined in Project.toml
76+
ccall(:jl_set_module_uuid, Cvoid, (Any, NTuple{2, UInt64}), Compiler,
77+
(0x807dbc54_b67e_4c79, 0x8afb_eafe4df6f2e1))
78+
7979
const getproperty = Core.getfield
8080
const setproperty! = Core.setfield!
8181
const swapproperty! = Core.swapfield!
@@ -131,7 +131,7 @@ something(x::Any, y...) = x
131131
############
132132

133133
baremodule BuildSettings
134-
using Core: ARGS, include
134+
using Core: ARGS, include, Int, ===
135135
using ..Compiler: >, getindex, length
136136

137137
global MAX_METHODS::Int = 3
@@ -191,7 +191,7 @@ macro __SOURCE_FILE__()
191191
end
192192

193193
module IRShow end # relies on string and IO operations defined in Base
194-
baremodule TrimVerifier end # relies on IRShow, so define this afterwards
194+
baremodule TrimVerifier using Core end # relies on IRShow, so define this afterwards
195195

196196
if isdefined(Base, :end_base_include)
197197
# When this module is loaded as the standard library, include these files as usual

src/abstractinterpretation.jl

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -3289,7 +3289,7 @@ function abstract_eval_isdefinedglobal(interp::AbstractInterpreter, mod::Module,
32893289
if allow_import !== true
32903290
gr = GlobalRef(mod, sym)
32913291
partition = lookup_binding_partition!(interp, gr, sv)
3292-
if allow_import !== true && is_some_imported(binding_kind(partition))
3292+
if allow_import !== true && is_some_binding_imported(binding_kind(partition))
32933293
if allow_import === false
32943294
rt = Const(false)
32953295
else
@@ -3542,7 +3542,7 @@ end
35423542

35433543
function walk_binding_partition(imported_binding::Core.Binding, partition::Core.BindingPartition, world::UInt)
35443544
valid_worlds = WorldRange(partition.min_world, partition.max_world)
3545-
while is_some_imported(binding_kind(partition))
3545+
while is_some_binding_imported(binding_kind(partition))
35463546
imported_binding = partition_restriction(partition)::Core.Binding
35473547
partition = lookup_binding_partition(world, imported_binding)
35483548
valid_worlds = intersect(valid_worlds, WorldRange(partition.min_world, partition.max_world))

src/ssair/EscapeAnalysis.jl

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,7 @@ using Base: Base
1515
# imports
1616
import Base: ==, copy, getindex, setindex!
1717
# usings
18+
using Core
1819
using Core: Builtin, IntrinsicFunction, SimpleVector, ifelse, sizeof
1920
using Core.IR
2021
using Base: # Base definitions

test/codegen.jl

Lines changed: 20 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1007,3 +1007,23 @@ let
10071007
end
10081008
nothing
10091009
end
1010+
1011+
# Test that turning an implicit import into an explicit one doesn't pessimize codegen
1012+
module TurnedIntoExplicit
1013+
using Test
1014+
import ..get_llvm
1015+
1016+
module ReExportBitCast
1017+
export bitcast
1018+
import Base: bitcast
1019+
end
1020+
using .ReExportBitCast
1021+
1022+
f(x::UInt) = bitcast(Float64, x)
1023+
1024+
@test !occursin("jl_apply_generic", get_llvm(f, Tuple{UInt}))
1025+
1026+
import Base: bitcast
1027+
1028+
@test !occursin("jl_apply_generic", get_llvm(f, Tuple{UInt}))
1029+
end

0 commit comments

Comments
 (0)