Skip to content

Conversation

zhouwfang
Copy link
Contributor

@zhouwfang zhouwfang commented Nov 18, 2024

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
@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
Copy link
Contributor Author

zhouwfang commented Dec 1, 2024

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.

Copy link
Member

@ia0 ia0 left a comment

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.

@zhouwfang
Copy link
Contributor Author

zhouwfang commented Dec 2, 2024

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
Copy link
Member

ia0 commented Dec 2, 2024

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
Copy link
Contributor Author

zhouwfang commented Dec 6, 2024

@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 <33002388+zhou-w-fang@users.noreply.github.com>
@ia0
Copy link
Member

ia0 commented Dec 6, 2024

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
Copy link
Member

@ia0 ia0 left a comment

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.

@zhouwfang
Copy link
Contributor Author

zhouwfang commented Dec 10, 2024

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.

@zhouwfang zhouwfang requested a review from ia0 December 10, 2024 06:53
Copy link
Member

@ia0 ia0 left a comment

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.

@zhouwfang zhouwfang requested a review from ia0 December 11, 2024 15:25
Copy link
Member

@ia0 ia0 left a comment

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
20 checks passed
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 <33002388+zhou-w-fang@users.noreply.github.com>
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