Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1524 +/- ##
==========================================
- Coverage 83.82% 83.80% -0.03%
==========================================
Files 187 188 +1
Lines 28954 29032 +78
Branches 27875 27928 +53
==========================================
+ Hits 24271 24330 +59
- Misses 3522 3540 +18
- Partials 1161 1162 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
70438a0 to
696e194
Compare
696e194 to
caa565a
Compare
cqc-alec
left a comment
There was a problem hiding this comment.
Thanks, just small comments.
|
|
||
| // Inline function calls. | ||
| pub mod inline_funcs; | ||
| pub use inline_funcs::InlineFunctionsPass; |
There was a problem hiding this comment.
Maybe this should be called something like InlineSmallFunctionsPass, since the name suggests that it inlines all functions.
There was a problem hiding this comment.
I don't think we want to limit ourselves to only small functions here.
The max_size limit is the simplest of the heuristics we could apply.
We may want to add alternatives in the future that decide based on the function types or something else.
We'll also want to add the option to inline functions based on inline hints in their metadata.
I think I'll add an InlineHeuristic enum instead of the max_size flag.
Do you have an alternative name suggestion that also works for different heuristics?
There was a problem hiding this comment.
Sounds good to me. I think the name is fine if it is referring to a general heuristic.
| - remove_redundant_order_edges: Whether to remove redundant order edges. | ||
| """ | ||
|
|
||
| def inline_functions( |
There was a problem hiding this comment.
Maybe rename to inline_small_functions? Alternatively we could have max_inline_size be optional and if not set inline all functions?
There was a problem hiding this comment.
I'd like to have the default behaviour be useful. I don't think "inline everything" is something users will want to do most of the time.
See my comment about a Heuristics enum below. I'll add an All variant.
Co-authored-by: Alec Edgington <54802828+cqc-alec@users.noreply.github.com>
|
Added the heuristic enum, and a flag for following inline hints. I'm using |
Adds a pass definition that calls the already-defined function inlining tool.
This version uses a simple heuristic to choose when to inline:
We can add better heuristicts in the future, and check for inline hints.
This is a first version to help with simple optimization cases.
Note that the inlined funcDefs are not removed.
RemoveDeadFuncsPasscan be called afterwards to remove what's not needed.We should also add this as a first step in
NormalizeGuppy