Stream: git-wasmtime

Topic: wasmtime / issue #5165 cranelift: Rename `iadd_carry`/ `i...

Wasmtime GitHub notifications bot (Nov 01 2022 at 10:36):

afonso360 opened issue #5165:

:wave: Hey,

While implementing iadd_cout for aarch64 I realized that the output of the carry bit depends on signedness of the operation. If we implement it as unsigned we only signal overflow on 0xFF + 1, and if we implement it as signed we signal overflow on 0x7F + 1

Thus should we rename this operation to sadd_cout to keep consistency with our other operations that depend on signedness (i.e. sdiv/udiv)? The operation seems to be signed based on the existing test suite.

Does the existence of sadd_cout also imply that we should have uadd_cout?

The same question applies for iadd_carry, but not for iadd_cin.

Wasmtime GitHub notifications bot (Nov 01 2022 at 10:43):

bjorn3 commented on issue #5165:

Makes sense. I would have expected iadd_cout to be unsigned. As the name carry is often used for unsigned overflow AFAIK. For example x86 has the carry and overflow flags for unsigned cq signed overflow.

Does the existence of sadd_cout also imply that we should have uadd_cout?

IMO yes there should be.

Wasmtime GitHub notifications bot (Nov 01 2022 at 11:28):

afonso360 commented on issue #5165:

It looks like we also discussed the naming of the new iadd_overflow_trap instruction last week, and it got renamed to uadd_overflow_trap.

And it looks like folks preferred uadd_overflow_trap instead of uadd_carry_trap, so maybe we should apply the same naming convention here? (I'd be in favor of this.)

Wasmtime GitHub notifications bot (Nov 01 2022 at 11:28):

afonso360 edited a comment on issue #5165:

It looks like we also discussed the naming of the new iadd_overflow_trap instruction last week, and it got renamed to uadd_overflow_trap.

And it looks like folks preferred uadd_overflow_trap instead of uadd_carry_trap, so maybe we should apply the same naming convention here? (I'd also be in favor of this.)

Wasmtime GitHub notifications bot (Nov 01 2022 at 11:28):

afonso360 edited a comment on issue #5165:

It looks like we also discussed the naming of the new iadd_overflow_trap instruction last week, and it got renamed to uadd_overflow_trap.

And it looks like folks preferred uadd_overflow_trap instead of uadd_carry_trap, so maybe we should apply the same naming convention here? (I'd be in favor of this.)

Wasmtime GitHub notifications bot (Apr 12 2023 at 09:45):

afonso360 commented on issue #5165:

This was fixed in #5784 and #6198.

Wasmtime GitHub notifications bot (Apr 12 2023 at 09:45):

afonso360 closed issue #5165:

:wave: Hey,

While implementing iadd_cout for aarch64 I realized that the output of the carry bit depends on signedness of the operation. If we implement it as unsigned we only signal overflow on 0xFF + 1, and if we implement it as signed we signal overflow on 0x7F + 1

Thus should we rename this operation to sadd_cout to keep consistency with our other operations that depend on signedness (i.e. sdiv/udiv)? The operation seems to be signed based on the existing test suite.

Does the existence of sadd_cout also imply that we should have uadd_cout?

The same question applies for iadd_carry, but not for iadd_cin.

Last updated: Nov 22 2024 at 16:03 UTC