Skip to content

Fix bug in unboxing decision code + use max unboxing depth cli flag #446

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 9 commits into from
May 26, 2021

Conversation

Gbury
Copy link

@Gbury Gbury commented May 10, 2021

No description provided.

@Gbury
Copy link
Author

Gbury commented May 10, 2021

So, in this case, CI found a bug with the new implementation of unboxing (this PR triggers it because it makes the default max unboxing depth go from 1 to 3). I think I see what the bug is, I'm working on fixing it.

@Gbury Gbury force-pushed the use_max_unboxing_depth branch from b5a0333 to fbf4b71 Compare May 10, 2021 13:55
@Gbury
Copy link
Author

Gbury commented May 10, 2021

This PR now also fixes a bug in the unboxing code, see the commit message.

@Gbury Gbury changed the title Use the max_unboxing_depth cli flag Fix bug in unboxing decision code + use max unboxing depth cli flag May 10, 2021
@mshinwell
Copy link

@Gbury Please can you discuss the typing env changes with @lthls and also have @chambart approve the unboxing-related changes.

It looks like some of the meet tests have been accidentally commented out in this diff?

@mshinwell mshinwell requested review from lthls and chambart May 17, 2021 17:14
@Gbury
Copy link
Author

Gbury commented May 18, 2021

Sure, though in practice, except for the invariant code for aliases, all of the typing_env change actually come from one of the commits of @lthls in #316
The commented out tests are indeed an error.

@lthls
Copy link

lthls commented May 19, 2021

The typing env changes are correct, and @Gbury and @chambart explained to me why the new unboxing code triggers a bug that needs this patch (at the time I wrote the patch, I wasn't sure how to actually trigger the bug, so I left it in the draft PR and forgot about it).

Gbury and others added 9 commits May 25, 2021 13:35
In some cases, a generated poison values for an unboxed arg was later
"transformed" into an available simple pointing to the "invalid"
constant for the adequate kind. This could bring problem where a
constant value is then passed to a deeper decisions that expects
a variant with no constant constructors.
@Gbury Gbury force-pushed the use_max_unboxing_depth branch from d3297bb to ac252ef Compare May 25, 2021 12:05
@Gbury
Copy link
Author

Gbury commented May 25, 2021

This has been rebased, so it should be good for merging once CI is green.

@Gbury Gbury merged commit 1a0101e into ocaml-flambda:flambda2.0-stable May 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants