Skip to content

Fix duplicate lifted constants for code of lifted sets of closures#567

Merged
mshinwell merged 7 commits intooxcaml:mainfrom
lthls:fix-simplification-of-lifted-closures
Jun 15, 2022
Merged

Fix duplicate lifted constants for code of lifted sets of closures#567
mshinwell merged 7 commits intooxcaml:mainfrom
lthls:fix-simplification-of-lifted-closures

Conversation

@lthls
Copy link
Copy Markdown
Contributor

@lthls lthls commented Mar 1, 2022

Found while working on #164. The code generated by simplifying a lifted set of closures is already added to the lifted constants of the accumulator in the shared simplify_set_of_closures0 function, so returning it as a new binding in simplify_lifted_set_of_closures0 creates a duplicate binding. If the code and the closure are mutually recursive, this should be handled by the normal mechanism in Lifted_constant_state.

@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Mar 2, 2022

This PR now also contains a relatively big refactoring around the propagation of accumulators, to fix the handling of groups of mutually recursive lifted sets of closures (the dacc was not correctly threaded, so we lost the accumulators from all but the last set of closures).

@mshinwell mshinwell added the flambda2 Prerequisite for, or part of, flambda2 label Mar 2, 2022
@lthls
Copy link
Copy Markdown
Contributor Author

lthls commented Mar 4, 2022

Note: the bugs fixed by this PR cannot be triggered currently, as they only occur when Simplify is called on a term containing lifted sets of closures. We don't plan to support either multiple rounds of Simplify or lifting in closure conversion outside of classic mode, so the fix is not urgent.

@lthls lthls force-pushed the fix-simplification-of-lifted-closures branch from 21a04ab to 3b00f90 Compare June 14, 2022 09:09
@mshinwell mshinwell merged commit 40bdb03 into oxcaml:main Jun 15, 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.

3 participants