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

flambda: consider all bound variables when inlining #1614

Merged
merged 6 commits into from Mar 15, 2018

Conversation

Projects
None yet
4 participants
@xclerc
Copy link
Contributor

xclerc commented Feb 15, 2018

When computing the list of free variables, all the functions
from the same let rec construct should be considered, not
only the function that acts as the "entry point" of the inlining.

@xclerc

This comment has been minimized.

Copy link
Contributor Author

xclerc commented Feb 15, 2018

The following example, from @lpw25, exhibits the problem:

let foo bar init =
  let outer_f =
    let baz = bar + 1 in
    let rec inner_f x_in_f y_in_f =
      match x_in_f with
      | Some _ -> inner_g x_in_f (y_in_f - 1)
      | None -> inner_g x_in_f (y_in_f - 2)
    and inner_g x_in_g y_in_g =
      match x_in_g with
      | Some _ -> inner_f x_in_g (y_in_g - baz)
      | None -> inner_f x_in_g (y_in_g - 4)
    in
    inner_f
  in
  let s = Some init in
  (outer_f [@specialised]) s 10

The compiler will raise a fatal error because baz does not
appear in the set of free variables, as it occurs in inner_g
while the "entry point" of the inlining is inner_f.

let function_free_vars = function_decl.free_variables in
let function_params = Parameter.Set.vars function_decl.params in
let bound_variables =
function_free_vars -- function_params -- function_names

This comment has been minimized.

@chambart

chambart Feb 15, 2018

Contributor

This should probably have parentheses to make the intent more obvious.

This comment has been minimized.

@xclerc

xclerc Feb 15, 2018

Author Contributor

Indeed.

@@ -272,7 +302,7 @@ let inline_by_copying_function_declaration ~env ~r
copied. We add these bindings using [Let] around the new
set-of-closures declaration. *)
let free_vars, free_vars_for_lets =
fold_over_projections_of_vars_bound_by_closure ~closure_id_being_applied
fold_over_projections_of_vars_bound_by_closures ~closure_id_being_applied

This comment has been minimized.

@chambart

chambart Feb 15, 2018

Contributor

You should restrict this to the 'required_functions', which are computed just after.

This comment has been minimized.

@xclerc

xclerc Feb 15, 2018

Author Contributor

Done. As a side note, required_functions is probably too large:
it takes into account every function that is referenced while we
are interested in functions such as there is a direct call to them
(will open another PR with this change).

@chambart

This comment has been minimized.

Copy link
Contributor

chambart commented Feb 19, 2018

You should add a Change entry to say that this fixes something. And it's good to go.

acc)
required_functions
Variable.Map.empty)
in

This comment has been minimized.

@chambart

chambart Feb 19, 2018

Contributor

You don't really need to build a closure here just to fold on it. If you find a lighter way to write that it would be a bit better. Otherwise I'm still ok.

@alainfrisch alainfrisch added the bug label Feb 28, 2018

@mshinwell mshinwell merged commit 77de8d6 into ocaml:trunk Mar 15, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
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.