Stream: git-wasmtime

Topic: wasmtime / issue #5123 Add iadd_overflow_trap

Wasmtime GitHub notifications bot (Oct 25 2022 at 22:08):

elliottt commented on issue #5123:

@uweigand does the s390x implementation of iadd_overflow_trap look okay?

Wasmtime GitHub notifications bot (Oct 26 2022 at 07:11):

uweigand commented on issue #5123:

@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?

Wasmtime GitHub notifications bot (Oct 26 2022 at 07:24):

elliottt commented on issue #5123:

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.

Wasmtime GitHub notifications bot (Oct 26 2022 at 07:51):

uweigand commented on issue #5123:

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?)

Wasmtime GitHub notifications bot (Oct 26 2022 at 08:07):

afonso360 commented on issue #5123:

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?

Wasmtime GitHub notifications bot (Oct 26 2022 at 08:24):

uweigand commented on issue #5123:

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.

Wasmtime GitHub notifications bot (Oct 26 2022 at 08:50):

afonso360 commented on issue #5123:

(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 way!)

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.

Wasmtime GitHub notifications bot (Oct 26 2022 at 08:54):

afonso360 edited a comment on issue #5123:

(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.

Wasmtime GitHub notifications bot (Oct 26 2022 at 09:14):

afonso360 edited a comment on issue #5123:

(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

Wasmtime GitHub notifications bot (Oct 26 2022 at 16:12):

uweigand commented on issue #5123:

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).

Wasmtime GitHub notifications bot (Oct 26 2022 at 18:12):

elliottt commented on issue #5123:

@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 :)

Wasmtime GitHub notifications bot (Oct 26 2022 at 18:28):

uweigand commented on issue #5123:

@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 ...

Last updated: Nov 22 2024 at 16:03 UTC