Skip to content

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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

anfelor
Copy link
Contributor

@anfelor anfelor commented Nov 8, 2024

This PR adds unique barriers for Pduprecord, Parraylength and Parrayrefu. It pushes them into flambda2 and adds coeffects if the unique barrier is enabled.

  • When reviewing this, keep in mind that our correctness condition is that a unique barrier is never forgotten. However, if a new instance of the primitive is generated, it will be fine to add a dummy barrier May_be_pushed_down.
  • There are three cases in middle_end/flambda2/simplify/ where I did not know what to do. I have left CRs to be discussed in review.
  • I have added the unique barrier to fexpr, added the ability to print it and make a corresponding parser change. However, both make regen-flambda2-tests and make runtest currently fail for me (see slack).

@anfelor anfelor added flambda2 Prerequisite for, or part of, flambda2 lambda Lambda language changes uniqueness labels Nov 8, 2024
@anfelor anfelor requested a review from mshinwell November 8, 2024 19:14
@anfelor
Copy link
Contributor Author

anfelor commented Nov 8, 2024

Note that the CI currently only fails on tests3.flt as discussed on slack. I would appreciate a pointer about how to fix the failing tests and which test updates to include in this PR.

@anfelor anfelor marked this pull request as ready for review November 11, 2024 16:13
@lpw25
Copy link
Collaborator

lpw25 commented Nov 15, 2024

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 Array.length. Operations the uniqueness analysis does not know about should not need barriers.

@lpw25
Copy link
Collaborator

lpw25 commented Nov 17, 2024

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.

@goldfirere
Copy link
Collaborator

make the uniqueness analysis stricter so that it considers such pattern matches as full uses of the array's address.

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.

@lpw25
Copy link
Collaborator

lpw25 commented Nov 18, 2024

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.

@anfelor anfelor marked this pull request as draft November 25, 2024 21:38
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 lambda Lambda language changes uniqueness
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants