Migrate from MCJIT to ORC JIT#7166
Conversation
dkurt
force-pushed
the
llvm_orcjit
branch
2 times, most recently
from
672a420 to
191a048
Compare
|
@steven-johnson, please review. All tests passed now (except halide-testbranch-main-llvm16-arm-32-linux-cmake which has not started today but used to do before). |
steven-johnson
left a comment
steven-johnson
left a comment
There was a problem hiding this comment.
This looks pretty great! Let me pull it into Google's source and do some further testing, but I suspect this will be good once it passes our buildbots.
| ctorRunner.add(ctors); | ||
|
|
||
| auto dtors = llvm::orc::getDestructors(*m); | ||
| llvm::orc::CtorDtorRunner *dtorRunner = new llvm::orc::CtorDtorRunner(JIT->getMainJITDylib()); |
There was a problem hiding this comment.
Should this be wrapped in unique_ptr? It's not clear to me where/how this gets disposed eventually.
| dtorRunner->add(dtors); | ||
|
|
||
| // Resolve system symbols (like pthread, dl and others) | ||
| JIT->getMainJITDylib().addGenerator(llvm::cantFail(llvm::orc::DynamicLibrarySearchGenerator::GetForCurrentProcess(target_data_layout.getGlobalPrefix()))); |
There was a problem hiding this comment.
nit: this line is probably a bit too long, adding a break might be nice
| internal_assert(!err) << llvm::toString(std::move(err)) << "\n"; | ||
|
|
||
| // Resolve symbol dependencies | ||
| llvm::orc::SymbolMap NewSymbols; |
There was a problem hiding this comment.
nit: in Halide codebase, local vars should be initial-lowercase, newSymbols
| } | ||
| listeners.clear(); | ||
|
|
||
| // TODO: I don't think this is necessary, we shouldn't have any static constructors |
|
|
||
| if (call_counter2 != 32 * 32) { | ||
| printf("C function my_func2 was called %d times instead of %d\n", call_counter, 32 * 32); | ||
| printf("C function my_func2 was called %d times instead of %d\n", call_counter2, 32 * 32); |
There was a problem hiding this comment.
eek, nice catch :-)
|
Looks like a lot of failures on arm32, unfortunately ("JIT session error: Unsupported target machine architecture in ELF object shared runtime-jitted-objectbuffer") |
|
Google tests look good, just need to fix the arm32 cases |
|
@steven-johnson, thanks for comments! Switched 32 ARM to |
steven-johnson
left a comment
steven-johnson
left a comment
There was a problem hiding this comment.
LGTM, thanks for the fixes!
(Adding @abadams)
|
(Will merge once all buildbots are green) |
* Migrate from MCJIT to ORC JIT
3 participants
resolves #7161
resolves #7078