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

Use let expression in copySlice to decrease expression size #489

Merged
merged 1 commit into from
May 13, 2024

Conversation

samalws-tob
Copy link
Contributor

Description

This PR changes copySlice so that it returns an expression of the form (let ((src [value of src])) (store (store [value of dst] 0 (select src 0)) 1 (select src 1)), rather than (store (store [value of dst] 0 (select [value of src] 0)) 1 (select [value of src] 1)). This reduces the number of times src has to be written out in the SMT expression from size times to only 1 time. This can drastically reduce the size of the resulting SMT expression, especially in the case of nested CopySlices.
Fixes #487 .

Checklist

  • tested locally
  • added automated tests
  • updated the docs
  • updated the changelog

@samalws-tob samalws-tob marked this pull request as ready for review April 19, 2024 16:36
Copy link
Collaborator

@arcz arcz left a comment

Choose a reason for hiding this comment

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

I think it looks good and it's a great catch. @d-xo @msooseth @zoep would you take a look too?

Copy link
Collaborator

@zoep zoep left a comment

Choose a reason for hiding this comment

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

This looks good to me too! But I'd like @d-xo or @msooseth to have a look too

@ggrieco-tob
Copy link
Contributor

I'm wondering what is the status of this PR?

@samalws-tob
Copy link
Contributor Author

@ggrieco-tob PR is ready but waiting for review from @d-xo or @msooseth

@d-xo d-xo merged commit 1ccfa8f into ethereum:main May 13, 2024
9 checks passed
@d-xo
Copy link
Collaborator

d-xo commented May 13, 2024

In general a positive review from @zoep or @arcz is imo enough to merge.

@d-xo
Copy link
Collaborator

d-xo commented May 13, 2024

I am a little confused why the common sub expression elimination pass isn't handling this during smt generation?

@zoep
Copy link
Collaborator

zoep commented May 13, 2024

I am a little confused why the common sub expression elimination pass isn't handling this during smt generation?

The copySlice expansion happens post-CSE, which happens at the Expr level, so this repeating subterm will not be eliminated.

Regarding SMT encoding, is there any difference between local let and a top-level constant declaration (e.g., in terms of performance)?

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 this pull request may close these issues.

copySlice can return giant SMT queries from reasonable-sided expressions
5 participants