Repository navigation
jit: use the native process target - #6415
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6415 +/- ##
==========================================
- Coverage 86.95% 86.94% -0.01%
==========================================
Files 439 439
Lines 66016 65970 -46
Branches 7514 7514
==========================================
- Hits 57404 57360 -44
+ Misses 7039 7038 -1
+ Partials 1573 1572 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Adds opt-in host O3 optimization after separating the refactoring requested on [PR #6410, comment #3889970708](#6410 (comment)) --- stacked on #6415.
Reject modules whose data layout is not the native one, and give the entry point external linkage so that global DCE in the optimization pipeline cannot drop it. Annotate the llvmlite stubs both need.
LLVM writes one target several ways, such as arm64 for aarch64 and macosx for darwin, and the order of the specs in a data layout carries no meaning, so comparing either as a string rejects a module that names the native target. Closes xdslproject#6426
7bc7473 to
789044c
Compare
|
Okidokes - I'm happy with this PR now and would really appreciate a second pair of eyes to look at the diff :) |
| _compile_module, # pyright: ignore[reportPrivateUsage] | ||
| _create_target_machine, # pyright: ignore[reportPrivateUsage] | ||
| _is_native_data_layout, # pyright: ignore[reportPrivateUsage] | ||
| _is_native_triple, # pyright: ignore[reportPrivateUsage] |
There was a problem hiding this comment.
I would recommend against this, we should be testing user-facing observable behaviour. If these are observable to the user then they should be public, otherwise let's test the APIs and results/errors for those. (To me it feels like_is_native_data_layout and _is_native_triple should be public, not sure about the other ones).
There was a problem hiding this comment.
Makes sense - I also thought harder about this and lots of pieces were actually even unreachable because we were assigning them ourselves a couple steps further up the call chain, before verifying
The data layout check, the missing function check, and the external linkage of the entry point all guard a state that `jit` cannot produce. It overwrites the data layout with the native one, rejects a symbol that is not an `llvm.func` before the module is compiled, and the LLVM converter drops the linkage of a function anyway. Testing them meant importing private helpers, which is what kept the three from looking dead. Test the target triple through the public API instead, and make the predicate that survives it public.
|
Thank you |
|
Thank you! |
2 participants
Uses the process triple and native data layout for MCJIT, following feedback on PR #6410, comment #3889958560 --- stacked on #6414.
Closes the stacked #6410, whose remaining work it carries, and fixes #6426.