S390x: implement arith overflow#12523
S390x: implement arith overflow#12523theotherjimmy wants to merge 6 commits intobytecodealliance:mainfrom
Conversation
theotherjimmy
requested review from
alexcrichton
and removed request for
a team
|
r? @uweigand |
github-actions
bot
added
the
cranelift
| ; lgbr %r3, %r2 | ||
| ; msgr %r5, %r3 | ||
| ; lgr %r2, %r5 | ||
| ; nill %r2, 255 |
There was a problem hiding this comment.
This is unnecessary - the high bytes are considered don't care.
| ; msgr %r5, %r3 | ||
| ; lgr %r2, %r5 | ||
| ; nill %r2, 255 | ||
| ; srlk %r3, %r5, 8 |
There was a problem hiding this comment.
This seems just wrong. The second return should be a boolean indicating whether overflow happened, not simply the high part of the widened multiplication.
There was a problem hiding this comment.
Multiply integers unsigned with overflow out.
ofis set when the multiplication overflowed.a &= x * y \pmod 2^B \\ of &= x * y > 2^B
My read is that the overflow out is the whole upper word size of the multiply. My read was incorrectly reading > (greater than) as >> (right shift), I've fixed the PR.
| (producer ProducesFlags (alu_rrr_with_flags_paired ty op x y)) | ||
| (overflow Reg (lower_bool $I8 (bool (produces_flags_ignore producer) cond))) | ||
| (out Reg (lshr_imm $I32 (produces_flags_get_reg producer) (type_shift_up ty)))) | ||
| (output_pair out overflow))) |
There was a problem hiding this comment.
This is more of a cosmetic issue, but I think I'd prefer to keep these types of helpers in inst.isle - see e.g. what we're doing for the ..._sat_reg helpers. Ideally, we'd have a interface between inst.isle and lower.isle that looks more symmetrical across the different types ...
theotherjimmy
force-pushed
the
s390x-arith-overflow
branch
2 times, most recently
from
73de80a to
9761e19
Compare
|
|
||
| ; VCode: | ||
| ; block0: | ||
| ; lgfr %r5, %r3 |
There was a problem hiding this comment.
This needs to sign-extend from byte, not word.
|
|
||
| ; VCode: | ||
| ; block0: | ||
| ; lgfr %r5, %r3 |
There was a problem hiding this comment.
And this needs to sign-extend from halfword.
| ; sllk %r3, %r2, 24 | ||
| ; msrkc %r5, %r3, %r5 | ||
| ; lhi %r3, 0 | ||
| ; lochio %r3, 1 |
There was a problem hiding this comment.
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: |
theotherjimmy
force-pushed
the
s390x-arith-overflow
branch
from
9761e19 to
7d65b04
Compare
theotherjimmy
force-pushed
the
s390x-arith-overflow
branch
from
7d65b04 to
40499a9
Compare
|
@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 |
|
It looks like I need to rebase onto main. I'll do that now then. Tests still pass with the rebase onto main. |
theotherjimmy
force-pushed
the
s390x-arith-overflow
branch
from
40499a9 to
5557ac3
Compare
Note that this also forces all mul to use the *k multiplication instruction, as it sets the condition codes. This will have no affect on instruction stream size, as it's the same size as the non-k variant of this instruction
theotherjimmy
force-pushed
the
s390x-arith-overflow
branch
from
5557ac3 to
a21cf71
Compare
|
Fixed |
3 participants
Implement the following isle instructions for S390x:
uadd_overflowsadd_overflowusub_overflowssub_overflowumul_overflowsmul_overflow