Use new BE API from intx #131
Conversation
Codecov Report
@@ Coverage Diff @@
## master #131 +/- ##
=========================================
Coverage ? 87.87%
=========================================
Files ? 19
Lines ? 2054
Branches ? 218
=========================================
Hits ? 1805
Misses ? 224
Partials ? 25 |
chfast
force-pushed
the
intx
branch
2 times, most recently
from
05d3dbe to
3bb2b01
Compare
|
Added benchmark results |
|
Anyone wants to review this? |
lib/evmone/instructions.cpp
Outdated
| @@ -103,7 +106,7 @@ void op_div(execution_state& state, instr_argument) noexcept | |||
| void op_sdiv(execution_state& state, instr_argument) noexcept | |||
| { | |||
| auto& v = state.stack[1]; | |||
| v = v != 0 ? intx::sdivrem(state.stack[0], v).quot : 0; | |||
| v = v != 0 ? sdivrem(state.stack[0], v).quot : 0; | |||
There was a problem hiding this comment.
I actually like the explicit namespace because it improves readability in many places.
There was a problem hiding this comment.
Me too, especially for things like be::store(..., ...) it would be clearer, that some conversion is going on
There was a problem hiding this comment.
I brought back the be::... but intx::be::load<intx::uint256> seems a bit too much...
There was a problem hiding this comment.
The intx::sdivrem() is never needed, because of ADL. I can bring it back in similar cases, but I cannot promise it will stay consistent over time.
lib/evmone/instructions.cpp
Outdated
| @@ -139,7 +139,7 @@ void op_mulmod(execution_state& state, instr_argument) noexcept | |||
| const auto y = state.stack.pop(); | |||
| auto& m = state.stack.top(); | |||
|
|
|||
| m = m != 0 ? ((uint512{x} * uint512{y}) % uint512{m}).lo : 0; | |||
| m = m != 0 ? mulmod(x, y, m) : 0; | |||
There was a problem hiding this comment.
Is there any speed change with these?
lib/evmone/instructions.cpp
Outdated
| @@ -629,17 +599,15 @@ void op_blockhash(execution_state& state, instr_argument) noexcept | |||
| auto upper_bound = state.host.get_tx_context().block_number; | |||
| auto lower_bound = std::max(upper_bound - 256, decltype(upper_bound){0}); | |||
| auto n = static_cast<int64_t>(number); | |||
| auto header = evmc_bytes32{}; | |||
| auto header = evmc::bytes32{}; | |||
There was a problem hiding this comment.
Seems to be a somewhat irrelevant change?
lib/evmone/instructions.cpp
Outdated
| @@ -1066,16 +1018,12 @@ void op_create(execution_state& state, instr_argument arg) noexcept | |||
|
|
|||
| msg.sender = state.msg->destination; | |||
| msg.depth = state.msg->depth + 1; | |||
| intx::be::store(msg.value.bytes, endowment); | |||
| msg.value = store<evmc::uint256be>(endowment); | |||
There was a problem hiding this comment.
In other places you do it like store(msg.value.bytes, value);
I like this assignment form better, maybe change other places for consistency?
|
Ok, I pushed a version which is much more verbose, but still ok in my opinion. We still keep using short |
This is to see new intx API in usage. The intx PR: chfast/intx#107.
It also uses
intx::addmod()andintx::mulmod(). Closes #110.I didn't expect it to perform any better, but it does.
Skylake CPU:
Haswell CPU: