Join for Pfield to allow optimisation of matches broken by PR #88 #579
Conversation
| @@ -539,6 +553,41 @@ let make_key e = | |||
| Some (tr_rec Ident.empty e) | |||
| with Not_simple -> None | |||
|
|
|||
| let join_actions_to_be_shared ~printer:_ lam1 lam2 = | |||
There was a problem hiding this comment.
Why are we catching such a specific case rather than merge any lambda terms that differ only in read semantics?
| | C r -> r.x | ||
| *) | ||
| let sem = join_field_read_semantics sem1 sem2 in | ||
| let scoped_loc = |
There was a problem hiding this comment.
Can't we just always take the debuginfo -- and indeed everything except the read semantics -- from the first one?
| @@ -539,6 +553,41 @@ let make_key e = | |||
| Some (tr_rec Ident.empty e) | |||
| with Not_simple -> None | |||
|
|
|||
| let join_actions_to_be_shared ~printer:_ lam1 lam2 = | |||
There was a problem hiding this comment.
What is the unused printer parameter for?
| joined when constructing a shared action (see below). *) | ||
| match p with | ||
| | Pfield (index, _sem) -> | ||
| Pfield (index, Reads_agree (* arbitrary *) ) |
There was a problem hiding this comment.
Whilst I agree it is basically arbitrary, I think that using Reads_vary is slightly better because it means that the key is actually a safe replacement for any of the actions that generate it.
| @@ -516,6 +516,7 @@ module Storer = | |||
| Switch.Store | |||
| (struct type t = lambda type key = lambda | |||
| let compare_key = Stdlib.compare | |||
| let join_actions_to_be_shared lam _ = lam | |||
There was a problem hiding this comment.
I think this should be the Lambda.join_actions_to_be_shared. It happens to be fine now, since the read semantics are ignored in bytecode, but if that changes I bet no one will think to change this line.
| type intern = | ||
| { mutable map : (bool * int) AMap.t ; | ||
| mutable next : int ; | ||
| mutable acts : (bool * A.t) list; } | ||
| acts : act Int.Tbl.t; } |
There was a problem hiding this comment.
I would have thought it was a little simpler to use:
type act =
{ mutable shared : bool;
mutable act : A.t;
}
type intern =
{ mutable map : act AMap.t ;
mutable next : int ;
mutable acts : act list; }Then you can just look up the key in the map and edit the action and sharedness, and at the end you can just read the results straight off the list.
|
This PR will have negative consequences in some cases (with Flambda 2). Adding to the example in the description: let two = (get[@inlined]) (A { x = 2 })This will not simplify to |
PR #88 broke an optimisation which is being relied on at JS. For code like the following:
the
Lambdacode should be a singlePfield. Unfortunately after PR #88 this doesn't happen because of the differences in the "field read semantics". It should be noted that even prior to PR #88 the pattern was fragile: inline record matches{ x }did not optimise as desired.This is a bit of a nuisance to fix because of how sharing keys are used in
Lambdaand I experimented with a couple of approaches (one involving a different comparison function on sharing keys, which I abandoned as it would have added a lot of boilerplate code). I think the current approach here seems reasonable.@lpw25 has volunteered to review this in the first instance.