Conversation
|
Changes in hir + masm: Below is the diff in What's changing:
Diff in hir and masmdiff --git a/tests/integration/expected/add_i8.hir b/tests/integration/expected/add_i8.hir
index 4d336c54..1d66f702 100644
--- a/tests/integration/expected/add_i8.hir
+++ b/tests/integration/expected/add_i8.hir
@@ -3,7 +3,7 @@ builtin.component root_ns:root@1.0.0 {
public builtin.function @entrypoint(v0: i32, v1: i32) -> i32 {
^block6(v0: i32, v1: i32):
v5 = arith.add v1, v0 : i32 #[overflow = wrapping];
- v6 = arith.sext v5 : i32;
+ v6 = wasm.i32_extend_8s v5 : i32;
builtin.ret v6;
};
diff --git a/tests/integration/expected/add_i8.masm b/tests/integration/expected/add_i8.masm
index 9bc4bdc6..cfce1329 100644
--- a/tests/integration/expected/add_i8.masm
+++ b/tests/integration/expected/add_i8.masm
@@ -22,5 +22,16 @@ end
@callconv("C")
pub proc entrypoint(i32, i32) -> i32
u32wrapping_add
+ push.255
+ u32and
+ dup.0
+ push.128
+ u32and
+ eq.128
+ push.0
+ push.4294967040
+ movup.2
+ cdrop
+ u32or
end
diff --git a/tests/integration/expected/neg_i8.hir b/tests/integration/expected/neg_i8.hir
index 6c70b2fc..cc0886c4 100644
--- a/tests/integration/expected/neg_i8.hir
+++ b/tests/integration/expected/neg_i8.hir
@@ -4,7 +4,7 @@ builtin.component root_ns:root@1.0.0 {
^block6(v0: i32):
v6 = arith.constant 0 : i32;
v4 = arith.sub v6, v0 : i32 #[overflow = wrapping];
- v5 = arith.sext v4 : i32;
+ v5 = wasm.i32_extend_8s v4 : i32;
builtin.ret v5;
};
diff --git a/tests/integration/expected/neg_i8.masm b/tests/integration/expected/neg_i8.masm
index 1bcde10d..7dbd0703 100644
--- a/tests/integration/expected/neg_i8.masm
+++ b/tests/integration/expected/neg_i8.masm
@@ -24,5 +24,16 @@ pub proc entrypoint(i32) -> i32
push.0
swap.1
u32wrapping_sub
+ push.255
+ u32and
+ dup.0
+ push.128
+ u32and
+ eq.128
+ push.0
+ push.4294967040
+ movup.2
+ cdrop
+ u32or
end
diff --git a/tests/integration/expected/shl_i8.hir b/tests/integration/expected/shl_i8.hir
index bdc582a6..61ccf782 100644
--- a/tests/integration/expected/shl_i8.hir
+++ b/tests/integration/expected/shl_i8.hir
@@ -6,7 +6,7 @@ builtin.component root_ns:root@1.0.0 {
v6 = arith.band v1, v10 : i32;
v7 = hir.bitcast v6 : u32;
v8 = arith.shl v0, v7 : i32;
- v9 = arith.sext v8 : i32;
+ v9 = wasm.i32_extend_8s v8 : i32;
builtin.ret v9;
};
diff --git a/tests/integration/expected/shl_i8.masm b/tests/integration/expected/shl_i8.masm
index 9eb91e97..5ccca7a0 100644
--- a/tests/integration/expected/shl_i8.masm
+++ b/tests/integration/expected/shl_i8.masm
@@ -25,5 +25,16 @@ pub proc entrypoint(i32, i32) -> i32
movup.2
u32and
u32shl
+ push.255
+ u32and
+ dup.0
+ push.128
+ u32and
+ eq.128
+ push.0
+ push.4294967040
+ movup.2
+ cdrop
+ u32or
end
diff --git a/tests/integration/expected/sub_i8.hir b/tests/integration/expected/sub_i8.hir
index a7d64c84..13c29d49 100644
--- a/tests/integration/expected/sub_i8.hir
+++ b/tests/integration/expected/sub_i8.hir
@@ -3,7 +3,7 @@ builtin.component root_ns:root@1.0.0 {
public builtin.function @entrypoint(v0: i32, v1: i32) -> i32 {
^block6(v0: i32, v1: i32):
v5 = arith.sub v0, v1 : i32 #[overflow = wrapping];
- v6 = arith.sext v5 : i32;
+ v6 = wasm.i32_extend_8s v5 : i32;
builtin.ret v6;
};
diff --git a/tests/integration/expected/sub_i8.masm b/tests/integration/expected/sub_i8.masm
index 11c83459..b0c5a0c3 100644
--- a/tests/integration/expected/sub_i8.masm
+++ b/tests/integration/expected/sub_i8.masm
@@ -23,5 +23,16 @@ end
pub proc entrypoint(i32, i32) -> i32
swap.1
u32wrapping_sub
+ push.255
+ u32and
+ dup.0
+ push.128
+ u32and
+ eq.128
+ push.0
+ push.4294967040
+ movup.2
+ cdrop
+ u32or
end |
b10aa4c to
59c3a2f
Compare
|
All comments addressed and I've implemented After changing the lowering to emit a Example: Masm generated for addition of `i8` |
codegen/masm/src/lower/lowering.rs
Outdated
| impl HirLowering for wasm::I32Extend8S { | ||
| fn emit(&self, emitter: &mut BlockEmitter<'_>) -> Result<(), Report> { | ||
| let mut inst_emitter = emitter.inst_emitter(self.as_operation()); | ||
| let operand = inst_emitter.pop().expect("operand stack is empty"); |
There was a problem hiding this comment.
You need to manipulate the logical operand stack if the emitter directly here (i.e. via stack_mut), or this will emit actual stack manipulation instructions, which we don't want in this case.
There was a problem hiding this comment.
Changed it to modify the OperandStack in 5362735.
There was a problem hiding this comment.
Actually sign extension might not work as expected when changing the type to I8 only on the logical stack as in 5362735. Since there are multiple things in flight, it's getting difficult to keep tabs on everything. So I would like to suggest the following:
- For now, land the initial wasm dialect with
inst_emitter.trunc(I8)followed byinst_emitter.sext(I32)in theI32Extend8Slowering. As I understand it, that's inefficient but correct. - Open a follow-up issue to optimize the
wasm::I32Extend8Slowering by getting rid of thetrunc. - Fix the encoding of smallints: miden-debug-i32
I've assembled these changes locally and then the overflowing arith tests pass for i8 (along with all other tests). Proceeding from there would be easier, I think.
There was a problem hiding this comment.
I'd like to try and pin down the cause of the issue before we merge - there shouldn't be a difference in observable behavior between emitting trunc and just "casting" the type of the value on top of the operand stack to i8 and sign-extending, if we assume that the lowering from the frontend is correct. The fact that something isn't working means there is a bug somewhere being papered over by emitting the trunc - I'd like us to to dig a bit deeper before we merge to figure that out.
There was a problem hiding this comment.
@bitwalker could you take a look at the test added in e954808? I think it shows that:
- The lowering for
wasm::I32Extend8Sis correct, becausetest_i32_extend8_spasses when the lowering is implemented as follows withtruncinstruction
impl HirLowering for wasm::I32Extend8S {
fn emit(&self, emitter: &mut BlockEmitter<'_>) -> Result<(), Report> {
let mut inst_emitter = emitter.inst_emitter(self.as_operation());
inst_emitter.trunc(&Type::I8, self.span());
inst_emitter.sext(&Type::I32, self.span());
Ok(())
}
}- However
test_i32_extend8_sfails whentruncis not emitted and instead only the type of the operand on the logical stack is modified, as in 5362735.
This should narrow down the problem to "something is wrong when only modifying the operand stack". I'll hold of opening an issue for it, as it's my first time using the builder pattern to construct a package.
greenhat
left a comment
There was a problem hiding this comment.
Besides Paul's notes LGTM!
Sets up the
wasmdialect and adds theI32Extend8Sop to it. See #971 for context and motivation.