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 incranelift-codegen
proper, and is not used bycranelift-wasm
; if at leastcg_clif
can adapt to these operators instead (cc @bjorn3 ?) then I like the explicitness of these opcode names much better and we should removeiadd_cout
.If others agree, I'm happy to do a full review here!
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.
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.
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
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
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: Dec 23 2024 at 12:05 UTC