| Message ID | 20231031053718.347100-1-iii@linux.ibm.com |
|---|---|
| Headers | show |
| Series | target/s390x: CC fixes | expand |
On 31.10.23 06:32, Ilya Leoshkevich wrote: > Hi, > > This series fixes two issues with updating CC. David was suggesting a > bigger rewrite [1], but I did not dare do this (yet). Instead, these I started coding that up but was distracted by other things; last time I looked at that, I concluded that the way we are calculating the carry in not suitable when we're doing two additions (like ADD LOGICAL WITH CARRY). In addition, the way cout_addu32/cout_addu64 relies on cc_src/cc_dst is wrong in some occurrences. > are targeted fixes: patch 1 helps with installing Fedora, and patch 3 > addresses something I noticed when reviewing the code. Patches 2 and 4 > are tests. I assume you are only scratching the surface with your fixes and we might be better off just doing CC_OP_ADDU / CC_OP_SUBU like CC_OP_ADD_32 / CC_OP_ADD_64 / ... to fix all issues involved. > > Best regards, > Ilya > > [1] https://gitlab.com/qemu-project/qemu/-/issues/1865#note_1545060658 > > Ilya Leoshkevich (4): > target/s390x: Fix CLC corrupting cc_src > tests/tcg/s390x: Test CLC with inaccessible second operand > target/s390x: Fix LAALG not updating cc_src > tests/tcg/s390x: Test LAALG with negative cc_src > > target/s390x/tcg/translate.c | 10 ++++--- > tests/tcg/s390x/Makefile.target | 2 ++ > tests/tcg/s390x/clc.c | 48 +++++++++++++++++++++++++++++++++ > tests/tcg/s390x/laalg.c | 27 +++++++++++++++++++ > 4 files changed, 84 insertions(+), 3 deletions(-) > create mode 100644 tests/tcg/s390x/clc.c > create mode 100644 tests/tcg/s390x/laalg.c >
On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote: > On 31.10.23 06:32, Ilya Leoshkevich wrote: > > Hi, > > > > This series fixes two issues with updating CC. David was suggesting > > a > > bigger rewrite [1], but I did not dare do this (yet). Instead, > > these > > I started coding that up but was distracted by other things; last > time I > looked at that, I concluded that the way we are calculating the carry > in > not suitable when we're doing two additions (like ADD LOGICAL WITH > CARRY). Do you per chance remember any details? IIUC the code in question is: static DisasJumpType op_addc64(DisasContext *s, DisasOps *o) { compute_carry(s); TCGv_i64 zero = tcg_constant_i64(0); tcg_gen_add2_i64(o->out, cc_src, o->in1, zero, cc_src, zero); tcg_gen_add2_i64(o->out, cc_src, o->out, cc_src, o->in2, zero); return DISAS_NEXT; } This looks correct to me, because the 128-bit result of the first addition is passed fully to the second addition, and the upper half is always either 0 or 1. I played with chaining ADD-LOGICAL-WITH-CARRYs with various inputs, and could not produce any wrong calculations in the emulation. Best regards, Ilya [...]
On 03.11.23 17:44, Ilya Leoshkevich wrote: > On Tue, 2023-10-31 at 09:38 +0100, David Hildenbrand wrote: >> On 31.10.23 06:32, Ilya Leoshkevich wrote: >>> Hi, >>> >>> This series fixes two issues with updating CC. David was suggesting >>> a >>> bigger rewrite [1], but I did not dare do this (yet). Instead, >>> these >> >> I started coding that up but was distracted by other things; last >> time I >> looked at that, I concluded that the way we are calculating the carry >> in >> not suitable when we're doing two additions (like ADD LOGICAL WITH >> CARRY). > > Do you per chance remember any details? IIUC the code in question is: Unfortunately, I don't. I thought there would be a case where we could overflow twice, and result in a carry value of 2. Or some other weird corner case where the result would not be expressive. Maybe I was daydreaming, let me see if I can re-discover what I found (should have taken notes but was just briefly looking at this). If not, your fixes might be just good enough.