-
-
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
[WIP] Add Base.build_sysimg() #7124
Closed
Closed
Changes from all commits
Commits
Show all changes
3 commits
Select commit
Hold shift + click to select a range
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -329,4 +329,4 @@ end | |
function download(url::String) | ||
filename = tempname() | ||
download(url, filename) | ||
end | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,77 @@ | ||
# By default, put the system image next to libjulia | ||
build_sysimg(;force=false, cpu_target="native") = build_sysimg(joinpath(dirname(Sys.dlpath("libjulia")),"sys"), force=force, cpu_target=cpu_target) | ||
|
||
# Build a system image binary at sysimg_path.dlext. If a system image is already loaded, error out, or continue if force = true | ||
function build_sysimg(sysimg_path; force=false, cpu_target="native") | ||
# Unless force == true, quit out if a sysimg is already loadable | ||
sysimg = dlopen_e("sys") | ||
if !force && sysimg != C_NULL | ||
println("System image already loaded at $(Sys.dlpath(sysimg)), pass \"force=true\" to override") | ||
return; | ||
end | ||
|
||
# Enter base/ and setup some useful paths | ||
base_dir = dirname(Base.find_source_file("sysimg.jl")) | ||
cd(base_dir) do | ||
julia = joinpath(JULIA_HOME, "julia") | ||
julia_libdir = dirname(Sys.dlpath("libjulia")) | ||
ld = find_system_linker() | ||
|
||
# Ensure we have write-permissions to wherever we're trying to write to | ||
if !success(`touch $sysimg_path.$(Sys.dlext)`) | ||
error("$sysimg_path unwritable, ensure parent directory exists and is writable! (Do you need to run this with sudo?)") | ||
end | ||
|
||
# Start by building sys0.{ji,o} | ||
sys0_path = joinpath(dirname(sysimg_path), "sys0") | ||
println("Building sys0.o...") | ||
println("$julia -C $cpu_target --build $sys0_path sysimg.jl") | ||
run(`$julia -C $cpu_target --build $sys0_path sysimg.jl`) | ||
|
||
# Bootstrap off of that to create sys.{ji,o} | ||
println("Building sys.o...") | ||
println("$julia -C $cpu_target --build $sysimg_path -J $sys0_path.ji -f sysimg.jl") | ||
run(`$julia -C $cpu_target --build $sysimg_path -J $sys0_path.ji -f sysimg.jl`) | ||
|
||
# Link sys.o into sys.$(dlext) | ||
FLAGS = ["-L$julia_libdir"] | ||
if OS_NAME == :Darwin | ||
push!(FLAGS, "-dylib") | ||
push!(FLAGS, "-undefined") | ||
push!(FLAGS, "dynamic_lookup") | ||
else | ||
push!(FLAGS, "--unresolved-symbols") | ||
push!(FLAGS, "ignore-all") | ||
end | ||
@windows_only append!(FLAGS, ["-L$JULIA_HOME", "-ljulia", "-lssp"]) | ||
|
||
println("Linking sys.$(Sys.dlext)") | ||
run(`$ld $FLAGS -o $sysimg_path.$(Sys.dlext) $sysimg_path.o`) | ||
end | ||
end | ||
|
||
# Search for a linker to link sys.o into sys.dl_ext. Honor LD environment variable, otherwise search for something we know works | ||
function find_system_linker() | ||
if haskey( ENV, "LD" ) | ||
if !success(`which $(ENV["LD"])`) | ||
warn("Using linker override $(ENV["LD"]), but unable to find `$(ENV["LD"])`") | ||
end | ||
return ENV["LD"] | ||
end | ||
|
||
poss_linkers = ["ld", "link"] | ||
|
||
for linker in poss_linkers | ||
try | ||
if success(`which $linker`) | ||
return linker | ||
end | ||
end | ||
end | ||
|
||
error( "No supported linker found (tried $(join(poss_linkers, ", "))), override with LD environment variable!" ) | ||
end | ||
|
||
if !isinteractive() | ||
build_sysimg() | ||
end |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
It might be that an empty target string defaults to i386 as a lowest common denominator and not to native.
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.
The empty string is what we were passing before, but I'm definitely not knowledgable about LLVM, so if you've got documentation you can share, I'd love to go over it. I can't find much about
setMCPU()
, but then again I don't really know where to look.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 tried to find some reliable documentation and the only thing I know is that clang defaults to i386 if no -march and -mcpu is given.
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.
This isn't clang though. This is MCJIT, which defaults to cpuid (from TargetSelect.cpp):
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.
But isn't TargetTriple -march?
In trunk MCPU is just passed through to createTargetMachine (compare https://github.com/llvm-mirror/llvm/blob/0b6cb7104b15504cd41f48cc2babcbcee70775f3/lib/ExecutionEngine/TargetSelect.cpp#L100)
And the question is what the backend is going to do with it. I haven't found the implementation for that yet.
And the TargetTriple just makes sure that the right bitcode is created for x86_64 vs x86 and linux vs mac vs windows. While MCPU sets the capabilities of the CPU.
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.
But that's my point with MCPU we are setting the CPU and not the target triplet and also not MARCH.
I think we probably should set it like here https://github.com/llvm-mirror/llvm/blob/master/unittests/ExecutionEngine/MCJIT/MCJITTestBase.h#L328
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.
@vchuravy if you can give me example lines to put in, I'm willing to experiment. We've already got some pretty good evidence that the
.setMCPU("")
call is causing AVX instructions to be emitted (whereas.setMCPU("i386")
is stopping atsse2
) but perhaps there are other considerations to be taken into account.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.
As far as I can tell,
.setMCPU(sys::getHostCPUName())
has the same effect as.setMCPU("")
. Indeed, it's when I discovered thatsys::getHostCPUName()
was returning a generic x86 string for a "Haswell" processor that I worked on implementing #7155.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.
Yes @ArchRobison is correct. If you still don't believe me, I'd encourage you to step through the function you referenced in a debugger to see the control flow.
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 do see an effect.
Where I applied the following change to codegen.cpp in L4330
The sysimages where generated by
I am on a second generation i5 (Sandybridge???) and my architecture should be something like corei7-avx.
So for me there is a definite change when I specify my host architecture and I am also seeing llvm defaulting to i386.
Edit: Oh and I am on llvm trunk, if that matters.