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

Keep more type information in Lambda #2156

Merged
merged 21 commits into from Nov 23, 2018

Conversation

Projects
None yet
4 participants
@alainfrisch
Copy link
Contributor

commented Nov 20, 2018

This PR adds more propagation of "value kinds" (used to drive float unboxing, in particular) through the compiler:

  • This is based on a version of #1029 from @chambart merged with trunk, i.e. keeping value kinds on lambda functions (parameters and return values).

  • Adding value kinds on parameters of staticcatch.

  • Unboxing across staticcatch handlers, as in MPR#7017, but based on value kinds from the item above. (For now, float only, not boxed numbers.)

The first point is somehow independent from the other ones, but (1) it goes in the same direction as the second point; and (2) all together they will allow to make sure that functions compiled "statically" as in #2143 will not require boxing when jumping to a shared continuation.

Variants of #1322 (unboxed calling conventions) and other optimizations could also benefit from keeping more value kinds in lambda/clambda.

The text below does not apply any more to this PR

In its current state, this PR already avoids boxing b in;

let f z x y =
  let a, b =
    if z then (x * 2, y *. 3.)
    else (x, y)
  in
  float a -. b

The tuple binding is compiled to a staticcatch, and this optimization now makes sure to annotate the parameters of the staticcatch with value kinds derived from the pattern type. (This only works without flambda. Extra work will be needed to propagate -- and hopefully enrich -- the value kind annotations through flambda.)

Similarly, the PR avoids some boxing with or-pattern, such as for x in:

type t =
  {
    x: float;
    y: float;
  }

let f = function
  | Some {x; y = 0.} | Some {x = 0.; y = x} -> x +. x
  | _ -> 0.

The unboxing decision for staticcatch parameters (in Cmmgen) is currently only based on value kind annotations (inherit from Lambda). One bad case is when all staticraise sites naturally produce a boxed version (e.g. by calling a float-returning function, or reading from a boxed data structure), and the statichandler can use this boxed version directly. With the new heuristics, those floats will be unboxed and then reboxed when accessed in the handler. This is similar to what happens for float mutable variables, which are always unboxed (independently of the bound expression), and I think one can live with that.~~

chambart and others added some commits Feb 3, 2017

Propagate type information on stub return in the presence on labels
This only makes things more obvious, nothing is gained as some other
information is missing.

@alainfrisch alainfrisch changed the title [WIP] Keep more type information in Lambda Keep more type information in Lambda Nov 20, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

I've edited the description and removed the [WIP] marker. This is ready for review.

@alainfrisch alainfrisch changed the title Keep more type information in Lambda Keep more type information in Lambda, unbox float across statichandlers Nov 20, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 20, 2018

(Note: CI reported on #1029 a problem while compiling Camlp4, because of "empty pattern matching", not allowed by the normal syntax. I've added a commit which should fix this problem. AppVeyor/Travis don't compile Camlp4 anymore.)

alainfrisch added some commits Nov 20, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Commit 998d831 avoids boxing of x in:

type t =
  {
    x: float;
    y: float;
  }

let f = function
  | Some {x; y = 0.} | Some {x = 0.; y = x} -> x +. x
  | _ -> 0.
@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 21, 2018

Would someone be able to review this PR (perhaps @chambart , @lpw25 or @mshinwell)?

Show resolved Hide resolved asmcomp/un_anf.ml Outdated
| _ -> Llet(str, kind, var, exp, body)

let bind str var exp body =
bind_with_value_kind str (var, Pgenval) exp body

This comment has been minimized.

Copy link
@chambart

chambart Nov 22, 2018

Contributor

I've looked at the use points for this function. They are all in Matching. Most of them do have kind information. I think it would probably be better to update the uses rather than the definition.

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 22, 2018

Author Contributor

I've switched two call sites to compute the value_kind, but all others in Matching, there does not seem to be a trivial way to compute the value_kind without more refactoring.

Show resolved Hide resolved bytecomp/translcore.ml Outdated
Show resolved Hide resolved asmcomp/cmmgen.ml Outdated
Show resolved Hide resolved asmcomp/cmmgen.ml Outdated
Show resolved Hide resolved asmcomp/cmmgen.ml Outdated
@chambart

This comment has been minimized.

Copy link
Contributor

commented Nov 22, 2018

I would have separated the propagation/unboxing in 2 PR, but it doesn't really matter.
I had a few remarks, but otherwise I have no objection to the propagation part and the code is ok.
The unboxing part, I think, could have been handled exactly like the non-mutable lets.

@mshinwell

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

Could we please separate the unboxing part from the propagation of kinds in this patch? They seem like distinct features.

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Ok, let's try to get the (tedious but mechanical) propagation stuff first. I've reverted the commit about the actual unboxing and will submit another PR later.

@alainfrisch alainfrisch changed the title Keep more type information in Lambda, unbox float across statichandlers Keep more type information in Lambda Nov 23, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

All comments addressed. I've reviewed the part originally written by @chambart , and @chambart looked at the part I added. @chambart : if you are happy with the current state, can you add a formal review?

let lvars = List.map (fun id -> Lvar id) val_ids in
static_catch (transl_list argl) val_ids
(Matching.for_multiple_match e.exp_loc lvars val_cases partial)
let val_ids =

This comment has been minimized.

Copy link
@chambart

chambart Nov 23, 2018

Contributor

Why did you change the indentation depth here ? I know that 4 is the default, but the rest of the code is 2 here

This comment has been minimized.

Copy link
@alainfrisch

alainfrisch Nov 23, 2018

Author Contributor

I didn't; emacs+ocp-indent did, and I'm a bit too lazy to find how to disable this behavior. You can ask GitHub to hide whitespace changes if this helps reviewing.

@@ -527,7 +527,9 @@ and to_clambda_set_of_closures t env
in
{ label = Compilenv.function_label closure_id;
arity = Flambda_utils.function_arity function_decl;
params = List.map (fun var -> VP.create var) (params @ [env_var]);
params =
List.map (fun var -> VP.create var, Lambda.Pgenval) (params @ [env_var]);

This comment has been minimized.

Copy link
@chambart

chambart Nov 23, 2018

Contributor

check_typo is complaining about this line (81 characters)

@chambart

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

I think it's good to go when the CI is happy (the only failure is check_typo).

@chambart chambart added the approved label Nov 23, 2018

@alainfrisch alainfrisch merged commit 7a746de into ocaml:trunk Nov 23, 2018

0 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
@shindere

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

This breaks the tests/translprim/array_spec.ml test when the
compiler is configured with -no-flat-float-array.

More precisely: there is a difference between the expected and the obtained
output:

  •  (array.length[addr] addr_a) (function a (array.length[addr] a))
    
  •  (array.length[addr] addr_a) (function a : int (array.length[addr] a))
    

Shall I just go ahead and update the reference file or is there something
deeper to do here?

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

No, nothing deep, this simply needs to be updated!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

@alainfrisch

This comment has been minimized.

Copy link
Contributor Author

commented Nov 23, 2018

Thanks! @shindere!

@shindere

This comment has been minimized.

Copy link
Contributor

commented Nov 23, 2018

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.