#3607 mcs51: genGetWord wrong code with overlapping input/output registers

[Oleg Endo]

The attached reproducer hits a bug related to genGetWord, where the input operands overlap with the output operands. I've attached the reproducer source and asm output that I get when compiled with

sdcc -c --model-large --std-sdcc2x -mmcs51 --stack-auto --fomit-frame-pointer --nogcse  bug.c

The following is the problematic code snippet:

    jnb _g_arg_2,00110$
    mov r5,#0xe6
    mov r4,#0xdf
    mov r3,#0x0c
    mov r2,#0x00
    sjmp    00111$
00110$:
    mov r5,#0x3c
    mov r4,#0x59
    mov r3,#0x2e
    mov r2,#0x00
00111$:
    mov ar2,r3      <<<  input operand: [r3:r2]
    mov ar3,r2      <<<  output operand: [r2:r3]
                    <<<  wrong result: r2 = r3
    clr c
    mov a,r2
    subb    a,r6
    mov a,r3
    subb    a,r7
    jnc 00108$

I've tried to dig a bit through the code gen code. 'genGetWord' doesn't check for overlapping input/output regs. 'sameRegs' would also not catch this case.

The problem could be handled in genGetWord by converting the 2 mov insns into 1 xch insn or such. But I'm not sure whether this could be an actual problem in the register allocation, or something else that makes it select overlapping register ranges.

[Philipp Klaus Krause]

What is the output of sdcc --version?

[Oleg Endo]

SDCC : mcs51/z80/z180/r2k/r2ka/r3ka/gbz80/tlcs90/ez80_z80/z80n/ds390/TININative/ds400/hc08/s08/stm8 4.1.6 #12575 (Linux)

I'm pinned to that older version for now. Can you please check if the bug is also present on a more recent version?

[Philipp Klaus Krause]

I can reproduce the issue in current SDCC 4.3.0 RC1 on my amd64 Debian GNU/Linux testing system.

[Oleg Endo]

Thanks for checking.

The attached patch fixes the issue for me. Feedback appreciated!

[Philipp Klaus Krause]

Looks ok at first sight, though I didn't test it yet.

Two minor points:
* The middle case could be left out, as the fallback we get to in that case does the same.
* FALSE shouldn't be used in new code, please use false instead.

[Oleg Endo]

Yeah, that's right -- the middle case is the same as the "normal" case. Just wanted to leave it in there for clarity that this case is also taken care of, but I'm also OK to omit it.

Attached patch fixes the 'FALSE' -> 'false' thing.

[Philipp Klaus Krause]

Thanks. Could you also add a regression test to your patch, i.e. a file support/regression/tests/bug-3607.c that fails an ASSERT on SDCC with the bug present (when doing one of the sdcc related tests, e.g. make test-mcs51-large-stack-auto), but passes with the patch?

[Oleg Endo]

I've added the test case. It was a bit difficult to construct and I'm not sure how it will hold up if other optimizations happen in the future (constant propagation, inlining ... ). But perhaps better than nothing.

[Philipp Klaus Krause]

Thanks. I've applied the patch to the next branch in [r14197] (with minor changes).

[Oleg Endo]

Thanks!

Another unrelated thing, out of curiosity ...
Is there any way to achieve the below thing within the exising SDCC datastructures/framework?

I know once it gets to the 'gen ...' parts of the backend it's already too late to do it. Where would be the appropriate place to do this?

    jnb _g_arg_2,00110$
    mov r5,#0xe6
    mov r4,#0xdf
    mov r3,#0x0c
    mov r2,#0x00
    sjmp    00111$
00110$:
    mov r5,#0x3c
    mov r4,#0x59
    mov r3,#0x2e
    mov r2,#0x00
00111$:
    mov ar2,r3        << could omit these ...
    mov ar3,r2      

    clr c
    mov a,r2          << and rename the registers 
    subb    a,r6
    mov a,r3          << here, where they're used
    subb    a,r7
    jnc 00108$

[Philipp Klaus Krause]

1) The quick-and-dirty way: introduce a suitable peephole optimizer rule that applies to this case. Not very flexible, but easy to do, without requirinrg much knowledge about SDCC.
2) The right way: switch mcs51 to the new register allocator. This is a multi-month project, that I intend to look into for SDCC 4.5.0.

[Oleg Endo]

Even if the register allocator is improved, there should be a way to modify register assignments after register allocation. Sometimes the register allocator leaves some opportunities on the table, or they emerge later down the optimzation pipeline....

[Philipp Klaus Krause]

Fixed in [r14211] by merging next to trunk.