Stream: git-wasmtime

Topic: wasmtime / issue #5784 Add `{u,s}{add,sub,mul}_overflow` ...

Wasmtime GitHub notifications bot (Feb 16 2023 at 21:46):

cfallin commented on issue #5784:

FWIW, I think that these instructions are a reasonable addition -- thanks for proposing and prototyping this!

I think, as you suggest as well, it's worth clarifying how this interacts with the existing operators; in particular

I left iadd_cout and friends untouched as the semantics are a bit confusing right now, e.g. iadd_cout seems to be indicating signed overflow while the pseudocode in the description describes unsigned overflow).

does indeed seem to be true (e.g. here for x86-64), but is pretty surprising to me, and IMHO is incorrect. It doesn't appear that iadd_cout is the target of any rewrite in cranelift-codegen proper, and is not used by cranelift-wasm; if at least cg_clif can adapt to these operators instead (cc @bjorn3 ?) then I like the explicitness of these opcode names much better and we should remove iadd_cout.

If others agree, I'm happy to do a full review here!

Wasmtime GitHub notifications bot (Feb 16 2023 at 22:40):

bjorn3 commented on issue #5784:

cg_clif currently has all overflow checks manually due to iadd_cout not working in combinations of target archs and integer sizes. I did be more than happy to switch to the _overflow instructions added by this PR.

Wasmtime GitHub notifications bot (Mar 25 2023 at 22:32):

T0b1-iOS commented on issue #5784:

Okay, sorry for the long wait but university got a bit busy the past month. I did the emit tests now (and fixed a few bugs along the way) and I think this is now in a somewhat cleaned up stage and could be reviewed.
The CI currently fails because of the s390x tests failing which is because QEMU incorrectly emulates the overflow flag (see https://gitlab.com/qemu-project/qemu/-/issues/618). This was fixed in QEMU 7.0.0 so the version in the CI would need to be updated but I don't know if I can do that.

Wasmtime GitHub notifications bot (Mar 27 2023 at 11:04):

bjorn3 commented on issue #5784:

Tried this with cg_clif. Didn't find any issues. https://github.com/bjorn3/rustc_codegen_cranelift/tree/clif_overflow_insts

Wasmtime GitHub notifications bot (Mar 27 2023 at 11:04):

bjorn3 edited a comment on issue #5784:

Tried this with cg_clif. Didn't find any failing tests. https://github.com/bjorn3/rustc_codegen_cranelift/tree/clif_overflow_insts

Wasmtime GitHub notifications bot (Apr 11 2023 at 19:21):

T0b1-iOS commented on issue #5784:

This looks good now -- thanks so much for your patience as we iterated on the implementation!

No problem, I was the slow one, after all ^^

I think once the merge conflict in the interpreter is resolved, this should be good to merge.

I'll rebase the branch if that's okay.

Last updated: Nov 22 2024 at 17:03 UTC