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

Semantic tests via IR #14511

Merged
merged 7 commits into from
Aug 23, 2023
Merged

Semantic tests via IR #14511

merged 7 commits into from
Aug 23, 2023

Conversation

cameel
Copy link
Member

@cameel cameel commented Aug 22, 2023

Resolves #12668.

The PR is a revival of #11949, with some adjustments.

In the end, it turned out that thanks to #13972 none of our tests actually needs the newly introduced settings at the moment. Not even at the minimalStack level. I left it in, but I could also easily remove it by dropping the two middle commits.

The complications I ran into were of a completely different nature: #14500 and one weird failure on homestead which I initially tried to debug but eventually resolved simply by increasing the gas values used in the test.

ekpyron and others added 5 commits August 22, 2023 19:58
…timization level that avoids "Stack too deep"
- After the test framework changes it fails via Yul on Homestead but passes on all other EVM versions and on legacy. The test itself does not seem to be testing these specific numbers so and increasing them resolves the problem.
@cameel cameel self-assigned this Aug 22, 2023
@ekpyron
Copy link
Member

ekpyron commented Aug 22, 2023

In the end, it turned out that thanks to #13972 none of our tests actually needs the newly introduced settings at the moment. Not even at the minimalStack level. I left it in, but I could also easily remove it by dropping the two middle commits.

Nice, I was hoping that'd happen!

The only reason why we may want to keep the setting would be if we ever have a bug involving a recursive function (for which we can't do stack-to-memory) and which will only compile with optimizer... Given that, if it's not too much of a hassle to address #14511 (comment) (if that comment is actually even true), we can keep the setting for now, even though it's currently a bit of an unnecessarily complication. (But I'd also not object to removing it.)

@cameel
Copy link
Member Author

cameel commented Aug 22, 2023

Given that, if it's not too much of a hassle to address #14511 (comment) (if that comment is actually even true), we can keep the setting for now, even though it's currently a bit of an unnecessarily complication. (But I'd also not object to removing it.)

Sure, I have no issue with keeping it and I also already fixed the problem you found.

@cameel
Copy link
Member Author

cameel commented Aug 22, 2023

Some jobs are failing but this looks like some CircleCI breakage, not a problem with the PR:

Starting container cimg/node:current
Warning: No authentication provided, using CircleCI credentials for pulls from Docker Hub.
image cache not found on this host, downloading cimg/node:current

Error response from daemon: unauthorized: authentication required

Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

lgtm

@ekpyron ekpyron merged commit 78b1f5a into develop Aug 23, 2023
@ekpyron ekpyron deleted the semantic-tests-via-ir branch August 23, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run semantics tests using the proper viaIR pipeline and make optimizer requirements explicit in the tests.
3 participants