#4115 z80: peephole 97b stores the wrong byte of a 16-bit constant

[Chanung Park]

SDCC version: 4.6.0 #16555 (Linux), and trunk r16974 built from source
Target: z80
Host: x86_64 Arch Linux

Peephole 97b in src/z80/peeph.def replaces a constant store through hl followed by a 16-bit constant load into bc or de with the load and a store of the high register of the pair:

replace {
    ld  (hl), #%2
    ld  %1, #%3
} by {
    ; common peephole 97b reused constant loaded into register pair.
    ld  %1, #%3
    ld  (hl), %4
} if same(%1 'bc' 'de'), operandsLiteral(%2 %3), canSplitReg(%1 %4 %5), immdInRange(0x00 0xff '/' %3 0xff %6), immdInRange(0x00 0xff '&' %6 0xff %7), immdInRange(0 0 '-' %7 %2 %8)

The conditions are meant to check that the high byte of %3 equals %2, but the first immdInRange divides %3 by 0xff instead of 0x100. Whenever the quotient differs from the high byte, as for 0x00ff (1 instead of 0) or 0x01ff (2 instead of 1), the rule stores the wrong byte.

Sample code:

void h(unsigned int a, unsigned int b);

void f(unsigned char *p)
{
    *p = 1;
    h(0, 0x00ff);
}

sdcc -mz80 -S peephole97b.c gives

_f::
    ld  de, #0x00ff
    ld  (hl), d
    ld  hl, #0x0000
    jp  _h

which stores 0 through p instead of 1. With --no-peep the store is ld (hl), #0x01.

The attached patch divides by 0x100 instead and adds a regression test, which fails on ucz80 without the patch and passes with it.

I found this while checking SDCC's peephole rules with z80-lift and Alive2.

It also found other rules that can go wrong for some operands. If you are interested, I can file tickets for them; otherwise, feel free to use the list for reference: https://github.com/zlfn/alive-sdcc/blob/main/FINDINGS.md

[Philipp Klaus Krause]

Thanks. The reproducible case is fixed in [r16987].

I haven't looked at the rest in detail yet. On one hand the large number of found counterexamples for peephole optimizer rules in a stable port is quite worrying; on the other hand, that only one out of them could be reproduced from C code explains that the issues were not found earlier. In particular, many of the bugs are about missing conditions on the h flag, and AFAIK the h flag is currently not used in SDCC codegen.

[Philipp Klaus Krause]

As of [r16995], all issues from the "Wrong code from C" and "Wrong rewrites in the rule text" lists in your link should be fixed.