-
Notifications
You must be signed in to change notification settings - Fork 6.1k
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
Conversation
1b14dca
to
eb8f830
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add tests.
libevmasm/RuleList.h
Outdated
Instruction::COINBASE | ||
}) | ||
rules.push_back({ | ||
{Instruction::AND, {{u256("0xffffffffffffffffffffffffffffffffffffffff"), op}}}, |
There was a problem hiding this comment.
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
)?
There was a problem hiding this comment.
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.
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.
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
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
probably better, yes
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is weird.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
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.