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? |
| { | ||
| 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.
| 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?
| 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?
| 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 |
Use new BE API from intx
The KZG point-evaluation precompile verifies
e(C + [z]·π - [y]·G1, [1]₂) =? e(π, [s]₂)
The right side already uses blst_miller_loop_lines with precomputed
lines for [s]₂. The left side used blst_aggregated_in_g1, which is
a convenience wrapper that calls miller_loop_n(&BLS12_381_G2, sig, 1)
on the fly — no precomputed lines.
Precompute the 68 vec384fp6 Miller-loop lines for the G2 generator
[1]₂ (the same way [s]₂ was done) and route the left pairing through
blst_miller_loop_lines too. Adds ~19.6 KiB of static data.
Both line tables now live in kzg_precomputed_lines.{hpp,cpp} (renamed
from kzg_setup_g2_1_lines.{hpp,cpp}). Each table has a runtime equality
test (memcmp against blst_precompute_lines output) in
test/unittests/precompiles_kzg_test.cpp.
Bench follow-up (TODO from the previous KZG joint-MSM commit refers
to this: ~18% extra win reported by erigontech/go-eth-kzg PR #131).
4 participants
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: