Skip to content

Apply delta_ip and delta_stp from side table#690

Merged
ia0 merged 17 commits into
google:dev/fast-interpfrom
zhouwfang:use-delta-ip-stp
Dec 11, 2024
Merged

Apply delta_ip and delta_stp from side table#690
ia0 merged 17 commits into
google:dev/fast-interpfrom
zhouwfang:use-delta-ip-stp

Conversation

@zhouwfang

@zhouwfang zhouwfang commented Nov 18, 2024

Copy link
Copy Markdown
Contributor

Apply delta_ip and delta_stp from side table in exec.rs.

#46

@zhouwfang
zhouwfang requested a review from ia0 as a code owner November 18, 2024 05:12
@zhouwfang
zhouwfang marked this pull request as draft November 18, 2024 05:12
Comment thread crates/interpreter/src/parser.rs Outdated
@zhouwfang
zhouwfang marked this pull request as ready for review December 1, 2024 01:25
@zhouwfang zhouwfang changed the title Add skip_to() Apply delta_ip and delta_stp Dec 1, 2024
@zhouwfang zhouwfang changed the title Apply delta_ip and delta_stp Apply delta_ip and delta_stp in side table Dec 1, 2024
@zhouwfang zhouwfang changed the title Apply delta_ip and delta_stp in side table Apply delta_ip and delta_stp from side table Dec 1, 2024
@zhouwfang

zhouwfang commented Dec 1, 2024

Copy link
Copy Markdown
Contributor Author

The entire test suite is passed, but when CI/hw-host runs target/wasefire/applet.wasm, it points out a bug. Unfortunately this WASM module is too big to debug. Maybe you could find the bug in review. I'll try to debug in the meanwhile.

@ia0 ia0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I didn't review everything in details but I think the global idea is there.

Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
@zhouwfang

zhouwfang commented Dec 2, 2024

Copy link
Copy Markdown
Contributor Author

Forgot to ask: How can I enable debug feature when running hwci.sh? I'd like to print logs for the applet.wasm error.

Another approach I tried: I converted it to applet.wast and moved it to the test suite directory, and tried to run spec.rs with it. But its module format/layout seems incompatible, which would require changes in spec.rs.

@ia0

ia0 commented Dec 2, 2024

Copy link
Copy Markdown
Member

Forgot to ask: How can I enable debug feature when running hwci.sh?

You can apply the following patch:

diff --git a/scripts/hwci.sh b/scripts/hwci.sh
index 4c222fd0..1f4ac986 100755
--- a/scripts/hwci.sh
+++ b/scripts/hwci.sh
@@ -77,7 +77,7 @@ full() {
 }
 
 case $1 in
-  host) full unix host --arg=--protocol=unix ;;
+  host) full unix host --arg=--protocol=unix --features=wasefire-interpreter/debug ;;
   # P1.01, P1.02, and P1.03 must be connected together (gpio_test).
   nordic) full usb nordic ;;
   *) run --protocol=${1:-usb} "$2" ;;

And then simply run ./scripts/hwci.sh host as usual.

@zhouwfang

zhouwfang commented Dec 6, 2024

Copy link
Copy Markdown
Contributor Author

@ia0 I was wondering how to get around the following "cannot borrow *self as mutable more than once at a time" error. Thanks.

In Thread::step(), I basically use the following function to take the jump

fn take_jump(&mut self, ls_idx: Option<usize>, side_tables: &[Vec<SideTableEntry>]) {
    let frame = self.frame();
    let parser = &mut self.parser;
    frame.take_jump(parser, ls_idx, side_tables);
}

where self.frame() returns &mut Frame<'m>.

ia0 pushed a commit that referenced this pull request Dec 6, 2024
When "if" has an "else", we should skip the entry for "else" in the side
table. It was addressed in #690 during execution and should be moved to
validation.

#46

Co-authored-by: Zhou Fang <[email protected]>
@ia0

ia0 commented Dec 6, 2024

Copy link
Copy Markdown
Member

Yes, that's a common problem in Rust. You'll need to inline the definition of fn frame() instead of calling it.

@zhouwfang
zhouwfang requested a review from ia0 December 6, 2024 16:02

@ia0 ia0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! It looks pretty good.

Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/module.rs
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
@zhouwfang

zhouwfang commented Dec 10, 2024

Copy link
Copy Markdown
Contributor Author

I'd like to refactor incrementally. After storing side_table: &'m [SideTableEntry] in Frame and making Module::func() return side table additionally, there appear to be three confusing Rust errors when creating Frame. I'm not sure why they occur. Could you take a look at the last commit? Thanks.

Comment thread crates/interpreter/src/exec.rs
Comment thread crates/interpreter/src/exec.rs
Comment thread crates/interpreter/src/exec.rs
Comment thread crates/interpreter/src/exec.rs Outdated
@zhouwfang
zhouwfang requested a review from ia0 December 10, 2024 06:53
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/module.rs

@ia0 ia0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to merge modulo the nits.

Comment thread crates/interpreter/src/module.rs
Comment thread crates/interpreter/src/module.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
Comment thread crates/interpreter/src/exec.rs Outdated
@zhouwfang
zhouwfang requested a review from ia0 December 11, 2024 15:25

@ia0 ia0 left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, let's merge it and continue in follow-up PRs.

@ia0
ia0 merged commit 715bbaa into google:dev/fast-interp Dec 11, 2024
ia0 pushed a commit that referenced this pull request Dec 13, 2024
On my Linux Docker container:

1. 

CoreMark results of this PR: 37.991516 (in 18.695s), 36.7287 (in
19.258s)

CoreMark results of #690:
37.101162 (in 19.133s), 37.39716 (in 18.996s)

Look very similar. Not sure about `nordic`.

2. 

CoreMark results of `main`: 28.791477 (in 18.09s), 28.814291 (in
17.734s)

So there does seem some minor performance improvement with `delta_ip`
and `delta_stp`.

#46

---------

Co-authored-by: Zhou Fang <[email protected]>
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