Skip to content

EIP-7939 CLZ for Osaka #16122

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
Open

EIP-7939 CLZ for Osaka #16122

wants to merge 2 commits into from
+153 −3

Conversation

Saw-mon-and-Natalie
Copy link
Contributor

@Saw-mon-and-Natalie Saw-mon-and-Natalie commented Jul 16, 2025 •
edited
Loading

This PR addresses:

By adding a new YUL builtin function clz for EVM versions greater than or equal to Osaka.

Followed the patterns from #14757 to create these modifications:

Tests

Currently the tests in semanticTests/inlineAssembly/clz.sol fail due to evmone not having implemented CLZ:

solidity/test/EVMHost.cpp

Line 420 in 779daec

evmc::Result result = m_vm.execute(*this, m_evmRevision, message, code.data(), code.size());

In the above line m_vm.execute(...) returns EVMC_UNDEFINED_INSTRUCTION and the m_vm is:

{
    abi_version: 12,
    name: "evmone",
    version: "0.15.0",
    destroy: ... ,
    execute: ... ,
    get_capabilities: ... ,
    set_option: ...
}

References:

@github-actions GitHub Actions
Copy link

Thank you for your contribution to the Solidity compiler! A team member will follow up shortly.

If you haven't read our contributing guidelines and our review checklist before, please do it now, this makes the reviewing process and accepting your contribution smoother.

If you have any questions or need our help, feel free to post them in the PR or talk to us directly on the #solidity-dev channel on Matrix.

Vectorized
Vectorized reviewed Jul 19, 2025
View reviewed changes
libevmasm/Instruction.cpp
@@ -219,6 +220,7 @@ static std::map<Instruction, InstructionInfo> const c_instructionInfo =
{Instruction::SHL, {"SHL", 0, 2, 1, false, Tier::VeryLow}},
{Instruction::SHR, {"SHR", 0, 2, 1, false, Tier::VeryLow}},
{Instruction::SAR, {"SAR", 0, 2, 1, false, Tier::VeryLow}},
{Instruction::CLZ, {"CLZ", 0, 1, 1, false, Tier::Low}},
Copy link
Contributor

Choose a reason for hiding this comment

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

CLZ has the same gas cost as ADD, which is in the Tier::VeryLow.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it was updated. Also it is 5 in geth. Plus in the EIP it mentions:

The cost of the opcode is 5, matching MUL (raised from 3 to avoid under-pricing DoS risk).

matheusaaguiar
matheusaaguiar reviewed Jul 23, 2025
View reviewed changes
Copy link
Collaborator

@matheusaaguiar matheusaaguiar left a comment

Choose a reason for hiding this comment

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

Hi @Saw-mon-and-Natalie , thanks for the PR!
Looking good.

Changelog.md Outdated
@@ -1,6 +1,7 @@
### 0.8.31 (unreleased)

Language Features:
* Yul: Introduce builtin ``clz(x)`` for counting leading zeoros.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* Yul: Introduce builtin ``clz(x)`` for counting leading zeoros.
* Yul: Introduce builtin ``clz(x)`` for counting the number of leading zero bits in a 256-bit word.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied.

docs/yul.rst Outdated
Comment on lines 755 to 756
Opcodes marked with ``F``, ``H``, ``B``, ``C``, ``I``, ``L``, ``P``, ``N``, ``R`` and ``O`` are present since
Frontier, Homestead, Byzantium, Constantinople, Istanbul, London, Paris, Cancun, Prague or Osaka respectively.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why R and Prague? Isn't it available only for Osaka and later versions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, just added Prague for completeness. Even though the R flag is never used. I can remove it. PS, how are these flags derived from the original fork name? The first letter pattern breaks for Cancun.

Copy link
Contributor Author

@Saw-mon-and-Natalie Saw-mon-and-Natalie Jul 23, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

In the table O is used for clz corresponding to Osaka. I'll remove Prague since in the past only the relevant hard fork names are listed here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed.

docs/yul.rst Outdated
@@ -812,6 +812,8 @@ the ``dup`` and ``swap`` instructions as well as ``jump`` instructions, labels a
+-------------------------+-----+---+-----------------------------------------------------------------+
| sar(x, y) | | C | signed arithmetic shift right y by x bits |
+-------------------------+-----+---+-----------------------------------------------------------------+
| clz(x) | | O | leading zeros of x in binary representation, 256 if x == 0 |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| clz(x) | | O | leading zeros of x in binary representation, 256 if x == 0 |
| clz(x) | | O | number of leading zero bits of x, 256 if x == 0 |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

applied.

@Saw-mon-and-Natalie
Copy link
Contributor Author

clz also got added to evmone which should solve the test issues (see PR description) here in this PR. But it has not assigned a new version I think yet. So in this repo, one might need to use the compiled binary from the HEAD of that repo.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Vectorized Vectorized Vectorized left review comments

@matheusaaguiar matheusaaguiar matheusaaguiar left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
external contribution ⭐
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Saw-mon-and-Natalie @Vectorized @matheusaaguiar