Add val_cnt and pop_cnt in side table by zhouwfang · Pull Request #649 · google/wasefire

zhouwfang · 2024-10-23T05:30:46Z

There was an earlier review in zhouwfang#2.

zhouwfang · 2024-10-23T05:38:03Z

The hw-host error says Internal:NotImplemented. What does it refer to?

ia0

Thanks! Going through I realized we probably want to store val_cnt in source branches already since that's an information available at that time. This is particularly relevant in br_label().

So I suggest:

Add result: usize to SideTableBranch which is always zero for target branches and represents val_cnt for source branches.
Make branch_source() take result: usize.
Refactor br_label() to match on the label kind before creating the source branch, returning (ResultType, Option<SideTableBranch>) which is the result of the function and the target branch (if it's a loop). Then create the source branch with the result length, match on the option to stitch or push, and return the result.

ia0 · 2024-10-23T09:09:55Z

The hw-host error says Internal:NotImplemented. What does it refer to?

Ideally we should #[cfg(feature = "debug")] eprintln!(""); before returning Unsupported such that we can easily debug with runner host --features=wasefire-interpreter/debug.

ia0

Ok, I think it should be mostly correct. It'll be easier to debug once we can run the test suite using the side table.

ia0

I tried to look into the tests and actually there are fundamental problems:

Since I added auto-skipping of unsupported behaviors, we may accidentally pass the test because everything is unsupported due to a bug. I need to fix the tests so that we can distinguish between really unsupported and unsupported by accident. Not completely sure yet how, probably just a counter of skipped tests due to unsupported, but I'd like to find something better.
We need to be very careful when we call branch_{source,target}() because it will save the stack. This matters particularly when we push a label, because we don't want to save the parameters of the label.

Sadly I don't have time at the moment to dig into this. I think the design is not there yet (for example, maybe we actually want to store the "val_cnt/result" in the target branch instead. Maybe it will help with capturing too much of the stack because we know val_cnt is included.

If you want to continue working on this until I get time to look into again, you can try to experiment on your own and run the test suite to test the invariants you come up with. The more asserts you can write that don't fail in the test suite, the better your understanding of the code will be.

zhouwfang · 2024-10-28T04:15:19Z

With the commit cc29418 and #658, all the tests pass now.

In the meanwhile, I'm printing the side tables for some test cases in the test suite as a sanity check.

ia0

I don't mind merging this without thorough review such that you can start using the side table in exec.rs which is the important part to be able to debug. I think this PR needs a redesign and it's hard to tell unless the side table is actually used during execution. I'll do a proper review once we have everything working end-to-end.

But you can also use the side table in exec.rs in this PR if you want. As you prefer.

zhouwfang · 2024-10-29T05:42:54Z

Thanks for the review. I plan to use val_cnt and pop_cnt in exec.rs in this PR.

Also self-update at startup for `cargo install` builds, since those lack the host platform.

Verified for 3 examples.

zhouwfang · 2024-11-17T02:41:07Z

@ia0 Could you merge main to dev/fast-interp again? Thanks.

zhouwfang · 2024-11-17T03:28:30Z

Note: the side tables are now correct for the examples we discussed.

zhouwfang · 2024-11-17T06:06:54Z

            Loop(b) => {
-                self.push_label(self.blocktype(&b)?, LabelKind::Loop(self.branch_target()))?
+                let type_ = self.blocktype(&b)?;
+                self.push_label(type_, LabelKind::Loop(self.branch_target(type_.results.len())))?


For loop, I'm not sure whether I should use type_.results.len() (as in other cases) or type_.params.len() for val_cnt. But I suspect the latter is correct for the following reason: in exec.rs, the last arity values are kept which should be the same as val_cnt. By the definition of arity in exec.rs, the latter is used.

Exactly, it's params.

Let me know if my understanding about the reason why it is different for loop is correct: For loop, we jump to the beginning instead of the end. When we jump to the beginning, we need to look at the params. The result of the loop will be handled by the outer scope of the loop.

zhouwfang · 2024-11-18T00:22:14Z

After thinking more, it may be better to merge this PR first because using val_cnt and pop_cnt in exec.rs requires using delta_ip and delta_stp as a first step (need to be located at the right side table entry first). I'd like to have a separate PR for using delta_ip and delta_stp in exec.rs. WDYT?

ia0

Sounds good, let's merge this.

ia0 · 2024-11-18T08:53:12Z

            Loop(b) => {
-                self.push_label(self.blocktype(&b)?, LabelKind::Loop(self.branch_target()))?
+                let type_ = self.blocktype(&b)?;
+                self.push_label(type_, LabelKind::Loop(self.branch_target(type_.results.len())))?


Exactly, it's params.

zhouwfang requested a review from ia0 as a code owner October 23, 2024 05:30

zhouwfang commented Oct 23, 2024

View reviewed changes

Comment thread crates/interpreter/src/valid.rs Outdated

zhouwfang commented Oct 23, 2024

View reviewed changes

Comment thread crates/interpreter/src/valid.rs Outdated

ia0 reviewed Oct 23, 2024

View reviewed changes

zhouwfang requested a review from ia0 October 24, 2024 05:45

ia0 reviewed Oct 24, 2024

View reviewed changes

zhouwfang requested a review from ia0 October 25, 2024 02:46

ia0 reviewed Oct 25, 2024

View reviewed changes

Comment thread crates/interpreter/src/valid.rs Outdated

Comment thread crates/interpreter/src/valid.rs

zhouwfang force-pushed the add-val-and-pop-cnt branch from 5c1c205 to cc29418 Compare October 28, 2024 03:58

zhouwfang requested a review from ia0 October 28, 2024 04:15

ia0 reviewed Oct 28, 2024

View reviewed changes

Comment thread crates/interpreter/src/valid.rs Outdated

Comment thread crates/interpreter/src/valid.rs Outdated

Comment thread crates/interpreter/src/valid.rs Outdated

Comment thread crates/interpreter/src/valid.rs Outdated

ia0 and others added 11 commits November 14, 2024 21:21

Add wasefire self-update command (google#689)

a91de33

Also self-update at startup for `cargo install` builds, since those lack the host platform.

fix: increase mask size to 64 bits

93b7de2

review

62bb080

feat!: add val_cnt and pop_cnt

daf29ee

refactor: rename

ab0de80

fix!: review

3009794

fix: use i32 in pop_cnt()

2c84e24

refactor: review

9fbb0cb

fix: target-source in pop_cnt()

99ec978

fix: review

33e6ab4

fix: val_cnt and pop_cnt calculation

6a1f2e5

Verified for 3 examples.

zhouwfang force-pushed the add-val-and-pop-cnt branch from f509e36 to 6a1f2e5 Compare November 17, 2024 02:33

zhouwfang added 2 commits November 16, 2024 18:54

fix: source < target due to target result

2c0082e

fix: subtraction overflow

2efa29b

fix: subtraction overflow

668a981

zhouwfang commented Nov 17, 2024

View reviewed changes

zhouwfang and others added 2 commits November 17, 2024 17:04

fix: val_cnt for loop

1ce859c

Merge branch 'dev/fast-interp' into add-val-and-pop-cnt

a7b1257

ia0 approved these changes Nov 18, 2024

View reviewed changes

ia0 merged commit 24781ed into google:dev/fast-interp Nov 18, 2024

Uh oh!

Conversation

zhouwfang commented Oct 23, 2024

Uh oh!

Uh oh!

Uh oh!

zhouwfang commented Oct 23, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ia0 commented Oct 23, 2024

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zhouwfang commented Oct 28, 2024

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

zhouwfang commented Oct 29, 2024

Uh oh!

zhouwfang commented Nov 17, 2024

Uh oh!

zhouwfang commented Nov 17, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zhouwfang Nov 17, 2024

Choose a reason for hiding this comment

Uh oh!

ia0 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

zhouwfang Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

ia0 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

zhouwfang commented Nov 18, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ia0 left a comment

Choose a reason for hiding this comment

Uh oh!

ia0 Nov 18, 2024

Choose a reason for hiding this comment

Uh oh!

Reviewers

zhouwfang commented Oct 23, 2024 •

edited

Loading

zhouwfang commented Nov 17, 2024 •

edited

Loading

zhouwfang commented Nov 18, 2024 •

edited

Loading