Skip to content
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

Branch optimization #7

Closed
wants to merge 7 commits into from
Closed

Conversation

Robertorosmaninho
Copy link

@Robertorosmaninho Robertorosmaninho commented Jul 25, 2024

FIX: #1588

This PR implements a branch continuation optimization. Previously, we were popping values in the <instrs> cell once we found a br instruction without a label after it, and once we found a label if the IDX of the br instruction wasn't 0, we rewrote it until reaching the base case. This was fairly time-consuming and inefficient.

This new optimization addresses the two issues by sacrificing space efficiency. Here, we introduce a new K list cell, labels, which contains the whole state continuation of a block. So, right now, instead of popping instructions, we just replace the whole <instr> cell, and instead of rewriting the br instruction until it reaches the base case, we just use the list index to "jump" to the correct block and pop the others using the range hook.

So, in programs that don't have many changes in control flow, we may have some slowdown. However, for programs that contain such cases, we expect to see some great speedups.

@Robertorosmaninho Robertorosmaninho requested a review from theo25 July 25, 2024 22:49
@Robertorosmaninho Robertorosmaninho marked this pull request as ready for review July 31, 2024 17:55
@dwightguth
Copy link

This needs evidence that it's faster on a simple program with some huge basic blocks.

@dwightguth
Copy link

Please rebase this on the latest master so I can more effectively review it.

@Robertorosmaninho
Copy link
Author

Please rebase this on the latest master so I can more effectively review it.

Done!

@Robertorosmaninho
Copy link
Author

Robertorosmaninho commented Aug 8, 2024

For some reason that I couldn't spot easily, after my changes, call.wast started to fail in only one test: (assert_return (invoke "as-unary-operand") (f32.const 0x0p+0)) with:

poetry -C pykwasm run -- kwasm run tests/wasm-tests/test/core/call.wast > tests/wasm-tests/test/core/call.wast.llvm-out
/home/robertorosmaninho/rv/k/k-distribution/bin/../lib/kframework/k-util.sh: line 114: 1037198 Bus error               (core dumped) "$@"
[Error] krun: 
/home/robertorosmaninho/.cache/kdist-efbf2fa/wasm-semantics/llvm/interpreter 
/tmp/.krun-2024-08-08-21-47-27-pYn6dr884r/tmp.in.PMv6wHOLHm -1 
/tmp/.krun-2024-08-08-21-47-27-pYn6dr884r/result.kore
Syntax error at /tmp/.krun-2024-08-08-21-47-27-pYn6dr884r/result.kore:1.135162-135168: Expected: [:, {] Actual: <EOF>
[Error] krun: kore-print --definition 
/home/robertorosmaninho/.cache/kdist-efbf2fa/wasm-semantics/llvm --output 
pretty /tmp/.krun-2024-08-08-21-47-27-pYn6dr884r/result.kore --color off
make: *** [Makefile:51: tests/wasm-tests/test/core/call.wast.run-term] Error 255

@dwightguth
Copy link

This looks about what I expected. It's unfortunate that this doesn't end up helping performance, but I guess it's good that the existing code wasn't that bad in the first place.

@dwightguth dwightguth closed this Aug 9, 2024
@dwightguth dwightguth deleted the branch-optimization branch August 9, 2024 20:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants