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

Unrecursify in closure conversion #456

Open
wants to merge 2 commits into
base: flambda2.0-stable
Choose a base branch
from

Conversation

chambart
Copy link

Add a transformation to closure_conversion to turn self tail calls into apply_cont.
It is far simpler to do that in closure_conversion right now than during simplification, but this might miss a few interesting cases. I don't think that they occur often enough to bother too much.
This does not handle multiply recursive functions. If we want to handle that properly we might need to exend let_rec_cont. But this is not strictly required if we allow an intermediate dispatch.

Much of the (rather little) complexity comes from handling tupled functions.

Right now this does not allow to improve code a lot because transformed functions are not marked as non-recursive, preventing inlining.

@lpw25
Copy link

lpw25 commented May 20, 2021

Right now this does not allow to improve code a lot because transformed functions are not marked as non-recursive, preventing inlining.

No need to do anything now, but at some point we will probably need to mark the functions produced by this transformation in some way, so that we can make [@inline] and [@specialise] behave the same as if the transformation hadn't been applied.

@mshinwell
Copy link

Also, we should try to clear the recursive flag after contification, otherwise we won't get the benefit of this replacing specialisation in certain cases. @chambart that shouldn't be too hard, I hope?

@lpw25
Copy link

lpw25 commented May 21, 2021

Thinking about it some more, I'm not sure it is a good idea to do this change. Firstly, note that this is not contification -- it isn't replacing a function with a continuation, it is adding a new continuation that is only used for the recursive calls.

It is changing:

let foo x y z =
  ...
  foo x y w

into

let foo x y z =
  let cont rec tail x' y' z' =
    ...
    goto tail x' y' w
  in
  goto tail x y z

Before this transformation:

  • inlining foo a b c would copy the body of foo and allow its body to be specialised based on x = a, y = b, z = c with its recursive calls still going to the unspecialised original version.
  • specialising foo a b c -- which isn't currently supported in flambda2 -- would copy the definition of foo and allow its body to be specialised based on x = a, y = b, i.e. the invariant parameters, and recursive calls would go to this new definition.

After this transformation:

  • inlining foo a b c will copy the body of foo and allow its body to be specialised based on x = a, y = b, z = c, however this will not allow the continuation parameters to be specialised at all, since the continuation is recursive, so there is no actual specialisation happening.
  • specialisation would not be available as the function is not recursive anymore.

So from an inlining perspective this transformation makes things strictly worse. I guess there is some benefit now that there is support for unboxing things around recursive continuations, but I don't think we should be universally applying a transformation that makes some optimisations worse and some better. The unboxing gains could be got more effectively by doing a worker-wrapper transformation anyway.

@chambart
Copy link
Author

Also, we should try to clear the recursive flag after contification, otherwise we won't get the benefit of this replacing specialisation in certain cases. @chambart that shouldn't be too hard, I hope?

This isn't too hard in closure_conversion. We just have to check whether my_closure is used. During simplify it is a bit more complex (but not that much) because we also have to check that the symbol is not used for empty closures

@chambart
Copy link
Author

@lpw25 I don't think that specialisation would be worse after that simplification. Or at least harder to do since it is not done right now. The annotation on the function could easilly be transfered to the continuation. And it is probably a bit easier to detect specialisation worthy situations on continuations than on functions. But specialisation would have to be doable on non-tail recursive functions anyway, so this is not going to replace the general case.
I think that this is simple enought that we can have it right now and remove it once we have some function argument/return unboxing that subsume it.

@lpw25
Copy link

lpw25 commented May 21, 2021

It's not just specialisation that is worse, inlining is worse too. This transformation essentially disables both.

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

Successfully merging this pull request may close these issues.

3 participants