Skip to content

Remove unnecessary masking of the result of known short instructions #3924

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

Merged
merged 1 commit into from
Apr 20, 2018

Conversation

axic
Copy link
Member

@axic axic commented Apr 18, 2018

Fixes #3920.

@axic axic force-pushed the optim-address-op branch 2 times, most recently from 1b14dca to eb8f830 Compare April 18, 2018 12:38
Copy link
Contributor

@chriseth chriseth left a comment

Choose a reason for hiding this comment

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

Please add tests.

Instruction::COINBASE
})
rules.push_back({
{Instruction::AND, {{u256("0xffffffffffffffffffffffffffffffffffffffff"), op}}},
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add both and(0xfff, x) and and(x, 0xfff)?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, we should, forgot its not doing that anymore on its own.

@chriseth
Copy link
Contributor

We should check how this behaves with regard to ExpressionClasses, since some of these opcodes do not return anything.

Instruction::COINBASE
})
rules.push_back({
{Instruction::AND, {{u256("0xffffffffffffffffffffffffffffffffffffffff"), op}}},
Copy link
Member Author

Choose a reason for hiding this comment

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

Should this instead be (u256(1) << 160) - 1?

Copy link
Contributor

Choose a reason for hiding this comment

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

probably better, yes

@axic
Copy link
Member Author

axic commented Apr 18, 2018

Please add tests.

I was wondering about that, since we only have tests for the peephole. Because other optimisations can be turned on, it is a bit harder to test for things in the simplification list.

@chriseth
Copy link
Contributor

Oh I'm sorry, I confused these operations, they actually all return something.

@chriseth
Copy link
Contributor

Optimiser.cpp and SolidityOptimzer.cpp should be appropriate to add tests.

@axic axic force-pushed the optim-address-op branch from eb8f830 to f27379e Compare April 19, 2018 00:00
Instruction::AND
}, {
u256("0xffffffffffffffffffffffffffffffffffffffff"),
Instruction::SWAP1,
Copy link
Member Author

Choose a reason for hiding this comment

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

This is weird.

Copy link
Contributor

Choose a reason for hiding this comment

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

The semantics are the same (balance takes an argument), although it is weird that it generates sub-optimal code in that case. I think the reason the simplification rule is not applied here is because you did not specify the argument for balance in the rule. We should perhaps add an assertion somewhere that the arity of the operations in the rules is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah sorry, the rule for balance is not there at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, balance is one of those misnamed ones, because it requires the address, should be called extbalance.

However the problem is there it won't check the arity. In the first case AND receives only 1 operand, in the second case SWAP1 applied to a single stack item and AND receives only 1 operand.

Copy link
Member Author

Choose a reason for hiding this comment

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

It is kind of hard to ensure that with dynamic jumps though :)

Copy link
Contributor

Choose a reason for hiding this comment

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

No, it's fine. The first example, rewritten in functional style, where x is the topmost slot before the code starts executing: and(0xffff..fff, balance(x)). After CSE ran over it, eliminating the swap: and(balance(0xfff..fff), x)

Instruction::AND
}, {
op,
u256(1234),
Copy link
Member Author

Choose a reason for hiding this comment

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

It always seems to swap operands.

Copy link
Contributor

Choose a reason for hiding this comment

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

CSE does not guarantee any order of operands for commutative operations (although it is of course deterministic). During analysis phase, everything is normalized and in the code-generation phase, the whole thing is built from scratch.

@axic axic force-pushed the optim-address-op branch 3 times, most recently from 4c594db to 7b2a20c Compare April 19, 2018 08:43
@chriseth
Copy link
Contributor

Test seems to fail.

@axic
Copy link
Member Author

axic commented Apr 19, 2018

Do you have any idea why? The code looks ok, but I cannot test it due to cpp-ethereum still not working properly on mac.

@axic
Copy link
Member Author

axic commented Apr 19, 2018

I wouldn't rush this into 0.4.23 if we are releasing it today.

@axic axic force-pushed the optim-address-op branch from 7b2a20c to 38460d8 Compare April 19, 2018 22:59
@axic
Copy link
Member Author

axic commented Apr 19, 2018

Test seems to fail.

Fixed. It was missing the payable constructor.

@chriseth chriseth merged commit 0f32843 into develop Apr 20, 2018
@axic axic deleted the optim-address-op branch April 20, 2018 09:23
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.

2 participants