S390x: implement arith overflow by theotherjimmy · Pull Request #12523

S390x: implement arith overflow by theotherjimmy · Pull Request #12523 · bytecodealliance/wasmtime

Implement the following isle instructions for S390x:

uadd_overflow
sadd_overflow
usub_overflow
ssub_overflow
umul_overflow
smul_overflow

		; lgbr %r3, %r2
		; msgr %r5, %r3
		; lgr %r2, %r5
		; nill %r2, 255

Choose a reason for hiding this comment

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

This is unnecessary - the high bytes are considered don't care.

		; msgr %r5, %r3
		; lgr %r2, %r5
		; nill %r2, 255
		; srlk %r3, %r5, 8

Choose a reason for hiding this comment

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

This seems just wrong. The second return should be a boolean indicating whether overflow happened, not simply the high part of the widened multiplication.

Choose a reason for hiding this comment

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


		; VCode:
		; block0:
		; lgfr %r5, %r3

Choose a reason for hiding this comment

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

This needs to sign-extend from byte, not word.

Choose a reason for hiding this comment

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

Ah, oops.


		; VCode:
		; block0:
		; lgfr %r5, %r3

Choose a reason for hiding this comment

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

And this needs to sign-extend from halfword.

		; sllk %r3, %r2, 24
		; msrkc %r5, %r3, %r5
		; lhi %r3, 0
		; lochio %r3, 1

Choose a reason for hiding this comment

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

This is wrong, the CC checks for signed overflow, not unsigned overflow. For umul_overflow, you'll have to either multiply in a wider mode, or use a widening multiply, and then check the high part is nonzero to detect unsigned overflow.

You should enable the run-time filetests on s390x as well, these would probably have caught some of those errors:
filetests/filetests/runtests/*overflow*.clif

@uweigand I ran all these tests on my lpar. The current version passes all enabled runtests on s390x, and I enabled the runtests for these instructions.

I missed the cargo test tests. My bad. Please hold off on reviewing while I fix them.

It looks like I need to rebase onto main. I'll do that now then. Tests still pass with the rebase onto main.

Fixed cargo test locally.

Rebased for the regalloc2 version bump. Re-blessed all new tests. ran all runtests on my s390x LPAR, they passed.

Rebased for the new types-in-lower-instructions update

Choose a reason for hiding this comment

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

Sorry for the delay in reviewing. This all looks functionally correct to me now, see a few additional cosmetic comments inline.

Also, I think you should also enable the uadd_overflow_128.clif runtest test case.

I've handled most of the review locally. I'll be adding the uadd overflow i128 and verifying locally before pushing that up.