-
Notifications
You must be signed in to change notification settings - Fork 149
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
WordStack optimizations #2632
WordStack optimizations #2632
Conversation
6466343
to
7205c9b
Compare
<localMem> LM </localMem> | ||
<log> L => L ListItem({ ACCT | WordStack2List(#take(N, WS)) | #range(LM, MEMSTART, MEMWIDTH) }) </log> | ||
requires #sizeWordStack(WS) >=Int N | ||
[preserves-definedness] | ||
``` |
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.
Why does this rule fail the preserves-definedness
check in the first place?
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.
Ah, it shouldn't anymore, there was an interim moment when it did. I'll remove that.
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.
Removed.
syntax Int ::= #sizeWordStack ( WordStack ) [symbol(#sizeWordStack), function, total, smtlib(sizeWordStack)] | ||
// ------------------------------------------------------------------------------------------------------------ | ||
rule [ws-size-00]: #sizeWordStack (.WordStack) => 0 | ||
rule [ws-size-01]: #sizeWordStack (_ : .WordStack) => 1 | ||
rule [ws-size-02]: #sizeWordStack (_ : _ : .WordStack) => 2 | ||
rule [ws-size-03]: #sizeWordStack (_ : _ : _ : .WordStack) => 3 | ||
rule [ws-size-04]: #sizeWordStack (_ : _ : _ : _ : .WordStack) => 4 | ||
rule [ws-size-05]: #sizeWordStack (_ : _ : _ : _ : _ : .WordStack) => 5 | ||
rule [ws-size-06]: #sizeWordStack (_ : _ : _ : _ : _ : _ : .WordStack) => 6 | ||
rule [ws-size-07]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : .WordStack) => 7 | ||
rule [ws-size-08]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 8 | ||
rule [ws-size-09]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 9 | ||
rule [ws-size-10]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 10 | ||
rule [ws-size-11]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 11 | ||
rule [ws-size-12]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 12 | ||
rule [ws-size-13]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 13 | ||
rule [ws-size-14]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 14 | ||
rule [ws-size-15]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 15 | ||
rule [ws-size-16]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : .WordStack) => 16 | ||
rule [ws-size-N]: #sizeWordStack (_ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : _ : WS) => 16 +Int #sizeWordStack(WS) | ||
rule [ws-size-O]: #sizeWordStack (_ : WS) => 1 +Int #sizeWordStack(WS) [priority(75)] |
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 the reason for how it was structured before was to keep the computation tail-recursive, which the LLVM backend requires for efficient memory/time usage. Can we check the LLVM backend performance on teh conformance test-suite here?
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 don't like this, I've reverted back to just the simple tail recursion.
@ehildenb Do you remember why there was an auxiliary function here with two parameters instead of a direct computation?
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.
evm-semantics/kevm-pyk/src/kevm_pyk/kproj/evm-semantics/evm-types.md
Lines 398 to 401 in 3d7c9c2
syntax Int ::= #sizeWordStack ( WordStack ) [symbol(#sizeWordStack), function, total, smtlib(sizeWordStack)] | |
// ------------------------------------------------------------------------------------------------------------ | |
rule [ws-size-base]: #sizeWordStack (.WordStack) => 0 | |
rule [ws-size-ind]: #sizeWordStack (_ : WS) => 1 +Int #sizeWordStack(WS) |
This isn't tail recursive. It's doing an add operation after the call. That's why there was an auxiliary function with the extra parameter.
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.
Ah yes yes, sorry, hm.
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 will bring it back. It's not the thing that's breaking the proofs anyways and works well.
If we wanted to speed this calculation up, we can merge something like this.
optimizer/optimizations.md
Outdated
claim | ||
[optimized.add]: | ||
<kevm> | ||
<k> | ||
( #next[ ADD ] => .K ) ... | ||
</k> | ||
<schedule> | ||
SCHED | ||
</schedule> | ||
<useGas> | ||
USEGAS | ||
</useGas> | ||
<ethereum> | ||
<evm> | ||
<callState> | ||
<wordStack> | ||
( W0 : W1 : WS => chop( ( W0 +Int W1 ) ) : WS ) |
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.
Why is this added here?
We already have a test-harness that takes the rules in this file and discharges them as claims directly.
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.
Oh, actually, this entire directory isn't used at all anymore. We really should delete it, but I left it for illustrative purposes. Any reason to add this?
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.
Ah, that's a leftover of me trying to test a failing optimisation, will remove.
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.
Removed.
tests/specs/mcd/dai-adduu-fail-rough-spec.k | ||
tests/specs/mcd/flipper-addu48u48-fail-rough-spec.k | ||
tests/specs/mcd-structured/dai-adduu-fail-rough-spec.k | ||
tests/specs/mcd-structured/flipper-addu48u48-fail-rough-spec.k |
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.
Well that's less than ideal I guess.....
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 is, but the booster will adapt and be more expressive, as per this issue.
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 might allow us to prove further tests using just booster-dev
.
We really need some performance metrics posted here, both for Haskell and LLVM backend. |
What would be the best way to get the metrics? For the LLVM backend, I guess the conformance test suite. |
@PetarMax regarding the performance metrics for the Haskell backend: please ask @anvacaru, I remember that he has adapted our Nix-based performance measurement script to vary the version of KEVM instead of HB. This thould get tou a table comparing KEVM |
Just to say regarding concrete execution and the LLVM backend - the moment that we have anything symbolic in the word stack, we cannot use the LLVM backend to compute anything over it, even if it doesn't depend on the symbolicness (say, length), and so any tail recursion optiimisations will only work for purely concrete word stacks. |
Yes, I put something together from the Haskell Backend's test-performance-kevm.sh script. I would run this profile-kevm script for both branches I'm interested in. export KEVM_VERSION=master; profile-kevm
export KEVM_VERSION=wordstack-optimizations; profile-kevm then I would use the compare_logs.py logfile_1 logfile_2 to generate the table with the time values. I will give it a go in the background and post the results here. |
It seems like these optimizations are causing a slowdown. @PetarMax can you confirm this?
|
This PR introduces some optimizations that should allows us to manipulate word stacks more efficiently..