-
Notifications
You must be signed in to change notification settings - Fork 6.1k
eof: Enable peephole optimizer for EOF. #15991
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
base: develop
Are you sure you want to change the base?
Conversation
This comment was marked as resolved.
This comment was marked as resolved.
04e39f0
to
b1af4bd
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.
Looks generally ok, but needs some small tweaks and a few optimizer tests.
if (it[0].type() == Operation) | ||
{ | ||
// These instructions have their own AssemblyItemType | ||
solAssert( | ||
it[0] != Instruction::RJUMP && | ||
it[0] != Instruction::RETURNCONTRACT && | ||
it[0] != Instruction::JUMPF && | ||
it[0] != Instruction::RETF | ||
); | ||
|
||
if ( | ||
it[0] != Instruction::JUMP && | ||
it[0] != Instruction::RETURN && | ||
it[0] != Instruction::STOP && | ||
it[0] != Instruction::INVALID && | ||
it[0] != Instruction::SELFDESTRUCT && | ||
it[0] != Instruction::REVERT | ||
) | ||
return false; | ||
} | ||
else if ( | ||
it[0].type() != RelativeJump && | ||
it[0].type() != JumpF && | ||
it[0].type() != RetF && | ||
it[0].type() != ReturnContract | ||
) |
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 reason I wanted these new items to store the instruction was exactly so that we can treat them uniformly in situations like this. You can pretty much disregard the type and only look at the instruction.
if (it[0].type() == Operation) | |
{ | |
// These instructions have their own AssemblyItemType | |
solAssert( | |
it[0] != Instruction::RJUMP && | |
it[0] != Instruction::RETURNCONTRACT && | |
it[0] != Instruction::JUMPF && | |
it[0] != Instruction::RETF | |
); | |
if ( | |
it[0] != Instruction::JUMP && | |
it[0] != Instruction::RETURN && | |
it[0] != Instruction::STOP && | |
it[0] != Instruction::INVALID && | |
it[0] != Instruction::SELFDESTRUCT && | |
it[0] != Instruction::REVERT | |
) | |
return false; | |
} | |
else if ( | |
it[0].type() != RelativeJump && | |
it[0].type() != JumpF && | |
it[0].type() != RetF && | |
it[0].type() != ReturnContract | |
) | |
if ( | |
it[0] != Instruction::JUMP && | |
it[0] != Instruction::RJUMP && | |
it[0] != Instruction::RETURNCONTRACT && | |
it[0] != Instruction::JUMPF && | |
it[0] != Instruction::RETF && | |
it[0] != Instruction::RETURN && | |
it[0] != Instruction::STOP && | |
it[0] != Instruction::INVALID && | |
it[0] != Instruction::SELFDESTRUCT && | |
it[0] != Instruction::REVERT | |
) | |
return false; |
If you want to be 100% sure we won't miss some weird item where someone forgot to set the instruction, you could also add an assert in this if
that if we're skipping it, it's not one of those 4 types. But I'd say it's optional.
return false; | ||
} | ||
else if ( | ||
it[0].type() != RelativeJump && |
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.
I originally wanted to say that this rule won't work on EOF, because we can no longer rely on JUMPDEST
s being the only targets, but it actually uses tags rather than JUMPDEST
s and we still have those on EOF. Nice! This saves us the need to scan the code and provide extra metadata about jump targets.
One thing we should still do though is to update the docstring to mention that. Right now it's a bit misleading.
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's a bit disappointing that this does not affect more tests. I guess it's due to the fact that our coverage of the evmasm optimizer output is generally minimal and we mostly test it indirectly when compiling high-level code. We also don't measure costs on EOF yet so those are not affected either.
We do have some peephole optimizer tests in test/libevmasm/Optimiser.cpp
and I just noticed that the if
in Assembly.cpp
was not actually bypassing them because they execute PeepholeOptimizer
directly. This means that they've been passing on EOF all along. We only have 11 of them though and they do not cover the EOF-specific optimization methods that were added for jumps and functions.
I think you should at least add cases there to cover UnreachableCode
method with each of the new EOF instructions. Would be nice to also a simple case for each of the new EOF-specific methods if you can.
Doing these tests will become simpler soon due to #16012, but for now the only way is to add them to Optimiser.cpp
. I'll convert then to the new format later - I need to implement assembly import for EOF first.
This PR enables and fixes peephole optimiser for EOF.