Compute delta_ip in side table for If/Else

[zhouwfang]

Would like to get some feedback about this simplest case before computing delta_ip for the other branch types.

#46

[zhouwfang]

There are a few issues:

• side_table_last_entry is wrong, you need to supported nested blocks. You might need to put an index to the side table in Label or LabelKind.
• I don't think instruction index is the correct abstraction, I think we want instruction address. So you don't need instr_idx at all and can use parser.save().as_ptr() as i32.

Then in terms of design, I think we should keep Context constant in Expr and make Expr::check_body return the side-table. We can then set it from Context::check_module where check_body is called.

Thanks! The issues with parser.save().as_ptr() as i32 are

1. parser.save().as_ptr() overflows.

2. The DELTA_IP_MASK is too small to hold it.

[ia0]

Thanks! The issues with parser.save().as_ptr() as i32 are

1. parser.save().as_ptr() overflows.
2. The DELTA_IP_MASK is too small to hold it.

Those are not issues because we only care about deltas between parser.save(). In the proposed design, I store directly the &'m [u8] delays converting to i32 until after we compute the delta.

[zhouwfang]

I'm not sure it's worth iterating too much. It's simpler to just design correctly from the start, which means understanding the goal. Essentially, we want all occurrences of skip_to_{else,end}() in exec.rs to be resolved from the side-table (to which we can add Loop to handle everything uniformly). This means If, Else, Br{,If} should each have an entry in the side-table and BrTable as much as its arguments. This defines when an entry is added. Then we need to fill their content, which is done on Loop, Else, and End (with Loop done before the entry is added). This means that for Loop we can store the parser position in the LabelKind (like done in exec.rs). For Else and End, we need the side-table entries to register themselves in their labels. This means that Label should contain an entries: Vec<(&'m [u8], usize)> recording for each side-table entry the parser position and the side-table length when it was added. Then in end_label() we can simply go over all entries compute the difference between the current parser position and the saved one to compute the delta IP, as well as the difference between the current side-table length and the index for the delta STP. We can then update the side-table entry.

Thanks! I implemented this design except Loop for the moment. I'll run some tests in the meanwhile.

[ia0]

I've merge main into dev/fast-interp. You might want to pull and merge your add-delta-ip branch into dev/fast-interp. This adds better support for unsupported operations, such that we can still run parts of a test instead of having to ignore it completely.

[zhouwfang]

Could you clarify the hw-host error? I guess it results from the side table bit mask size issue.

[zhouwfang]

I posted https://github.com/zhouwfang/wasefire/pull/2 to add val_cnt and pop_cnt on top of this PR. Could you also take a look at that? Thanks.

[ia0]

Could you clarify the hw-host error? I guess it results from the side table bit mask size issue.

Yes, this is indeed the side table masks being too short. So we'll probably have to use a u64 for the bit fields. You can also apply the following patch:

--- a/crates/interpreter/src/bit_field.rs
+++ b/crates/interpreter/src/bit_field.rs
@@ -29,6 +29,8 @@ fn offset(mask: u32) -> i32 {
 pub fn into_field(mask: u32, value: u32) -> Result<u32, Error> {
     let field = (value << mask.trailing_zeros()) & mask;
     if from_field(mask, field) != value {
+        #[cfg(feature = "debug")]
+        eprintln!("Bit field value {value:08x} doesn't fit in mask {mask:08x}.");
         return Err(unsupported(if_debug!(Unsupported::SideTable)));
     }
     Ok(field)

This is similar to what we do in the memory_too_small function.