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

Handle specialisation of recursive function that does not always preserve the arguments (MPR#7291) #780

Closed
wants to merge 4 commits into from

Conversation

Projects
None yet
4 participants
@chambart
Copy link
Contributor

commented Aug 25, 2016

This is a followup to #731

The other solution to fixing MPR#7291 is to drop the requirement from the parameter invariance analysis from being an over-approximation.

This is done by some rewriting of specialised functions before freshening. Every recursive calls that are not obviously preserving the arguments that will be specialised call the original function instead of a recursive call to the specialised version.

This is a bit more complex than originally expected to be able to multiply recursive functions like:

type a =
  | A of b
  | End

and b =
  | B of a

let rec iter_a f a =
  match a with
  | A b ->
    f ();
    iter_b f b
  | End ->
    ()

and iter_b f b =
  match b with
  | B a ->
    f ();
    iter_a f a


let count a =
  let r = ref 0 in
  let increm () = incr r in
  (iter_a [@specialised]) increm a;
  !r

This change is not the most efficient possible: There are 3 added rewritings of the functions.
Some could be eliminated. This is only a first proposal.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

Hmm, I think this might be plausible for 4.04 actually. I'm going to tidy it up a bit.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

I haven't read the patch properly yet, but doesn't this risk adding closure allocations when specializing a function. We're currently careful to only request specialisation when a set of closures has no free variables, whilst this change seems to add the original set of closures as a free variable of the new set.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

@chambart Please see chambart#33 -- I suggest merging that and then re-reading. I haven't got time to test this before going away; could you write some tests (ideally in the testsuite) including a distillation of the original problem from #731? If that all works, I think we should merge this in 4.04 instead of my original hack. I think it would be a mistake not to fix this problem in 4.04.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

@lpw25 Hmm, I'm not sure it will. Given the current behaviour of Invariant_params, is it not the case that the only recursive calls we are going to repoint to the non-specialised version of the function (via these new free variables) are in subfunctions? I think it is. Those subfunctions are presumably referencing the existing function in their closure in any case, so I'm not sure more allocations are added.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

I don't think it matters if the calls are in subfunctions. In order for the old set of closures to be available when we create those subfunctions it is going to need to be a free variable of the new set of closures.

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Aug 26, 2016

Actually, I think this is probably fine. The new free variables are necessarily pointing to closures which themselves have no free variables. Even though flambda doesn't express it, such closures will be statically allocated and so the free variables will disappear, and I think this means there will be no closure allocation.

This inability of flambda to understand that closures with no free variables will always be statically allocated -- even when other scoping issues prevent them from being lifted to symbols -- is quite annoying. At some point it will probably need to be addressed.

There are still some downsides to this approach. For instance, the existence of free variables will prevent the new set of closures from being further specialised.

@chambart chambart force-pushed the chambart:pr7291_bis branch from 28d74d4 to f832428 Aug 31, 2016

chambart and others added some commits Aug 24, 2016

Fix PR#7291
Make Inlining_transform.inline_by_copying_function_declaration robust to
Invariant_params being an overaproximation.

@chambart chambart force-pushed the chambart:pr7291_bis branch from f832428 to 78f3399 Aug 31, 2016

@chambart chambart added bug and removed work-in-progress labels Aug 31, 2016

@chambart chambart added this to the 4.04 milestone Aug 31, 2016

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

I tried to make a self contained test exposing the problem, but there are too many interactions to make a robust one. This clearly ask for something to help writing intricate inlining test cases. It could be done either with an inline attribute telling during which inlining phase to force the inline of a function (here specializing the call site before inlining the calls inside the body of the function). Or a concrete syntax for flambda could do (but would be awful to write with all the attributes everywhere...).

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2016

You are right @lpw25 this is certainly inelegant (at least) to not be able to consider that as closed function even if we certainly know that it will be the case. I am still unsure what would be the right approach with the less downsides on that point.

@damiendoligez This is quite an important bugfix for 4.04 do you approve in principle for something like this at that point in the release ?

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Sep 19, 2016

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Sep 29, 2016

Since it fixes a rather important bug and it only impacts flambda, I have no problem merging this into 4.04, but you guys need to choose between this one and #731.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 5, 2016

@mshinwell @chambart @lpw25
Have you decided between this one and #731 ? Is this reviewed and good to go?

@lpw25

This comment has been minimized.

Copy link
Contributor

commented Oct 5, 2016

I think we are waiting for @chambart to return from vacation before making a decision

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 6, 2016

@damiendoligez Yeah, I'm sorry, we all need to round up on this one again. I think we need to wait until next Monday.

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Oct 6, 2016

Let's say at least tuesday, I will be too wasted on monday to give a sensible answer.

@damiendoligez

This comment has been minimized.

Copy link
Member

commented Oct 7, 2016

No problem.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 13, 2016

@chambart Leo and I discussed this again and we think merging this one is probably best, since it seems more principled and possibly safer. If you are in agreement please go ahead.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 20, 2016

@chambart : Ping

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Oct 24, 2016

This is not completely certain, but https://forge.ocamlcore.org/tracker/index.php?func=detail&aid=1674&group_id=54&atid=291 is probably an instance of this bug.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Oct 25, 2016

We've agreed this is the right way to go, and will merge now.

@chambart chambart changed the base branch from trunk to 4.04 Oct 25, 2016

@chambart chambart changed the base branch from 4.04 to trunk Oct 25, 2016

@mshinwell mshinwell added the approved label Oct 25, 2016

@chambart

This comment has been minimized.

Copy link
Contributor Author

commented Oct 25, 2016

This branch was based on trunk, so this was manually pushed to 4.04

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.