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

Merged
merged 2 commits into from
Apr 24, 2025

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 589
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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure. This code was added to PoC branch before the first EOF branch was merged. Forgot about it.

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.

@rodiazet rodiazet force-pushed the enable-peephole-optimiser branch from b1af4bd to d85e242 Compare April 23, 2025 09:43
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 good, but the branch contains a merge commit. Please rebase on top of develop to remove it.

@rodiazet rodiazet force-pushed the enable-peephole-optimiser branch from 42bea7a to 03fde2a Compare April 24, 2025 09:18
@rodiazet
Copy link
Contributor Author

Done. We can merge now.

@cameel cameel merged commit 4c4860b into ethereum:develop Apr 24, 2025
74 checks passed
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