Skip to content

Code review of Simplify #704

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

Merged
merged 91 commits into from
Jul 5, 2022
Merged

Conversation

mshinwell
Copy link
Collaborator

This is from seven days' work in Paris. I believe it is the case that everything here has been looked at by at least two people.

If anyone would like some of this merged as a separate PR, please shout.

One notable thing is that the fixes here permit the removal of identity switches on variants, e.g.:

let f o =
  match o with
  | None -> None
  | Some x -> Some x

will compile to just let f o = o.

Based on #703.

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Jun 28, 2022
@mshinwell
Copy link
Collaborator Author

I will move the entries in the todo file to somewhere else on Github, by the way.

@mshinwell mshinwell force-pushed the flambda2-simplify-review1 branch from 78ef42c to 0950738 Compare June 28, 2022 11:57
@mshinwell mshinwell force-pushed the flambda2-simplify-review1 branch from 022ae00 to 1f54be2 Compare July 5, 2022 16:07
@mshinwell mshinwell force-pushed the flambda2-simplify-review1 branch from 80c8e84 to b4493ba Compare July 5, 2022 16:10
@mshinwell
Copy link
Collaborator Author

mshinwell commented Jul 5, 2022

Notes made in Paris, to be put elsewhere in due course:

Removal of unused value slots - dataflow through all functions
More information about in which branch free names are used, for To_cmm
Problem about rebuilding of let-bindings with Invalid

Poison / loss of information after unboxing variants on second round
(also relates to storing Typing_env_extensions in terms)

Mark to check JS example with value slots

Invariant checks need fixing - maybe a CI job somehow

check_cse_environment in simplify_switch expr is a problem

Rename "lifted constant"

Maybe we can remove prior_cont_uses stuff in simplify_let_cont

Exceptions to jumps: adding to the backtrace buffer, and a less
restrictive algorithm

Might be a problem with the let-symbol for module initialisers
ending up not at toplevel?

Already seems like a problem if Simplify is being re-run as
a continuation containing a let symbol could be inlined into
a non-toplevel position.

Continuation specialisation at toplevel needs care not to run into
trouble with being "at toplevel"

Should we relax the toplevel criterion?
Problem in that join assumes all symbols named on the branches
are present in the joined env

Exception handlers that are converted into jump targets aren't inlined.
Presumably the inlining decision is taken too early

Removed operations aren't taking into account whether the code
might be called.  Better optimisation might be turning into worse
cost metrics.
e.g. two branches and one allocation removed in each branch --
this is considered better than removing one branch plus one
allocation.
May make inlining less predictable

Continuation aliases
let_cont k1 x = ... in
let_cont k2 y = k1 y in
...
apply_cont k2 0
...
apply_cont k2 0

Since the handler of k2 will be simplified because of the typing information
known about y, then it won't be seen as an alias to k1.

Maybe these aliases should be done via Apply_cont_rewrites

Proper way of identifying which continuation parameters are exception buckets

Unboxing of variants for recursive continuations?
Will introduce an extra switch

Maybe try to reduce the number of different code paths for nonrecursive
and recursive continuations in Simplify_let_cont

Can we make switch simplification easier?

Vincent to coordinate comment-writing about slots

Testing with multiple rounds of Simplify

Use In_types mode to track compilation unit-local symbols

Mark to write up lifting proposal without symbols

Remove "lazy"

Removing things from Simplif? - on the list for later

Mark to check what else can be merged from the symbol offsets patch

For contification in the future: Simplify_common.is_self_tail_call
which will be on a separate branch flambda2-contification on gbury's github

Rename perm -> renaming

Was ocamldoc built before the build system changes?

If we had function purity annotations then we could replace Apply_expr
by Apply_cont after reifying the result types.  Otherwise reification of
result types of a function is only going to be useful if it allows
a subsequent allocation to be removed (because the allocation during the
application must remain).

Mark to check that all Cmm operations returning typ_void have
return_unit in To_cmm

Proper way of writing "print" functions

Too much use of get_alias_exn - add an option-returning version?

Row_like should only hold Tag.Scannable

Compile with -opaque

"immediates" in TG should not be Or_unknown

Stdlib: upstream raise

Fixes to Aliases to remove [identity_arms]

Classic mode bug - Mark to investigate reduced test case

@mshinwell mshinwell merged commit d2f2cf7 into oxcaml:main Jul 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
flambda2 Prerequisite for, or part of, flambda2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants