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

Suggestion: Val.of_bool and partial evaluation #625

Closed
roconnor-blockstream opened this issue Aug 22, 2022 · 10 comments · Fixed by #661
Closed

Suggestion: Val.of_bool and partial evaluation #625

roconnor-blockstream opened this issue Aug 22, 2022 · 10 comments · Fixed by #661

Comments

@roconnor-blockstream
Copy link
Contributor

If Val.of_bool b were redefined as Vint (if b then Int.one else Int.zero) or even better Vint (Int.repr (Z.b2z b)), then more partial evaluation could proceed because the constructor Vint is exposed.

I think this would be particularly helpful because (at least for me) Val.of_bool ends up occuring inside force_val (both_int or similar such expressions when it appears, which can reduce away as long as Val.of_bool can be reduced to a Vint.

@andrew-appel
Copy link
Collaborator

Interesting. I can't literally change the definition of Val.of_bool, because Val is actually compcert.common.Values.Val.of_bool, and I can't change things inside CompCert.

And I can't simply replace using Val.of_bool with some better definition of my choosing, because some of those uses may come directly from the CompCert Clight semantics.

But what might be possible is to replace all the places where VST introduces an of_bool that did not come directly from CompCert, with the better definition. Perhaps worth an experiment.

@roconnor-blockstream
Copy link
Contributor Author

Ah, I didn't realize that Val was part of Compcert.

andrew-appel added a commit that referenced this issue Feb 2, 2023
This P.R. implements the suggestion in issue #625.
It's an experiment, as in some cases it breaks existing VST proofs
at `forward_if` and related tactics.  The changes required in those
proofs are generally positive (simplifies proof scripts).
See further discussion at #625.  closes #625

Incidentally and unrelated:  closes #660
@andrew-appel
Copy link
Collaborator

I have now implemented the thing I described above in my previous comment. You can find it in P.R. #661. In connection with this, I also improved the efficiency of the do_repr_inj tactic that is used by forward_if (and similar tactics) to clean up conditionals in the then- and else-branch.

Because (in some cases) conditionals are cleaned up better than they were before, some user proofs may need to be modified. You can see examples here (in files other than in floyd/ and veric/ ). Is it worth merging this improvement, given the incompatibility?

@roconnor-blockstream
Copy link
Contributor Author

One option is to wait a few weeks and see of the AbsInt folks are interested in the change before deciding anything.

@andrew-appel
Copy link
Collaborator

That is not the difficulty. It doesn't matter that much whether CompCert uses Val.of_bool in its own Clight2 semantics, because it was not hard to adapt the Verifiable C proof rules to use a different (but provably equivalent) definition, and prove it sound. The only difficult is that it causes incompatibilities, in rare but real cases, that VST users will have to adapt to, if I merge the PR.

@roconnor-blockstream
Copy link
Contributor Author

roconnor-blockstream commented Feb 3, 2023

If AbsInt were to change its definition of Val.of_bool, then VST and VST users would both need to adapt their codebases to such a change.

@andrew-appel
Copy link
Collaborator

Well, yes, but that's not my point. In the PR, I have already done 90% of the work of adapting to that change. The question of whether we should ask VST users to adapt to this change is completely independent of whether AbsInt decides to change. That's what my PR demonstrates. So we are back to the pros and cons:

  • pro: Your suggested "bool2val" is, indeed, more elegant, and leads to (a) more efficient VST processing of the usual cases, and (b) in the corner cases where the user has to deal with it, a more tractable hypothesis (for exactly the reasons you explained in your original issue report).
  • con: Some existing VST proofs will have to be modified at the next VST release.

@lennartberinger
Copy link
Collaborator

If the changes are not too burdensome (and perhaps they are not, the effects should be rather local) I guess the "con" isn't too bad - and we've occasionally asked users to adjust their proofs anyway as VST (or CompCert/Coq underneath) evolves.

@roconnor-blockstream
Copy link
Contributor Author

From my perspective this is similar to PR #242, where partial evaluation was unlocked for reptype in a similar way (by commuting match expressions inside branches to expose an existsT constructor). That PR likely also required user to modify some existing VST proofs.

@andrew-appel
Copy link
Collaborator

Right. So, once I get my PR fully debugged, I'll go ahead and merge it.

andrew-appel added a commit that referenced this issue Feb 7, 2023
* Use (new) bool2val instead of Val.of_bool

This P.R. implements the suggestion in issue #625.
It's an experiment, as in some cases it breaks existing VST proofs
at `forward_if` and related tactics.  The changes required in those
proofs are generally positive (simplifies proof scripts).
See further discussion at #625.  closes #625

Incidentally and unrelated:  closes #660

* Adjustment for 32/64-bit portability
* Fix memory blowup; bring verif_strlib up to date in 64-bit mode
* Fix up a proof in tweetnacl
* Bring mailbox up to date with bool2val
* simplify_new_temp in fwd_result, in connection with new bool2val stuff
* Flesh out simplify_new_temp hint DB; bring 32-bit examples up to date
* Update CHANGES file
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants