[yul] Add support for EVM version-dependent rules. by aarlt · Pull Request #8958 · argotorg/solidity

aarlt · 2020-05-16T00:01:37Z

Fixes #7542, #8882

libyul/optimiser/SimplificationRules.h

libyul/optimiser/SimplificationRules.cpp

chriseth

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!

chriseth · 2020-05-19T17:52:13Z

libevmasm/RuleList.h

+/// @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,


Ah! Even better!

chriseth · 2020-05-19T17:57:43Z

libyul/optimiser/SimplificationRules.cpp

 		return nullptr;

-	static SimplificationRules rules;
+	static SimplificationRules rules(_dialect);


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>.

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

Never be sure about that :)

chriseth · 2020-05-19T17:59:24Z

libevmasm/RuleList.h

+	Pattern Z
+)
+{
+	std::vector<SimplificationRule<Pattern>> rules{simplificationRuleList(A, B, C, W, X, Y, Z)};


Why don't you change that function instead?

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

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.

Ah oki, I will change the function.

libyul/optimiser/SimplificationRules.cpp

chriseth · 2020-05-20T07:36:02Z

This is still missing the changes in the old optimizer.

chriseth · 2020-05-20T09:52:30Z

I'm working on this.

leonardoalt

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?

leonardoalt · 2020-05-26T21:00:40Z

libyul/optimiser/SimplificationRules.cpp

+	if (yul::EVMDialect const* evmDialect = dynamic_cast<yul::EVMDialect const*>(&_dialect))
+		version = evmDialect->evmVersion();
+
+	if (evmRules.find(version) == evmRules.end())


Suggested change

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

if (!evmRules.count(version))

chriseth · 2020-05-27T07:20:36Z

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.

chriseth · 2020-05-27T09:57:24Z

Added the fun test.

aarlt force-pushed the evm-version-dependent-rules branch from 6c87973 to 99768c2 Compare May 16, 2020 17:41

This comment has been minimized.

Sign in to view

aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from 5061893 to 0796771 Compare May 17, 2020 14:47

chriseth reviewed May 18, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.h Show resolved Hide resolved

chriseth reviewed May 18, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.h Outdated Show resolved Hide resolved

chriseth reviewed May 18, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the evm-version-dependent-rules branch 3 times, most recently from 54b85e5 to 590822c Compare May 18, 2020 14:22

aarlt marked this pull request as ready for review May 18, 2020 16:40

aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from 114c81d to 17cd482 Compare May 19, 2020 05:26

This comment has been minimized.

Sign in to view

stackenbotten reviewed May 19, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved

stackenbotten reviewed May 19, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the evm-version-dependent-rules branch from 17cd482 to 8341948 Compare May 19, 2020 05:36

chriseth suggested changes May 19, 2020

View reviewed changes

aarlt force-pushed the evm-version-dependent-rules branch from 8341948 to fece50c Compare May 19, 2020 16:41

This comment has been minimized.

Sign in to view

aarlt force-pushed the evm-version-dependent-rules branch from fece50c to e847940 Compare May 19, 2020 16:45

This comment has been minimized.

Sign in to view

aarlt force-pushed the evm-version-dependent-rules branch from e847940 to ab36603 Compare May 19, 2020 16:47

This comment has been minimized.

Sign in to view

aarlt force-pushed the evm-version-dependent-rules branch 2 times, most recently from db79174 to 2ba82cc Compare May 19, 2020 17:14

chriseth reviewed May 19, 2020

View reviewed changes

aarlt force-pushed the evm-version-dependent-rules branch from 2ba82cc to 6753cb5 Compare May 19, 2020 19:27

This comment has been minimized.

Sign in to view

stackenbotten reviewed May 19, 2020

View reviewed changes

libyul/optimiser/SimplificationRules.cpp Outdated Show resolved Hide resolved

aarlt force-pushed the evm-version-dependent-rules branch 5 times, most recently from 75982d0 to b000af2 Compare May 19, 2020 21:18

chriseth force-pushed the evm-version-dependent-rules branch from b000af2 to a5bedf3 Compare May 20, 2020 10:40

leonardoalt reviewed May 26, 2020

View reviewed changes

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

a7b8906

chriseth force-pushed the evm-version-dependent-rules branch from a5bedf3 to a7b8906 Compare May 27, 2020 09:57

leonardoalt approved these changes May 27, 2020

View reviewed changes

chriseth merged commit a06ac0f into develop May 27, 2020

chriseth deleted the evm-version-dependent-rules branch May 27, 2020 10:52

chriseth mentioned this pull request Jul 21, 2020

[Sol-Yul] Support selfbalance opcode #8882

Closed

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

Conversation

aarlt commented May 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

Uh oh!

This comment has been minimized.

Uh oh!

Uh oh!

chriseth left a comment

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

Uh oh!

chriseth commented May 20, 2020

Uh oh!

chriseth commented May 20, 2020

Uh oh!

leonardoalt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chriseth commented May 27, 2020

Uh oh!

chriseth commented May 27, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

aarlt commented May 16, 2020 •

edited

Loading