Stream: git-wasmtime

Topic: wasmtime / issue #6198 Remove `iadd_cout` and `isub_bout`

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 22:16):

bjorn3 commented on issue #6198:

Can unsigned_add_overflow_condition now be removed from TargetIsa?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 22:22):

T0b1-iOS commented on issue #6198:

Can unsigned_add_overflow_condition now be removed from TargetIsa?

I would think that there is no reasonable use-case anymore since I don't see a way to get access to any flag for an add directly and with compares it does not make a lot of sense.
I don't know if there is anyone who uses this, though.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 22:50):

jameysharp commented on issue #6198:

Good catch! It looks like the only real callers of unsigned_add_overflow_condition were deleted last year, in #5123 and #5162.

So yeah, I'd say let's delete that from both TargetIsa and cranelift-wasm's FuncEnvironment.

But I'd prefer to do that deletion in a separate PR. It shouldn't overlap with this one I think, so we can merge them in any order.

Meanwhile, this PR looks like exactly the deletions I'd expect for removing these two instructions, so it's probably in good shape. I just want to ask, are the deleted tests and lowering rules all covered by equivalents for the new instructions? I didn't look closely at #5784.

Also I want to double-check with @cfallin that there are no concerns about dropping iadd_cout and isub_bout now that we have equivalents with defined signed/unsigned behavior. Should we maybe keep both versions around for one release, for easier migration?

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 23:15):

T0b1-iOS commented on issue #6198:

Meanwhile, this PR looks like exactly the deletions I'd expect for removing these two instructions, so it's probably in good shape. I just want to ask, are the deleted tests and lowering rules all covered by equivalents for the new instructions? I didn't look closely at #5784.

The lowering rules are covered, isub_bout didn't have any and iadd_cout was only implemented on arm and x64 which the new instructions are, too.
The tests seem to cover the case where the overflow happens (so 0x7E + 1 and 0x7E + 2 essentially) which the new instructions also test.

view this post on Zulip Wasmtime GitHub notifications bot (Apr 11 2023 at 23:19):

cfallin commented on issue #6198:

Also I want to double-check with @cfallin that there are no concerns about dropping iadd_cout and isub_bout now that we have equivalents with defined signed/unsigned behavior. Should we maybe keep both versions around for one release, for easier migration?

I think it's fine to remove them now, given that we aren't losing any functionality, and the migration for any users we don't know about should be simple. The only other major user I'd be concerned about w.r.t. compatibility is cg_clif, and @bjorn3 is here aware of this PR already, so it seems we're fine!

Last updated: Nov 22 2024 at 17:03 UTC