Fix flambda issues following multicore merge#10909
Conversation
gasche
left a comment
There was a problem hiding this comment.
In general this looks fine, but I have more comments/questions on the lazy-related parts, sorry.
stdlib/camlinternalLazy.ml
Outdated
| (* Assumes [blk] is a block with tag forcing *) | ||
| let do_force_block blk = | ||
| let b = Obj.repr blk in | ||
| let b = Obj.repr (Sys.opaque_identity blk) in |
There was a problem hiding this comment.
This use of Sys.opaque_identity is not explained by a comment. Could you add a comment to explain what is going on?
In fact, the comment you added in 0f30379 has disappeared from trunk, probably dropped during the Multicore rebases. Could you add it back? (I think it makes sense to add the two comments at the same time, they will be more consistent with each other this way.)
There was a problem hiding this comment.
Is this the right place to call Sys.opaque_identity? (And if it is, why isn't do_force_val_block handled in the same way?) My understanding is that this function is consistent with the idea that blk is a mutable block, and that it is only called on values that do respect this assumption -- same thing with force_gen_lazy_block, which would have as precondition that the input is a mutable block, as it unconditionally calls update_to_forcing.
Where is the actual move from "possibly immutable" to "certainly mutable" going on? I think it takes place within the code of force_gen, so this is where Sys.opaque_identity should be placed. But there is one already. Maybe we should add a Popaque primitive in inline_lazy_force_* instead of here?
There was a problem hiding this comment.
Where is the actual move from "possibly immutable" to "certainly mutable" going on? I think it takes place within the code of
force_gen, so this is whereSys.opaque_identityshould be placed. But there is one already. Maybe we should add a Popaque primitive ininline_lazy_force_*instead of here?
That's a good idea. I'll push an update with that solution.
lambda/matching.ml
Outdated
| let call_force_lazy_block varg loc = | ||
| (* The argument is wrapped with [Popaque] to prevent the compiler from making | ||
| any assumptions on its contents. Alternatively, [ap_inlined] could be set | ||
| to [Never_inline]. *) |
There was a problem hiding this comment.
It would be nice to add a reference here to the PRs #9998 and #10909 that go into the details of why we need Popaque.
(I spend a good share of my review energy asking people to explain decisions more in comments. Maybe I'm more often than most in the position of reading year-old compiler code that I'm unfamiliar with, trying to reverse-engineer detailed justifications for the non-obvious things that are going on?)
|
I've added a Changes entry with only the user-visible change (the restriction on warning 59), because I'm not sure where the rest would fit (the testsuite fixes could be considered part of the multicore PR, and I guess the lazy stuff could have an entry in I think it should be safe to re-enable a flambda build in the CI, but it's not part of this PR. |
xavierleroy
left a comment
There was a problem hiding this comment.
LGTM. Thanks for this flambda resurrection. Merging ASAP.
|
You may need to fix the reference output of one of the tests, now that the |
|
You're right. I also forgot to adapt the test for warning 59, so I've opened #10912 with these fixes. |
As discussed in #10878.
The only change from what has been discussed there is that I haven't removed warning 59, but made it conditional on flambda invariant checks (@chambart thinks the check can still be useful).
As a consequence, I've also added a
Sys.opaque_identitycall inCamlinternalLazyto prevent the warning from triggering there even if it is enabled.Closes #10878.