#3386 New test bug-3385.c fails for hc08 / s08

[Philipp Klaus Krause]

The new regression test bug-3385.c (for a bug in a peephole rule for stm8) fails foc hc08 and s08. Looks like bad code is generated silently. I'll disable the test for those two ports for now.

[Philipp Klaus Krause]

Fails for mos6502, too.

[Gabriele Gorla]

I took a look at the code generation for both mos6502 and s08. The generated code is obviously incorrect. However it seems that the codegen is correctly translating the icode.
I then compared the icodes with the z80 which is passing and I noticed there is a key difference:

S08 and MOS6502:

;ic:4:  iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}[a x ] := iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_7}[a x ]

Z80:

;ic:4:  iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}[e d ] = iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_7}[e d ] + 0x2 {const-unsigned-char literal}

the extra + 0x2 is the key to generate the correct code.
how can I trace why the +2 is not in the 6502 icode?

[Philipp Klaus Krause]

You can compile with --dump-i-code, to get 14 iCode dumps from different stages in SDCC, and look at them to see at which point the bug gets introduced.

P.S.: If you are wondering about the order of those dumps, look at the corresponding calls to dumpEbbsToFileExt in src/SDCCopt.c and src/mos6502/ralloc.c.

[Gabriele Gorla]

It seems the issue is introduced by the register packing.

bug-3385.dumprange

tests/bug-3385.c(l20:s1:k0:d0:s0:b9)     _entry($2) :
tests/bug-3385.c(l20:s2:k1:d0:s0:b9)        proc _event_init [k1 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( struct __00000000 generic* fixed) fixed}
tests/bug-3385.c(l20:s3:k2:d0:s0:b9)    iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_8} = recv _event_init [k1 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( struct __00000000 generic* fixed) fixed}
tests/bug-3385.c(l22:s4:k4:d0:s0:b9)        iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed} = iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_8} + 0x2 {const-unsigned-char literal}
tests/bug-3385.c(l22:s5:k10:d0:s0:b9)       *(iTemp2 [k6 lr4:5 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}) := iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}

bug-3385.dumppack

tests/bug-3385.c(l20:s1:k0:d0:s0:b9)     _entry($2) :
tests/bug-3385.c(l20:s2:k1:d0:s0:b9)        proc _event_init [k1 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( struct __00000000 generic* fixed) fixed}
tests/bug-3385.c(l20:s3:k2:d0:s0:b9)    iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_8} = recv _event_init [k1 lr0:0 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{void function ( struct __00000000 generic* fixed) fixed}
tests/bug-3385.c(l22:s4:k4:d0:s0:b9)        iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed} := iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_8}
tests/bug-3385.c(l22:s5:k10:d0:s0:b9)       *(iTemp2 [k6 lr4:5 so:0]{ ia1 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}) := iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed}

the correct 3rd line (from Z80 output) should be:
iTemp2 [k6 lr4:5 so:0]{ ia0 a2p0 re0 rm0 nos0 ru0 dp0}{struct list_node_t generic* fixed} = iTemp0 [k2 lr3:4 so:0]{ ia0 a2p0 re1 rm0 nos0 ru0 dp0}{struct __00000000 generic* fixed}{ sir@ _event_init_self_10000_8} + 0x2 {const-unsigned-char literal}

On this line there are 2 differences between the passing Z80 and the failing 6502
the mos6502 is missing the + 0x2
in the mos6502 = is replaced by := which I do not understand the difference.

[Gabriele Gorla]

commenting out the call to packPointerOp in ralloc.c makes the test pass (the function is pretty much identical for HC08 which is consistent with the test failing in the same way).

However, removing the optimization makes the generated code larger and slower.
It seems the logic to detect if the optimization is safe has a bug, but I don't understand the code enough to fix the issue.

[Philipp Klaus Krause]

This pointer packing moves an offset added to a pointer to the pointer accesses. It is meant to allow for better use of indexed addressing modes. Some ports don't have this optimization (e.g. mcs51), some have it for reads from pointers only (e.g. stm8 and z80) and do it earlier (offsetFoldGet in SDCCopt.c).
hc08/s08/mos6502 apparently does it in ralloc.c for both writes and reads; and somehow that offset is lost here.

The bug might still be in code generation: Maybe that +2 isn't really lost, it just doesn't get printed in the iCode dump (since apparently this offset is handled differently for hc08/s07/mos6502 than for other ports)? I see a decodePointerOffset function in code generation that probably should find that +2, maybe that doesn't work here for some reason?

P.S.: Maybe @epetrich can help here, AFAIR he came up with this approach for hc08/s08.