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

Add iadd_overflow_trap #5123

Merged
merged 12 commits into from
Oct 27, 2022

Conversation

elliottt
Copy link
Member

@elliottt elliottt commented Oct 25, 2022

Add a new instruction iadd_overflow_trap, which is a fused version of iadd_ifcout and trapif. Adding this instruction removes a dependency on the iflags type, and would allow us to move closer to removing it entirely.

The instruction is defined for the i32 and i64 types only, and is currently only used in the legalization of heap_addr.

@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 Oct 25, 2022
Copy link
Member

@fitzgen fitzgen left a comment

Choose a reason for hiding this comment

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

LGTM! Didn't look super closely at non-x64 backends since I'm not super familiar with those architectures.

cranelift/codegen/meta/src/shared/instructions.rs Outdated Show resolved Hide resolved
@elliottt elliottt marked this pull request as ready for review October 25, 2022 22:08
@elliottt
Copy link
Member Author

@uweigand does the s390x implementation of iadd_overflow_trap look okay?

@uweigand
Copy link
Member

@uweigand does the s390x implementation of iadd_overflow_trap look okay?

It looks correct, but I'd prefer if it implemented support for all variants of the ADD LOGICAL instruction (i.e. including the memory-and-register and the 32->64 zero-extended operand versions). In particular, I'm assuming the original iadd_ifcout will go away at some point, and then this will be the only operation that can trigger emission of those instructions.

(The duplication is a bit annoying. Maybe it would be nicer to have a common add_logical helper that could be used both by iadd_overflow_trap -adding the trap- and by iadd_ifcout -ignoring the flags- ? On the other hand, if ifadd_ifcout goes away soon, that probably doesn't matter? Either way, that shouldn't delay merging this PR; I can fix it up afterwards.)

As an independent question, I'm wondering about naming: this is about overflow of unsigned addition, so maybe it should be called uadd_overflow_trap in case we may want to add an sadd_overflow_trap at some point?

@elliottt
Copy link
Member Author

It looks correct, but I'd prefer if it implemented support for all variants of the ADD LOGICAL instruction (i.e. including the memory-and-register and the 32->64 zero-extended operand versions). In particular, I'm assuming the original iadd_ifcout will go away at some point, and then this will be the only operation that can trigger emission of those instructions.

(The duplication is a bit annoying. Maybe it would be nicer to have a common add_logical helper that could be used both by iadd_overflow_trap -adding the trap- and by iadd_ifcout -ignoring the flags- ? On the other hand, if ifadd_ifcout goes away soon, that probably doesn't matter? Either way, that shouldn't delay merging this PR; I can fix it up afterwards.)

Happy to add in the additional variants, I'll try factoring out a helper as you suggest. My plan is to remove iadd_ifcout and the other instructions that use iflags as soon as this PR is merged, as the legalization of heap_addr will no longer rely on them. Once the PR that removes iadd_ifcout and friends is up, we can revisit the duplication.

As an independent question, I'm wondering about naming: this is about overflow of unsigned addition, so maybe it should be called uadd_overflow_trap in case we may want to add an sadd_overflow_trap at some point?

I'd be happy to rename this uadd_overflow_trap instead. I chose this name to mirror the existing name of iadd_ifcout, but would be happy to pick something that conveys the behavior better.

@uweigand
Copy link
Member

Happy to add in the additional variants, I'll try factoring out a helper as you suggest. My plan is to remove iadd_ifcout and the other instructions that use iflags as soon as this PR is merged, as the legalization of heap_addr will no longer rely on them. Once the PR that removes iadd_ifcout and friends is up, we can revisit the duplication.

Thanks!

As an independent question, I'm wondering about naming: this is about overflow of unsigned addition, so maybe it should be called uadd_overflow_trap in case we may want to add an sadd_overflow_trap at some point?

I'd be happy to rename this uadd_overflow_trap instead. I chose this name to mirror the existing name of iadd_ifcout, but would be happy to pick something that conveys the behavior better.

I understand the cout in the existing name stands for "carry out", where the use of "carry" instead of "overflow" is supposed to denote that we're preforming the unsigned operation. I guess we could also use iadd_carry_trap vs. iadd_overflow_trap, but mirroring the existing uadd_... vs. sadd_... scheme looks clearer to me.

(Also, as we're already bikeshedding on the name, all the other trap operations have the condition after the word "trap", e.g. trapz for trap-if-zero. So maybe it should actually be uadd_trap_overflow for unsigned-add-and-trap-if-overflow?)

@afonso360
Copy link
Contributor

Add a new instruction iadd_overflow_trap, which is a fused version of iadd_ifcout and trapif.

I understand both of those instructions are going away, but would it be better to implement this as an optimized lowering of trapnz + iadd_cout instead of adding a new specialized instruction?

@uweigand
Copy link
Member

Add a new instruction iadd_overflow_trap, which is a fused version of iadd_ifcout and trapif.

I understand both of those instructions are going away, but would it be better to implement this as an optimized lowering of trapnz + iadd_cout instead of adding a new specialized instruction?

I believe the current ISLE logic doesn't really allow folding an instruction with two outputs (like iadd_cout), so I don't think this would be possible.

However, either way, I think it's a good point: we should also make a decision on the other "carry" type instructions (iadd_cin, iadd_cout, iadd_carry). Right now those are defined, but not implemented on any target. I think they should either be implemented, or else removed.

@afonso360
Copy link
Contributor

afonso360 commented Oct 26, 2022

(Now that I'm re-reading it, I think I worded my earlier comment quite poorly, I didn't mean to imply that trapnz or iadd_cout were going away!)

I believe the current ISLE logic doesn't really allow folding an instruction with two outputs (like iadd_cout), so I don't think this would be possible.

That's unfortunate. But In that case I think the current approach makes sense.

However, either way, I think it's a good point: we should also make a decision on the other "carry" type instructions (iadd_cin, iadd_cout, iadd_carry). Right now those are defined, but not implemented on any target. I think they should either be implemented, or else removed.

I think when we discussed this in the past It was mentioned that they would be useful for cg_clif. I'm going to try to find the link to those discussions.

If we do decide to keep those, I don't mind working on an implementation for them.

Edit:
Here's some discussion about cg_clif needing them
Here's someone else on zulip mentioning that iadd_carry would be useful for them

@uweigand
Copy link
Member

I think when we discussed this in the past It was mentioned that they would be useful for cg_clif.

The Rust standard library provides routines like overflowing_add and (still unstable) carrying_add, whose semantics map onto those instructions:
https://doc.rust-lang.org/std/primitive.u32.html#method.overflowing_add
https://doc.rust-lang.org/std/primitive.u32.html#method.carrying_add

While I believe at the moment they're not actually implemented using those clif instructions at the moment, I guess the compiler could be improved to do so at some point. All in all, it's probably best to keep the clif instructions, since there's no real other way to expose those particular instructions (which many ISAs do have).

@elliottt
Copy link
Member Author

@uweigand I've renamed the new instruction to uadd_overflow_trap, and filled out the remaining cases in the s390x backend.

This did get me wondering if we should call the operation uadd_carry_trap, but I think at this point I'm going to leave it as-is, and we can change the name again later if it's bothering anyone :)

@uweigand
Copy link
Member

@uweigand I've renamed the new instruction to uadd_overflow_trap, and filled out the remaining cases in the s390x backend.

Thanks! This LGTM now.

This did get me wondering if we should call the operation uadd_carry_trap, but I think at this point I'm going to leave it as-is, and we can change the name again later if it's bothering anyone :)

Agreed. I prefer "unsigned overflow / signed overflow" over "carry / overflow" since the former is simply more explicit ...

@elliottt elliottt merged commit 0262044 into bytecodealliance:main Oct 27, 2022
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.

4 participants