Skip to content

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

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

rodiazet
Copy link
Contributor

@rodiazet rodiazet commented Apr 9, 2025

This PR enables and fixes peephole optimiser for EOF.

This comment was marked as resolved.

Copy link
Member

@cameel cameel left a 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.

Comment on lines +575 to 600
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
)
Copy link
Member

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.

Suggested change
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 &&
Copy link
Member

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 JUMPDESTs being the only targets, but it actually uses tags rather than JUMPDESTs 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.

Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants