Skip to content

Fix layouts in flambda1 partial application simplification #1549

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 2 commits into from
Jul 12, 2023

Conversation

ccasin
Copy link
Contributor

@ccasin ccasin commented Jul 7, 2023

(I'm not confident this is correct, and if it is other problems remain.)

This addresses an flambda1-specific failure I'm seeing over in #1528, which adds unboxed floats to the front-end. In that PR, this program trips an assert in Inline_and_simplify.simplify_partial_application:

let id_float (x : float#) = x

let app f (x : float#) = f x

let id_float' = app id_float

The assert tripped is:

    assert(Lambda.compatible_layout function_decl.A.return_layout result_layout);

This assert is surprising to me. As far as I can tell, result_layout is the result of the application. Since we're looking at a partial application, the result type is a function and the result layout will always be value. On the other hand, the return_layout of the function being partially applied may not be value.

This PR attempts to fix this by dropping the assert and correcting a layout in the resulting function. I can't test it here since there are no other layouts allowed by the front-end. I have tested it in #1528, and it seems to work. But I'm also seeing other flambda1 problems that I'm less sure how to fix, so I don't feel confident - I'll write a slack message about those.

Edit: The things that worried me in the original message did turn out to be a distinct issue, now addressed in #1557

@Ekdohibs
Copy link
Contributor

We might want to replace the existing assert with something like assert (Lambda.compatible_layout Lambda.layout_function result_layout) as well, however.

@ccasin
Copy link
Contributor Author

ccasin commented Jul 10, 2023

We might want to replace the existing assert with something like assert (Lambda.compatible_layout Lambda.layout_function result_layout) as well, however.

Reasonable. I added back the assert, but moved it just before the call to this function, where I think it makes the most sense.

@goldfirere
Copy link
Collaborator

Merging with @Ekdohibs's approval and in @mshinwell's absence.

1 similar comment
@goldfirere
Copy link
Collaborator

Merging with @Ekdohibs's approval and in @mshinwell's absence.

@goldfirere goldfirere merged commit cd8cb89 into ocaml-flambda:main Jul 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants