Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove iadd_cout and isub_bout #6198

Merged
merged 1 commit into from
Apr 12, 2023
Merged

Conversation

T0b1-iOS
Copy link
Contributor

@T0b1-iOS T0b1-iOS commented Apr 11, 2023

Followup to #5784.
This removes the iadd_cout and isub_bout instructions which can be replaced with {u,s}{add,sub}_overflow directly.

Searching for 'iadd_cout', 'isub_bout', 'IaddCout' and 'IsubBout' does not find anything anymore so I hope I deleted everything.

@T0b1-iOS T0b1-iOS requested a review from a team as a code owner April 11, 2023 22:04
@T0b1-iOS T0b1-iOS requested review from jameysharp and removed request for a team April 11, 2023 22:04
@bjorn3
Copy link
Contributor

bjorn3 commented Apr 11, 2023

Can unsigned_add_overflow_condition now be removed from TargetIsa?

@T0b1-iOS
Copy link
Contributor Author

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.

@github-actions github-actions bot added cranelift Issues related to the Cranelift code generator cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. labels Apr 11, 2023
@jameysharp
Copy link
Contributor

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?

@T0b1-iOS
Copy link
Contributor Author

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.

@cfallin
Copy link
Member

cfallin commented Apr 11, 2023

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!

Copy link
Contributor

@jameysharp jameysharp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alright, let's do this then!

@jameysharp jameysharp added this pull request to the merge queue Apr 11, 2023
Merged via the queue into bytecodealliance:main with commit f684a5f Apr 12, 2023
@T0b1-iOS T0b1-iOS deleted the rem_cout branch April 12, 2023 00:15
brendandburns pushed a commit to brendandburns/wasmtime that referenced this pull request Apr 13, 2023
eduardomourar pushed a commit to eduardomourar/wasmtime that referenced this pull request Apr 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cranelift:area:aarch64 Issues related to AArch64 backend. cranelift:area:x64 Issues related to x64 codegen cranelift:meta Everything related to the meta-language. cranelift Issues related to the Cranelift code generator
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants