Skip to content
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

Redo UPLC purity analysis to match PIR #5541

Merged
merged 1 commit into from Sep 25, 2023
Merged

Conversation

michaelpj
Copy link
Contributor

@michaelpj michaelpj commented Sep 14, 2023

This is the UPLC counterpart of the PIR work. Broadly small improvements.

The new logic almost entirely subsumes the old delayAndVarIsPure logic. There is one exception: we would effectively propagate the "forcing" context through several steps of evaluation, which is quite tricky, and the current code does not try to do. Now we only go into the body of a delay if it is immediately enclosed by a force. I don't think this is a huge loss, though.

Also this finally fixes the broken testcase (it's still weird, but now it's just GHC's fault).

Pre-submit checklist:

  • Branch
    • Tests are provided (if possible)
    • Commit sequence broadly makes sense
    • Key commits have useful messages
    • Changelog fragments have been written (if appropriate)
    • Relevant tickets are mentioned in commit messages
    • Formatting, PNG optimization, etc. are updated
  • PR
    • (For external contributions) Corresponding issue exists and is linked in the description
    • Targeting master unless this is a cherry-pick backport
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

@@ -1,9 +1,9 @@
(lam
b_5
[
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the case that gets worse. I think this is pretty subtle and I'm not too sad that we don't see this.

@michaelpj
Copy link
Contributor Author

(I think this is covered by the previous changelog entry)

@michaelpj michaelpj added the No Changelog Required Add this to skip the Changelog Check label Sep 14, 2023
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think the <> short circuits once it hits the first Unknown does it? It could build a much longer DList than needed, which then needs to be converted to a regular list in linear time, before it can be inspected.

@michaelpj
Copy link
Contributor Author

There's a test in the PIR version that the whole thing is lazy enough: it successfully processes a term with an undefined subterm, which it couldn't do if it was too strict. Note that it also can't be creating the "spine" of the DList, since knowing what the spine looks like requires analyzing later terms, which we know it doesn't.

Which reminds me, I should port the tests over too.

Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that it also can't be creating the "spine" of the DList, since knowing what the spine looks like requires analyzing later terms, which we know it doesn't.

Yeah, you are right.

@michaelpj michaelpj enabled auto-merge (squash) September 25, 2023 14:17
@michaelpj michaelpj enabled auto-merge (squash) September 25, 2023 15:49
@michaelpj michaelpj merged commit 8425a88 into master Sep 25, 2023
6 of 7 checks passed
@michaelpj michaelpj deleted the mpj/uplc-evaluation-order branch September 25, 2023 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
No Changelog Required Add this to skip the Changelog Check
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants