Sitelet https://web.archive.org/web/20201113050219/https://github.com/ethereum/solidity/pull/8958
Skip to content

/ solidity

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

Jump to bottom

[yul] Add support for EVM version-dependent rules. #8958

Merged
merged 1 commit into from May 27, 2020
Merged

[yul] Add support for EVM version-dependent rules. #8958

merged 1 commit into from May 27, 2020
+88 −11

Conversation

@aarlt
Copy link
Member

@aarlt aarlt commented May 16, 2020 •
edited

Fixes #7542, #8882
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from 6c87973 to 99768c2 May 16, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@aarlt aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from 5061893 to 0796771 May 17, 2020
@elenadimitrova elenadimitrova added this to In progress in Solidity via automation May 18, 2020
@chriseth
chriseth reviewed May 18, 2020
View changes
libyul/optimiser/SimplificationRules.h Show resolved Hide resolved
@chriseth
chriseth reviewed May 18, 2020
View changes
libyul/optimiser/SimplificationRules.h Outdated Show resolved Hide resolved
@chriseth
chriseth reviewed May 18, 2020
View changes
libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the evm-version-dependent-rules branch 3 times, most recently from 54b85e5 to 590822c May 18, 2020
@aarlt aarlt marked this pull request as ready for review May 18, 2020
@aarlt aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from 114c81d to 17cd482 May 19, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@stackenbotten
stackenbotten reviewed May 19, 2020
View changes
libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved
@stackenbotten
stackenbotten reviewed May 19, 2020
View changes
libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from 17cd482 to 8341948 May 19, 2020
@chriseth
chriseth requested changes May 19, 2020
View changes
Copy link
Contributor

@chriseth chriseth left a comment

This is still a little too complicated for my taste and only works for libyul and not the classic optimizer.

Can you change the functions in RuleList.h to accept an EVMVersion parameter and then - in a regular simplificationRuleListPartN function add

if (_evmVersion.hasSelfbalance())
  rules.push_back(...);

Please not that it should be hasSelfbalance and not a comparison for istanbul!
Solidity automation moved this from In progress to Review in progress May 19, 2020
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from 8341948 to fece50c May 19, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from fece50c to e847940 May 19, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from e847940 to ab36603 May 19, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@aarlt aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from db79174 to 2ba82cc May 19, 2020
@chriseth
chriseth reviewed May 19, 2020
View changes
libevmasm/RuleList.h Outdated
/// @returns a list of generic simplification rules with additional rules that may apply for the given dialect.
template <class Pattern>
std::vector<SimplificationRule<Pattern>> simplificationRuleList(
yul::Dialect const& _dialect,

This comment has been minimized.

Sign in to view
@chriseth

chriseth May 19, 2020
Contributor

Ah! Even better!
@chriseth
chriseth reviewed May 19, 2020
View changes
libyul/optimiser/SimplificationRules.cpp Outdated
@@ -46,7 +46,7 @@ SimplificationRule<yul::Pattern> const* SimplificationRules::findFirstMatch(
if (!instruction)
return nullptr;

static SimplificationRules rules;
static SimplificationRules rules(_dialect);

This comment has been minimized.

Sign in to view
@chriseth

chriseth May 19, 2020
Contributor

This way, the rules list will always use the dialect it was first initialized with. You need a static (maybe LazyInit) variable of type std::map<EVMVersion, SimplificationRules>.

This comment has been minimized.

Sign in to view
@aarlt

aarlt May 19, 2020
Author Member

Ah ok.. I thought that the compiler will only be initialized with exactly one EVMVersion..

This comment has been minimized.

Sign in to view
@chriseth

chriseth May 19, 2020
Contributor

Never be sure about that :)
@chriseth
chriseth reviewed May 19, 2020
View changes
libevmasm/RuleList.h Outdated
Pattern Z
)
{
std::vector<SimplificationRule<Pattern>> rules{simplificationRuleList(A, B, C, W, X, Y, Z)};

This comment has been minimized.

Sign in to view
@chriseth

chriseth May 19, 2020
Contributor

Why don't you change that function instead?

This comment has been minimized.

Sign in to view
@aarlt

aarlt May 19, 2020
Author Member

I thought it may make sense to separate evm version specific rules from generic rules

This comment has been minimized.

Sign in to view
@chriseth

chriseth May 19, 2020
Contributor

In a way, we already have a lot of them mixed in, but they do not matter because they do not introduce a new opcode.

This comment has been minimized.

Sign in to view
@aarlt

aarlt May 19, 2020
Author Member

Ah oki, I will change the function.
@aarlt aarlt force-pushed the evm-version-dependent-rules branch from 2ba82cc to 6753cb5 May 19, 2020
@stackenbotten

This comment has been hidden.

Sign in to view
@stackenbotten
stackenbotten reviewed May 19, 2020
View changes
libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved
@aarlt aarlt force-pushed the evm-version-dependent-rules branch 5 times, most recently from 75982d0 to b000af2 May 19, 2020
@chriseth
Copy link
Contributor

@chriseth chriseth commented May 20, 2020

This is still missing the changes in the old optimizer.
@chriseth
Copy link
Contributor

@chriseth chriseth commented May 20, 2020

I'm working on this.
@chriseth chriseth force-pushed the evm-version-dependent-rules branch from b000af2 to a5bedf3 May 20, 2020
@leonardoalt
leonardoalt reviewed May 26, 2020
View changes
Copy link
Member

@leonardoalt leonardoalt left a comment

It'd be fun to add a test like

{
    let a := address()
    let ret := balance(a)
}
// ====
// EVMVersion: >=istanbul
// ----
// step: expressionSimplifier
//
// { let ret := selfbalance() }

Does the rule list work before or after the Yul optimizers in that case?
libyul/optimiser/SimplificationRules.cpp Outdated
if (yul::EVMDialect const* evmDialect = dynamic_cast<yul::EVMDialect const*>(&_dialect))
version = evmDialect->evmVersion();

if (evmRules.find(version) == evmRules.end())

This comment has been minimized.

Sign in to view
@leonardoalt

leonardoalt May 26, 2020
Member

Suggested change
if (evmRules.find(version) == evmRules.end())
if (!evmRules.count(version))
@chriseth
Copy link
Contributor

@chriseth chriseth commented May 27, 2020

Oh sorry, forgot to comment here: It turned out that this is not really worth it for the old optimizer, because the rules cannot be used with the old optimizer. The old optimizer can only have rules with instructions that are deterministic. Since balance depends on previous calls, it is excluded.
@aarlt @chriseth
[yul] Add support for EVM version-dependent rules.
Loading status checks…
a7b8906
@chriseth chriseth force-pushed the evm-version-dependent-rules branch from a5bedf3 to a7b8906 May 27, 2020
@chriseth
Copy link
Contributor

@chriseth chriseth commented May 27, 2020

Added the fun test.
@leonardoalt
leonardoalt approved these changes May 27, 2020
View changes
@chriseth chriseth merged commit a06ac0f into develop May 27, 2020
33 of 36 checks passed
33 of 36 checks passed
ci/circleci: t_ubu_asan_constantinople CircleCI is running your tests
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci: b_archlinux Your tests passed on CircleCI!
Details
ci/circleci: b_docs Your tests passed on CircleCI!
Details
ci/circleci: b_ems Your tests passed on CircleCI!
Details
ci/circleci: b_osx Your tests passed on CircleCI!
Details
ci/circleci: b_ubu Your tests passed on CircleCI!
Details
ci/circleci: b_ubu18 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_asan Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_asan_clang Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_clang Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_cxx20 Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_ossfuzz Your tests passed on CircleCI!
Details
ci/circleci: b_ubu_release Your tests passed on CircleCI!
Details
ci/circleci: chk_antlr_grammar Your tests passed on CircleCI!
Details
ci/circleci: chk_buglist Your tests passed on CircleCI!
Details
ci/circleci: chk_coding_style Your tests passed on CircleCI!
Details
ci/circleci: chk_docs_pragma_min_version Your tests passed on CircleCI!
Details
ci/circleci: chk_proofs Your tests passed on CircleCI!
Details
ci/circleci: chk_pylint Your tests passed on CircleCI!
Details
ci/circleci: chk_spelling Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_colony Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_gnosis Your tests passed on CircleCI!
Details
ci/circleci: t_ems_compile_ext_zeppelin Your tests passed on CircleCI!
Details
ci/circleci: t_ems_solcjs Your tests passed on CircleCI!
Details
ci/circleci: t_osx_cli Your tests passed on CircleCI!
Details
ci/circleci: t_osx_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_asan_constantinople_clang Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_clang_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_cli Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_release_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_soltest Your tests passed on CircleCI!
Details
ci/circleci: t_ubu_soltest_enforce_yul Your tests passed on CircleCI!
Details
Solidity automation moved this from Review in progress to Done May 27, 2020
@chriseth chriseth deleted the evm-version-dependent-rules branch May 27, 2020
@chriseth chriseth mentioned this pull request Jul 21, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth

@stackenbotten stackenbotten

@leonardoalt leonardoalt

Assignees
No one assigned
Labels
None yet
Projects
Solidity
  
Done
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

[Yul] EVMVersion-dependent rules
4 participants
@aarlt @stackenbotten @chriseth @leonardoalt
You can’t perform that action at this time.