-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Add iadd_overflow_trap #5123
Conversation
There was a problem hiding this 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.
@uweigand does the s390x implementation of |
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 (The duplication is a bit annoying. Maybe it would be nicer to have a common As an independent question, I'm wondering about naming: this is about overflow of unsigned addition, so maybe it should be called |
Happy to add in the additional variants, I'll try factoring out a helper as you suggest. My plan is to remove
I'd be happy to rename this |
Thanks!
I understand the (Also, as we're already bikeshedding on the name, all the other trap operations have the condition after the word "trap", e.g. |
I understand both of those instructions are going away, but would it be better to implement this as an optimized lowering of |
I believe the current ISLE logic doesn't really allow folding an instruction with two outputs (like However, either way, I think it's a good point: we should also make a decision on the other "carry" type instructions ( |
(Now that I'm re-reading it, I think I worded my earlier comment quite poorly, I didn't mean to imply that
That's unfortunate. But In that case I think the current approach makes sense.
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: |
The Rust standard library provides routines like 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). |
2f91e47
to
4b8db21
Compare
@uweigand I've renamed the new instruction to This did get me wondering if we should call the operation |
Thanks! This LGTM now.
Agreed. I prefer "unsigned overflow / signed overflow" over "carry / overflow" since the former is simply more explicit ... |
Add a new instruction
iadd_overflow_trap
, which is a fused version ofiadd_ifcout
andtrapif
. Adding this instruction removes a dependency on theiflags
type, and would allow us to move closer to removing it entirely.The instruction is defined for the
i32
andi64
types only, and is currently only used in the legalization ofheap_addr
.