Repository navigation
jit: add optional LLVM optimization - #6416
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #6416 +/- ##
==========================================
- Coverage 86.95% 86.95% -0.01%
==========================================
Files 439 439
Lines 66002 66016 +14
Branches 7513 7514 +1
==========================================
+ Hits 57392 57404 +12
- Misses 7038 7039 +1
- Partials 1572 1573 +1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| """Pass pipeline applied before resolving ``symbol``.""" | ||
|
|
||
| optimize: bool | ||
| """Run host O3 optimization.""" |
There was a problem hiding this comment.
Not very clued up on what's happening here, but why have a boolean for O3 optimisation instead of a level allowing you to choose the optimisation lever?
There was a problem hiding this comment.
This is just a very opinionated way to just apply ✨ all the optimizations ✨.
Would it be better to make this more configurable or update the comment?
There was a problem hiding this comment.
This could also be a seperate follow up PR
There was a problem hiding this comment.
Let's just make this match the llvm API, if they provide integer levels then it feels like we should here also.
There was a problem hiding this comment.
Actually probably integer levels or None if we want to skip optimisations.
There was a problem hiding this comment.
Donzo 🫡
Did i get this right?
|
Am I right in understanding that this could now target |
|
Whoops 😅 Yes yes |
fab5be3 to
8fdca4d
Compare
opt_level was optional, and None skipped the IR pipeline while code generation still ran at level 2. Make it a plain int defaulting to 2, so both agree, and check its range when the backend is constructed rather than when it runs.
opt_level took any int and rejected out of range values at construction, which duplicates a constraint llvmlite already owns. Make it a Literal instead, so pyright rejects a bad level at the call site and the runtime check can go.
| ), | ||
| ), | ||
| *, | ||
| opt_level: Literal[0, 1, 2, 3] = 2, |
There was a problem hiding this comment.
I'm confused, does this change what is happening? previously, we were not passing anything to opt, now we pass 2, is that guaranteed to be the default value for the opt argument to target_machine?
There was a problem hiding this comment.
Previously, passing None was silly, and the 2 flag is the llvmlite default.
This should be the expected behavior now.
There was a problem hiding this comment.
Thanks for paying close attention
There was a problem hiding this comment.
Likewise, thanks for this improvement :)
3 participants
Adds opt-in host O3 optimization after separating the refactoring requested on PR #6410, comment #3889970708 --- stacked on #6415.