Remove unnecessary masking of the result of known short instructions #3924
Conversation
axic
force-pushed
the
optim-address-op
branch
2 times, most recently
from
1b14dca to
eb8f830
Compare
libevmasm/RuleList.h
Outdated
| Instruction::COINBASE | ||
| }) | ||
| rules.push_back({ | ||
| {Instruction::AND, {{u256("0xffffffffffffffffffffffffffffffffffffffff"), op}}}, |
There was a problem hiding this comment.
Do we need to add both and(0xfff, x) and and(x, 0xfff)?
There was a problem hiding this comment.
Yeah, we should, forgot its not doing that anymore on its own.
|
We should check how this behaves with regard to |
libevmasm/RuleList.h
Outdated
| Instruction::COINBASE | ||
| }) | ||
| rules.push_back({ | ||
| {Instruction::AND, {{u256("0xffffffffffffffffffffffffffffffffffffffff"), op}}}, |
There was a problem hiding this comment.
Should this instead be (u256(1) << 160) - 1?
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. |
|
Oh I'm sorry, I confused these operations, they actually all return something. |
|
Optimiser.cpp and SolidityOptimzer.cpp should be appropriate to add tests. |
test/libevmasm/Optimiser.cpp
Outdated
| Instruction::AND | ||
| }, { | ||
| u256("0xffffffffffffffffffffffffffffffffffffffff"), | ||
| Instruction::SWAP1, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah sorry, the rule for balance is not there at all.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It is kind of hard to ensure that with dynamic jumps though :)
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
It always seems to swap operands.
There was a problem hiding this comment.
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
force-pushed
the
optim-address-op
branch
3 times, most recently
from
4c594db to
7b2a20c
Compare
|
Test seems to fail. |
|
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. |
|
I wouldn't rush this into 0.4.23 if we are releasing it today. |
Fixed. It was missing the payable constructor. |
Fixes #3920.