-
-
Notifications
You must be signed in to change notification settings - Fork 264
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
D dynamic compilation support #2293
Conversation
|
Related druntime PR: ldc-developers/druntime#101 |
.gitmodules
Outdated
| @@ -1,6 +1,7 @@ | |||
| [submodule "druntime"] | |||
| path = runtime/druntime | |||
| url = https://github.com/ldc-developers/druntime.git | |||
| url = https://github.com/Hardcode84/druntime.git | |||
| branch = runtime_compile | |||
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.
Revert this, I'll pull the druntime PR as soon as you add a doc comment to it.
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.
done
ddmd/globals.h
Outdated
| @@ -234,6 +234,8 @@ struct Param | |||
| uint32_t hashThreshold; // MD5 hash symbols larger than this threshold (0 = no hashing) | |||
|
|
|||
| bool outputSourceLocations; // if true, output line tables. | |||
|
|
|||
| bool enableRuntimeCompile; | |||
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.
A small comment like the above would be nice.
bb143c2
to
a893405
Compare
|
So here some design overview of dynamic compilation feature
Compiler part:
Runtime part:
|
|
Thanks a lot for this, this is impressive work! I hope I can find time for a closer review soon, but I'm sure the others will weigh in as well if you just ping them often enough. Do you keep a todo-list somewhere looking at actually shipping this on multiple OSes, etc.? |
|
I tested this on linux and there is some issue with thread locals, except that everything seems to work. First comment updated. |
ddmd/globals.h
Outdated
| @@ -234,6 +234,8 @@ struct Param | |||
| uint32_t hashThreshold; // MD5 hash symbols larger than this threshold (0 = no hashing) | |||
|
|
|||
| bool outputSourceLocations; // if true, output line tables. | |||
|
|
|||
| bool enableRuntimeCompile; // Enable dynamic compilation | |||
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 is not needed (we try to reduce the frontend diff (i.e. changes in ./ddmd/) as much as possible). Instead you should define cl::opt<bool> enableRuntimeCompile, such that it does not use "external" storage.
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.
done
| @@ -0,0 +1,93 @@ | |||
| if(LDC_RUNTIME_COMPILE) | |||
| find_package(LLVM 3.9 REQUIRED support core irreader executionengine passes target nativecodegen) | |||
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 guess this conflicts with https://github.com/ldc-developers/ldc/blob/master/CMakeLists.txt#L22 and thus prevents successful linking.
runtime/CMakeLists.txt
Outdated
| @@ -600,11 +601,17 @@ macro(build_all_runtime_variants d_flags c_flags ld_flags path_suffix outlist_ta | |||
| get_target_suffix("" "${path_suffix}" target_suffix) | |||
| set_common_library_properties(ldc-profile-rt${target_suffix}) | |||
| endif() | |||
|
|
|||
| build_jit_runtime ("${d_flags}" "${c_flags}" "${ld_flags}" "${path_suffix}" ${outlist_targets}) | |||
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.
Any reason you're not including ${D_FLAGS} and maybe ${D_FLAGS_RELEASE} here?
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.
added
| @@ -1,5 +1,5 @@ | |||
| if(LDC_RUNTIME_COMPILE) | |||
| find_package(LLVM 3.9 REQUIRED support core irreader executionengine passes target nativecodegen) | |||
| find_package(LLVM 3.7 REQUIRED support core irreader executionengine passes target nativecodegen) | |||
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.
What I meant was that invoking find_package() twice probably doesn't work and that conditionally including your additional libs in the single invokation should, although at the cost of less modularity.
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 noticed another thing, it fail to find 32-bit llvm libraries when MULTILIB is ON, but as I understand compiler itself will still be 64 bit in this case. Does these machines have 32-bit llvm libs?
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 compiler will be 64-bit, no 32-bit LLVM available and the 32-bit libs definitely shouldn't be needed; only the LDC executable (edit: and tools/utils? all 64-bit anyway) needs to be linked to LLVM. So this would indicate that some compiler/linker flags leak into other build targets (e.g., the 32-bit shared runtime libs).
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 dynamic compilation through llvm in my jit runtime shared library, which then linked to the target application, and if this application is 32-bit then jit runtime and llvm libs also have to be 32-bit.
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.
Ah yes that makes perfect sense. I guess you'll have to build the native JIT runtime library exclusively then for now, excluding the 32-bit one for MULTILIB builds.
driver/linker-gcc.cpp
Outdated
| @@ -340,10 +340,12 @@ void ArgsBuilder::build(llvm::StringRef outputPath, | |||
| args.push_back("-lldc-profile-rt"); | |||
| } | |||
|
|
|||
| if (global.params.enableRuntimeCompile) { | |||
| #if defined(LDC_RUNTIME_COMPILE) | |||
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.
nitpick: to prevent all these #if things, you could create a function isRuntimeCompileEnabled() similar to isUsingLTO().
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.
Done
|
druntime submodule update: |
|
Why a separate PR? Just update the submodule in here. |
|
ok |
|
Renamed EVERYWHERE |
|
Sorry to bother you hopefully one last time, but there's still |
CMakeLists.txt
Outdated
| # | ||
| set(LDC_DYNAMIC_COMPILE False) # must be a valid Python boolean constant (case sensitive) | ||
| if (NOT (LDC_LLVM_VER LESS 400)) | ||
| message(STATUS "Building LDC with runtime compilation support") |
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.
"with dynamic compilation support" for consistency.
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.
Done
LICENSE
Outdated
| @@ -23,6 +23,9 @@ Libraries (lib/ and import/ in binary packages): | |||
| Phobos (runtime/phobos), is distributed under the terms of the Boost | |||
| Software License. See the individual source files for author information. | |||
|
|
|||
| - The jit runtime library is distributed under the terms of the Boost | |||
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'd make this "The ldc-jit-rt [or whatever library it really is] library is distributed under the terms of the Boost Software license (but additionally makes use of LLVM code)." or something like that for clarity – unless I'm missing something, people need to distribute the LLVM copyright notice when they are using dynamic compile support.
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.
Done
runtime/jit-rt/cpp-so/optimizer.cpp
Outdated
| var->setConstant(true); | ||
| var->setInitializer(initializer); | ||
| var->setLinkage(llvm::GlobalValue::PrivateLinkage); | ||
| // auto tempVar = new llvm::GlobalVariable( |
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.
What's the deal with this (and other) commented out sections?
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.
Removed
| /** | ||
| * Contains dynamic compilation API. | ||
| * | ||
| * Copyright: Authors 2017 |
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 should probably at least say something vague like "the LDC team", or your name.
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 was copypasted from somewhere, fixed to "the LDC team". But I don't like these 'authors' fields because everyone forget to update them.
|
More renamings and cleanups |
|
Thanks Ivan. I'll merge this tomorrow |
|
Ouch, I would have very much preferred this being merged in less than 93 commits. |
|
Feasible options being 1 or 93, I opted for the full bunch. I'll gladly let others make decisions like this in the future, I gave a 24+h notice. |
|
It's not a huge thing either way, but there is also no need to react in a snarky fashion. |
|
Thanks for review. Actually, I wanted to ask about squashing but I forgot. |
Here is current state of my 'jit' work. Simple examples work but everything mostly untested (and I'm not sure how properly test it). Tested on win64 and linux64. IR preparation for jit mostly in gen/runtimecompile.cpp. Jit itself implemented as shared library in runtime/rtcompile-rt/cpp-so/, it was mostly copypasted from llvm examples and I'm not sure it 100% correct.
Status:
Tests:
Platform support:
Win64:
Linux:
MacOS:
Areas of improvement:
@runtimeCompileattribute to allow mark some functions always non-dynamic