Function inlining #463
Function inlining #463
Conversation
dag_in_context/src/schedule.egg
Outdated
| (repeat 4 function-inlining-subst)) | ||
|
|
||
| ;; We don't want to inline too many layers | ||
| function-inlining |
There was a problem hiding this comment.
does it work to run function-inlining-subst here, but for more iterations?
I think that would make more sense- run function inlining, then immediately run the substitution rules for a while.
dag_in_context/src/schedule.egg
Outdated
| switch_rewrite | ||
| loop-simplify | ||
| ;; Substitution should go for a few iterations but not saturate | ||
| (repeat 4 function-inlining-subst)) |
There was a problem hiding this comment.
4 is quite few, we should talk about this
| v6: int = call @inc v3; | ||
| print v6; | ||
| v10: int = call @double v1; | ||
| print v10; |
|
Rewriting this PR to do function inlining in Rust. |
kirstenmg
force-pushed
the
kirstenmg-function-inlining
branch
3 times, most recently
from
e780737
to
35cacac
Compare
kirstenmg
force-pushed
the
kirstenmg-function-inlining
branch
from
da9f8b4
to
7b0a23e
Compare
|
|
||
| // Gets a list of all the calls in the program | ||
| // and pairs them with an inlined body | ||
| fn get_calls_and_subst( |
There was a problem hiding this comment.
I think you should separate concerns- make a get_calls function and use the resulting set to do the inlining
| calls | ||
| } | ||
|
|
||
| // Generates a ruleset with pairs of (call, inlined body) to union |
There was a problem hiding this comment.
I think this comment is out of date
dag_in_context/src/schema_helpers.rs
Outdated
| Rc::new(Expr::If( | ||
| new_pred.clone(), | ||
| new_input.clone(), | ||
| then.clone(), |
There was a problem hiding this comment.
shouldn't we add more context to then?
you can use replace context
There was a problem hiding this comment.
Using replace_ctx for if/switch branches or loop bodies is much slower. To generate egglog for fib_recursive.bril, it's < 1 s without replacing vs. > 1 min with replacing. This is because replacing context creates more calls to inline. Previously-inlined calls in branches and loop bodies are not equivalent to the context-replaced calls, so the context-replaced calls will also have their corresponding bodies substituted and added to the inlining unions.
If it is still correct to not replace the context, I think it is better to not replace the context and make inlining run in a reasonable time at the expense of losing some opportunities for optimization.
dag_in_context/src/schema_helpers.rs
Outdated
| } | ||
| Expr::DoWhile(input, body) => { | ||
| let new_input = Self::subst_with_cache(arg, arg_ty, arg_ctx, input, subst_cache); | ||
| Rc::new(Expr::DoWhile(new_input.clone(), body.clone())) |
There was a problem hiding this comment.
shouldn't we add more context to body?
|
Function inlining only runs for 2 iterations right now (see #494 ), so some bril programs, like |
dag_in_context/src/lib.rs
Outdated
| @@ -95,12 +104,72 @@ pub(crate) fn print_with_intermediate_vars(termdag: &TermDag, term: Term) -> Str | |||
| printed | |||
| } | |||
|
|
|||
| // Takes in a termdag and a shared cache referencing the termdag, which can be re-used for multiple exprs | |||
| fn global_cached_print_expr_with_intermediate_helper( | |||
There was a problem hiding this comment.
This has a pretty confusing name- I think you should inline this function so it doesn't exist
| @@ -3,7 +3,7 @@ source: tests/files.rs | |||
| expression: visualization.result | |||
| --- | |||
| @main(v0: int) { | |||
| v3: bool = const true; | |||
| v3: bool = const false; | |||
There was a problem hiding this comment.
This change seems a bit odd, but it was originally false: https://github.com/egraphs-good/eggcc/blob/86d79ea803034425ca1f81c5167950297083e6d1/tests/snapshots/files__unstructured-optimize.snap
There was a problem hiding this comment.
🤔 I've seen that change before, it probably doesn't matter for some reason
Implementation of function inlining for DAG-in-context.
I had to carefully manipulate the schedule to make substitution go deep enough without continuing forever on infinitely recursive functions. This is bad, and things may get too slow again once other optimizations are in. However, I don't know what alternative there is other than adding phi nodes.
Note that the majority of
function_inlining.eggis a copy-pasted Subst, renamed to FunctionInliningSubst. The actual inlining rule is at the end, since it uses FunctionInliningSubst.