Enable users to map from GV to Julia value and initialize them whenever possible #752
Conversation
|
Your PR requires formatting changes to meet the project's style guidelines. Click here to view the suggested changes.diff --git a/src/driver.jl b/src/driver.jl
index b1d55d3..bdd36e7 100644
--- a/src/driver.jl
+++ b/src/driver.jl
@@ -258,7 +258,7 @@ const __llvm_initialized = Ref(false)
merge!(compiled, dyn_meta.compiled)
if haskey(dyn_meta, :gv_to_value)
merge!(gv_to_value, dyn_meta.gv_to_value)
- end
+ end
@assert context(dyn_ir) == context(ir)
link!(ir, dyn_ir)
changed = true
diff --git a/src/jlgen.jl b/src/jlgen.jl
index 0e2f6d3..8be5dc8 100644
--- a/src/jlgen.jl
+++ b/src/jlgen.jl
@@ -789,15 +789,18 @@ function compile_method_instance(@nospecialize(job::CompilerJob))
elseif HAS_LLVM_GVS_GLOBALS
if VERSION >= v"1.12.0-DEV.1703"
num_gvars = Ref{Csize_t}(0)
- @ccall jl_get_llvm_gvs(native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
+ @ccall jl_get_llvm_gvs(
+ native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
C_NULL::Ptr{Cvoid}
)::Nothing
gvs = Vector{Ptr{LLVM.API.LLVMOpaqueValue}}(undef, num_gvars[])
- @ccall jl_get_llvm_gvs_globals(native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
+ @ccall jl_get_llvm_gvs_globals(
+ native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
gvs::Ptr{LLVM.API.LLVMOpaqueValue}
)::Nothing
inits = Vector{Ptr{Cvoid}}(undef, num_gvars[])
- @ccall jl_get_llvm_gvs(native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
+ @ccall jl_get_llvm_gvs(
+ native_code::Ptr{Cvoid}, num_gvars::Ptr{Csize_t},
inits::Ptr{Cvoid}
)::Nothing
else
diff --git a/src/utils.jl b/src/utils.jl
index 674d8f9..4aa2c9c 100644
--- a/src/utils.jl
+++ b/src/utils.jl
@@ -187,7 +187,8 @@ end
import Libdl
const HAS_LLVM_GVS_GLOBALS = Libdl.dlsym(
- unsafe_load(cglobal(:jl_libjulia_handle, Ptr{Cvoid})), :jl_get_llvm_gvs_globals, throw_error=false) !== nothing
+ unsafe_load(cglobal(:jl_libjulia_handle, Ptr{Cvoid})), :jl_get_llvm_gvs_globals, throw_error = false
+ ) !== nothing
const AL_N_INLINE = 29
diff --git a/test/helpers/native.jl b/test/helpers/native.jl
index 656028f..78d63b2 100644
--- a/test/helpers/native.jl
+++ b/test/helpers/native.jl
@@ -26,7 +26,7 @@ function create_job(@nospecialize(func), @nospecialize(types);
entry_safepoint::Bool=false, method_table=test_method_table, kwargs...)
config_kwargs, kwargs = split_kwargs(kwargs, GPUCompiler.CONFIG_KWARGS)
source = methodinstance(typeof(func), Base.to_tuple_type(types), Base.get_world_counter())
- target = NativeCompilerTarget(;jlruntime=true)
+ target = NativeCompilerTarget(; jlruntime = true)
params = CompilerParams(entry_safepoint, method_table)
config = CompilerConfig(target, params; kernel=false, config_kwargs...)
CompilerJob(source, config), kwargs
diff --git a/test/native.jl b/test/native.jl
index 83339ea..72087b5 100644
--- a/test/native.jl
+++ b/test/native.jl
@@ -36,17 +36,17 @@ end
@testset "compilation database" begin
mod = @eval module $(gensym())
@noinline inner(x) = x+1
- function outer(x, sym)
- if sym == :a
- return inner(x)
- end
- return x
+ function outer(x, sym)
+ if sym == :a
+ return inner(x)
+ end
+ return x
end
end
job, _ = Native.create_job(mod.outer, (Int, Symbol))
JuliaContext() do ctx
- ir, meta = GPUCompiler.compile(:llvm, job; validate=false)
+ ir, meta = GPUCompiler.compile(:llvm, job; validate = false)
meth = only(methods(mod.outer, (Int, Symbol)))
diff --git a/test/native/precompile.jl b/test/native/precompile.jl
index d4c0a7a..4a95276 100644
--- a/test/native/precompile.jl
+++ b/test/native/precompile.jl
@@ -13,33 +13,33 @@ precompile_test_harness("Inference caching") do load_path
A[1] = x
return
end
-
- function kernel_w_global(A, x, sym)
- if sym == :A
- A[1] = x
+
+ function kernel_w_global(A, x, sym)
+ if sym == :A
+ A[1] = x
+ end
+ return
end
- return
- end
- function square(x)
- return x*x
- end
+ function square(x)
+ return x * x
+ end
let
job, _ = NativeCompiler.Native.create_job(kernel, (Vector{Int}, Int))
precompile(job)
end
- let
- job, _ = NativeCompiler.Native.create_job(kernel_w_global, (Vector{Int}, Int, Symbol))
- precompile(job)
- end
+ let
+ job, _ = NativeCompiler.Native.create_job(kernel_w_global, (Vector{Int}, Int, Symbol))
+ precompile(job)
+ end
- let
- # Emit the func abi to box the return
- job, _ = NativeCompiler.Native.create_job(square, (Float64,), entry_abi=:func)
- precompile(job)
- end
+ let
+ # Emit the func abi to box the return
+ job, _ = NativeCompiler.Native.create_job(square, (Float64,), entry_abi = :func)
+ precompile(job)
+ end
# identity is foreign
@setup_workload begin |
maleadt
left a comment
maleadt
left a comment
There was a problem hiding this comment.
Needs documentation (what is the purpose of the map, how to use, etc) and polish (there's TODOs and comments that feel like they should be resolved). It seems like there was a reason #746 was still marked as draft, so I'll wait for @vchuravy to indicate the exact scope of what still needs to happen and for final approval.
src/jlgen.jl
Outdated
| # It's valid to call Base.unsafe_pointer_to_objref on values(gv_to_value), | ||
| # but we may not be able to "easily" obtain the pointer back later. | ||
| # (Types, etc, disallow Base.pointer_from_objref on them.) |
There was a problem hiding this comment.
it applies to the values whose pointer is stored in in gv_to_value. Specifically if you have a global representing 2.0, we have gvalue = pointer_from_objref(2.0). There's an issue however that performing pointer_from_objref on that type is invalid [since its not mutable], hence we store the actual pointer
There was a problem hiding this comment.
if you have a global representing 2.0
To provide a valid example
function f(sym)
if sym == :a
end
:a will initially be emitted as a GV jl_sym_a_8083 with an initializer, now at some point we privatize that GV leaving us with something like:
%.not = icmp eq ptr %"sym::Symbol", inttoptr (i64 140096668482288 to ptr), !dbg !38
Now this makes the code not relocatable, but even more annoyingly we don't know what this pointer out of thin air is pointing to. Can we use unsafe_pointer_to_objref to look at it? Or is it something else?
gv_to_value essentially answers the question and opens the door to later actually handle the relocatability
|
@maleadt my PR was stalled since there was some weird ordering around moving the initialization of the global variables. I cleaned up the comments a bit, added a (very) small test, and used it to do something useful (decide if we can cache something) |
vchuravy
changed the title
|
With this patch to simulate precompilation: diff --git a/src/jlgen.jl b/src/jlgen.jl
index b29e038..7a66505 100644
--- a/src/jlgen.jl
+++ b/src/jlgen.jl
@@ -801,7 +801,7 @@ function compile_method_instance(@nospecialize(job::CompilerJob))
else
ccall(:jl_create_native, Ptr{Cvoid},
(Vector{MethodInstance}, LLVM.API.LLVMOrcThreadSafeModuleRef, Ptr{Base.CodegenParams}, Cint, Cint, Cint, Csize_t),
- [job.source], ts_mod, Ref(params), CompilationPolicyExtern, #=imaging mode=# 0, #=external linkage=# 0, job.world)
+ [job.source], ts_mod, Ref(params), CompilationPolicyExtern, #=imaging mode=# 1, #=external linkage=# 0, job.world)
end
@assert native_code != C_NULL
and this setup: using GPUCompiler
include("test/helpers/runtime.jl")
include("test/helpers/native.jl")
square(a) = a*a
Native.code_llvm(square, (Float64,), entry_abi=:func, optimize=false)On normal 1.10 we get: ; ...
%12 = call fastcc double @julia_square_253(double %11)
%Float64 = ptrtoint {}* null to i64
%13 = inttoptr i64 %Float64 to {}*
; ...the global variable got internalized and we get a null. On the ; ...
%12 = call fastcc double @julia_square_569(double %11)
%Float64 = ptrtoint {}* inttoptr (i64 140628451007024 to {}*) to i64
%13 = inttoptr i64 %Float64 to {}*
; ... |
|
The check should be something like this [5:28 PM]b = unsafe_load(Ptr{Ptr{Cvoid}}(cglobal(:jl_libjulia_handle)))
Libdl.dlsym(b, :jl_get_llvm_gv_inits, throw_error=false) |
|
Okay this is now in a state that I am happy with. @maleadt do you want to do another review (especially since I rewrote a lot of the comments) |
| if haskey(dyn_meta, :gv_to_value) | ||
| merge!(gv_to_value, dyn_meta.gv_to_value) | ||
| end |
There was a problem hiding this comment.
We always include gv_to_value, so why the conditional merge?
There was a problem hiding this comment.
Downstream CI broke on Enzyme.jl
https://buildkite.com/julialang/gpucompiler-dot-jl/builds/2129/steps/canvas
and I would like to make this not a breaking release
|
Should we always emit this with imaging mode for consistency? Or do we only care when we're actually precompiling |
Perhaps, but that would be to disruptive. It would immediately break v1.10, and v1.11, 1.12 is a weird intermediate state, and 1.13 already uses So for now, this is mostly defensive against use during precompilation, and will pave the way to actually emit relocatable caches down the road. |
|
Integration test hits a seemingly internal error in GPUCompiler |
Where do you see that? They are still running and sofar green? |
|
from your earlier log: https://buildkite.com/julialang/gpucompiler-dot-jl/builds/2129/steps/canvas but it seems you might've fixed it already? |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #752 +/- ##
==========================================
- Coverage 74.75% 74.22% -0.54%
==========================================
Files 24 24
Lines 3672 3728 +56
==========================================
+ Hits 2745 2767 +22
- Misses 927 961 +34 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
since this seems approved by all, I'm going to go ahead and merge, but let you all do the release [presumably a minor version bump?] |
Simplified version of #746 that just adds the additional argument with remaining a NFC for other initialization/IR changes.
Context: it became somewhat more urgent to get this in for Enzyme/Reactant to support 1.12 + relocation/precompilation. Rather than do homebrew solutions, having this would enable a significant simplification.