-
Notifications
You must be signed in to change notification settings - Fork 6.3k
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.
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
EIP-7939 CLZ for Osaka #16122
Conversation
|
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. |
| {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}}, |
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.
CLZ has the same gas cost as ADD, which is in the Tier::VeryLow.
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 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).
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 linked EIP still mentions a gas cost similar to ADD, and as @Vectorized pointed out, it is indeed 3, i.e. Tier::VeryLow. Which is also the same used by evmone. Do you have the source for the change to 5, @Saw-mon-and-Natalie?
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.
Which is also the same used by evmone.
Oh, I see that in the PR it was actually indeed changed to 5: https://github.com/ipsilon/evmone/pull/1264/files#diff-d47e4d7f522e531f3080bb99145762aef9857717b9e9218a0e90e09e58fa865eR176
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 fine
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 the execution spec that should be the source of truth for us here. count_leading_zeros() uses GAS_LOW, so that's the right value.
But note that it matters whether it's LOW or independently defined as 5. If it was the latter and it had its own constant, we would have to introduce a distinct tier for it. Technically, a future repricing could change the values assigned to those tiers. The cost of CLZ should change in such a case only if it is explicitly defined as a part of such a tier.
matheusaaguiar
left a comment
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.
Hi @Saw-mon-and-Natalie , thanks for the PR!
Looking good.
|
|
|
Hi @Saw-mon-and-Natalie could you please rebase the PR? Or would you prefer if we take it over? |
clonker
left a comment
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.
generally lgtm
| {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}}, |
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 the execution spec that should be the source of truth for us here. count_leading_zeros() uses GAS_LOW, so that's the right value.
But note that it matters whether it's LOW or independently defined as 5. If it was the latter and it had its own constant, we would have to introduce a distinct tier for it. Technically, a future repricing could change the values assigned to those tiers. The cost of CLZ should change in such a case only if it is explicitly defined as a part of such a tier.
also resolve a whitespace issue in one test
Fixes #16118.
This PR addresses:
By adding a new YUL builtin function
clzfor 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.solfail due toevmonenot having implementedCLZ:https://github.com/ethereum/solidity/blob/779daec70826225a447680d2030f5fdaafca541b/test/EVMHost.cpp#L420
In the above line
m_vm.execute(...)returnsEVMC_UNDEFINED_INSTRUCTIONand them_vmis:References: