Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tail Recursion Optimization #218

Draft
wants to merge 51 commits into
base: mlscript
Choose a base branch
from
Draft

Conversation

CAG2Mark
Copy link
Contributor

@CAG2Mark CAG2Mark commented Mar 29, 2024

Adds a pass to the IR which optimizes mutually tail-recursive and mutually tail-recursive modulo cons functions.

The full pass first optimizes modulo cons functions to be tail recursive, then optimizes those functions to be tail recursive.

This PR consists of the following features

IR Field Assignment

Adds an assignment primitive to the IR. Given x: CtorApp and val: TrvialExpr, we now have the primitive

x.field = val in ...

Also modified the interpreter to no longer assume references to constructors are referentially transparent.

Tail Recursion Modulo Cons

Extends the idea in this paper to optimize strongly connected components of tail recursive functions, of which some calls may be within a constructor (guarded recursion).

Supports both modulo-cons calls and regular tail calls within the same strongly connected component.

Rewrite mutually tail recursive functions

Suppose we have functions:

def f(n) = e1
def g(m) = e2

where e1 and e2 tail-call f or g. This function will be rewritten a single function:

def f_g(branch, n, m) =
  let join j(branch, m, n) = 
    if branch == 0 then e1'
    else e2'
  j(branch, m, n)

def f(n) = f_g(0, n, dummy)
def g(n) = f_g(1, dummy, m)

where e1' and e2' are e1 and e2 with their tail calls to f and g replaced with jumps to j.

In general, strongly connected components of mutually tail recursive functions will be optimized in this way. All join points used in each strongly connected component will be rewritten and merged into the same join point.

@CAG2Mark CAG2Mark self-assigned this Mar 29, 2024
@CAG2Mark CAG2Mark added the enhancement New feature or request label Mar 29, 2024
@CAG2Mark
Copy link
Contributor Author

CAG2Mark commented Mar 29, 2024

TODO:

  • Make sure variable names in the stack frame do not clash
  • Make sure the merged function's name does not clash with other functions
  • Substitute dummy variables (may need extension to the IR). See here
  • Propagage @tailrec annotations to the IR.

@@ -66,37 +77,57 @@ class TailRecOpt(fnUid: FreshInt, classUid: FreshInt, tag: FreshInt):
if names.contains(nme) then Some(nme)
else None

// would prefer to have this inside discoverOptimizableCalls, but this makes scala think it's not tail recursive
// would prefer to have this inside discoverOptCalls, but scala does not support partially tail recursive functions directly
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you could just add private val discoverOptCallsIndirect = discoverOptCalls and call that instead.

@LPTK
Copy link
Contributor

LPTK commented May 13, 2024

Instead of writing class True and class False in every single test case, why not make it a built-in class info? It should be a trivial change and would remove much clutter. Could also do the same with Cons/Nil and Some/None.

// Tail calls to another function in the component will be replaced with a tail call
// to the merged function
def transformDefn(defn: Defn): Defn =
// TODO: Figure out how to substitute variables with dummy variables.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be addressed or explained better.

@LPTK
Copy link
Contributor

LPTK commented May 13, 2024

Steps before this can be an official PR:

  • Address the remaining TODOs or phrase them better so other people can address them later
  • Make a proper error reporting system that points to the problem
  • Make sure to document the nontrivial aspects/invariants of the approach
  • Make sure no reordering of computations can occur; you may use a purity check to determine if something can be reordered, whose current implementation can be very primitive (it will be improved later)
  • Document the semantics of @tailrec when ascribing calls and definitions
    We should in fact have both:
    • @tailcall to annotate calls, with the current semantics assigned to @tailrec
    • @tailrec-annotated calls that ensure the recursion back to the current function is tail-recursive from the meeting discussion: just drop these for now and only accept @tailrec on definitions

@LPTK LPTK requested a review from waterlens June 1, 2024 04:03
@CAG2Mark
Copy link
Contributor Author

CAG2Mark commented Jun 1, 2024

Completed:

  • Improved error reporting and propagation of locations.
    • Added compilation errors to the diff tests. Note that this changes the output of some old tests.
    • Also, postProcess now takes a raise: Diagnostic -> Unit parameter, so extensions using postProcess can now raise errors in the same way as tests in the main project.

TODO:

  • Purity check
  • Remaining TODOs and unresolved comments
  • Document semantics and non-trivial aspects

After this it should be good to merge.

@CAG2Mark
Copy link
Contributor Author

CAG2Mark commented Jun 5, 2024

Instead of writing class True and class False in every single test case, why not make it a built-in class info? It should be a trivial change and would remove much clutter. Could also do the same with Cons/Nil and Some/None.

resolved

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants