-
Notifications
You must be signed in to change notification settings - Fork 4
Hotfix pass singleton tuples as single elements to closures #815
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
Hotfix pass singleton tuples as single elements to closures #815
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What about removing this expected file, since it's already reached the #EndProgram.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it does not hurt to keep it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to use prove-rs folder even for concrete testing. Because it only has one file to be added and one test to be ran. exec-smir eliminates the smir.json generation process, but have to execute both llvm and haskell backend.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
All EDIT: The exec_smir testsexec_smir tests that run without step limit
are also run as proofs to prove termination. I thought it is an advantage to run the test in more than one way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed, just think it's a little bit annoying of these file changes (smir.json, .state, test_integration). Additionally, to my experience, the .state only different when there are some issues in the semantics.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree with you @Stevengre and I raised it before and @ehildenb and @jberthold convinced my it was safer to have a dump some intermediate state to compare against. It isn't much more overhead to update with the make commands, and it could potentially uncover a bug in our process if we have unexpected diffs in that intermediate state but the proof itself still passes.
…on-tuples-as-single-elements-to-closures
|
When testing this with p-token, I found that the recognition of closures is still not complete. |
325ba74 to
c6fb79a
Compare
dkcumming
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked at this with close account, and I do see more branching so I believe this is working. Is this meant to remove thunks that we were seeing in p-token, if so which ones? Maybe it would be good to add a description (even if brief) before merging so if we look back we can see what this is in relation to before needing to read the code changes.
Definitely working because the test programs will execute wrongly without the new K code. Will extend the documenting text and mention that flag before I merge. |
This PR implements a special calling convention applied to _closures_: * The first argument is the closure's environment (currently not supported nor are types extracted in stable-mir-json) * The second argument is a _tuple_ that needs to be unpacked and the individual components stored as locals `_2` to `_N`
No description provided.