-
Notifications
You must be signed in to change notification settings - Fork 85
Add unique barriers for Pduprecord, Parraylength, Parrayrefu #3243
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
base: main
Are you sure you want to change the base?
Conversation
Note that the CI currently only fails on |
I think it is not clear to me why these constructs need unique barriers. I would not expect the uniqueness analysis to know anything about |
Ok, I found the answer to my (implicit) question in #3240: array length is used to compile pattern matching on arrays. However, I think this PR is not the right initial solution to that problem: rather than changing compilation to support non-unique pattern matching on arrays, it is almost certainly much easier to make the uniqueness analysis stricter so that it considers such pattern matches as full uses of the array's address. Of course, you've written this PR now, so it's possibly too late to save much work by taking the other approach. Still if there are other cases that still need fixing I would encourage us to first fix them by making the analysis stricter, and then relax the analysis by making changes to compilation later. |
I would worry that this would decrease the usefulness of uniqueness in DRF programming, because now we can't really have an array that holds things that hold keys. |
You can still do that, and you can even pattern match on them, you just need to explicitly borrow the value before you match on it. |
This PR adds unique barriers for
Pduprecord
,Parraylength
andParrayrefu
. It pushes them into flambda2 and adds coeffects if the unique barrier is enabled.May_be_pushed_down
.middle_end/flambda2/simplify/
where I did not know what to do. I have left CRs to be discussed in review.fexpr
, added the ability to print it and make a corresponding parser change. However, bothmake regen-flambda2-tests
andmake runtest
currently fail for me (see slack).