#3484 Regression for MCS51 on Debian from SDCC 3.8 (buster) to 4.0 (bullseye)

[Martin Homuth-Rosemann]

SDCC version 4.0 on debian stable (bullseye) creates slower ISR code than the previous version 3.8 from debian oldstable (buster).
Used version (sdcc -v)

SDCC : mcs51/z80/z180/r2k/r3ka/gbz80/tlcs90/ez80z80/ds390/TININative/ds400/hc08/s08/stm8/pdk13/pdk14/pdk15 4.0.0 #11528 (Linux)
published under GNU General Public License (GPL)

The original code
if ( ledcounter && --ledcounter == 0 ) {...}
has to be unrolled to
if ( ledcounter ) { --ledcounter; if ( ledcounter == 0 ) {...} }
otherwise the asm code uses intermediate registers that are pushed/popped and slow down the execution.
Workaround in my code (real code snippet):

void timer2_isr( void ) __interrupt TF2_ISR {
    /* Toggle the probe calibration pin. */
    TOGGLE_CALIBRATION_PIN();
#ifdef LED_RED_TOGGLE
    // Avoid nasty sdcc 4.0 REGRESSION:
    // Do not use "if ( ledcounter && --ledcounter == 0 )"
    // Write separate statements!
    // Otherwise the ISR uses registers for reload -> additional push/pop ...
    // ... more cycles, fails for 100 kHz
    if ( ledcounter ) {
        --ledcounter;
        if ( ledcounter == 0 ) {  // led timed out?
            ledcounter = ledinit; // reload
            LED_RED_TOGGLE();
        }
    }
#endif
    TF2 = 0;
}

I've attached a stripped down test file isr_mcs51_test.c with two ISR, one with my old code for 3.8, the other ISR with my 4.0 code.
It compiles to assembler code with
sdcc -mmcs51 -c -o isr_mcs51_test.rel isr_mcs51_test.c
and links with
sdcc -mmcs51 -o isr_mcs51_test.hex isr_mcs51_test.rel

The relevant part of the created *.asm file shows the usage of ar6 and ar7 in the compact code while the unrolled code doesn't do so.

;------------------------------------------------------------
;Allocation info for local variables in function 'timer1_isr'
;------------------------------------------------------------
;   isr_mcs51_test.c:16: void timer1_isr( void ) __interrupt 3 {
;   -----------------------------------------
;    function timer1_isr
;   -----------------------------------------
_timer1_isr:
    ar7 = 0x07
    ar6 = 0x06
    ar5 = 0x05
    ar4 = 0x04
    ar3 = 0x03
    ar2 = 0x02
    ar1 = 0x01
    ar0 = 0x00
    push    acc
    push    ar7
    push    ar6
    push    psw
    mov psw,#0x00
;   isr_mcs51_test.c:18: PA7 = !PA7;
    cpl _PA7
;   isr_mcs51_test.c:20: if ( ledcounter && --ledcounter == 0 ) { // led timed out?
    mov a,_ledcounter
    orl a,(_ledcounter + 1)
    jz  00102$
    mov a,_ledcounter
    add a,#0xff
    mov r6,a
    mov a,(_ledcounter + 1)
    addc    a,#0xff
    mov r7,a
    mov _ledcounter,r6
    mov (_ledcounter + 1),r7
    mov a,r6
    orl a,r7
    jnz 00102$
;   isr_mcs51_test.c:21: ledcounter = ledinit; // reload
    mov _ledcounter,_ledinit
    mov (_ledcounter + 1),(_ledinit + 1)
;   isr_mcs51_test.c:23: PC0 = !PC0;
    cpl _PC0
00102$:
;   isr_mcs51_test.c:25: TF1 = 0;
;   assignBit
    clr _TF1
;   isr_mcs51_test.c:26: }
    pop psw
    pop ar6
    pop ar7
    pop acc
    reti
;   eliminated unneeded push/pop dpl
;   eliminated unneeded push/pop dph
;   eliminated unneeded push/pop b
;------------------------------------------------------------
;Allocation info for local variables in function 'timer2_isr'
;------------------------------------------------------------
;   isr_mcs51_test.c:30: void timer2_isr( void ) __interrupt 5 {
;   -----------------------------------------
;    function timer2_isr
;   -----------------------------------------
_timer2_isr:
    push    acc
    push    psw
;   isr_mcs51_test.c:32: PA7 = !PA7;
    cpl _PA7
;   isr_mcs51_test.c:34: if ( ledcounter ) {
    mov a,_ledcounter
    orl a,(_ledcounter + 1)
    jz  00104$
;   isr_mcs51_test.c:35: --ledcounter;
    dec _ledcounter
    mov a,#0xff
    cjne    a,_ledcounter,00116$
    dec (_ledcounter + 1)
00116$:
;   isr_mcs51_test.c:36: if ( ledcounter == 0 ) {  // led timed out?
    mov a,_ledcounter
    orl a,(_ledcounter + 1)
    jnz 00104$
;   isr_mcs51_test.c:37: ledcounter = ledinit; // reload
    mov _ledcounter,_ledinit
    mov (_ledcounter + 1),(_ledinit + 1)
;   isr_mcs51_test.c:39: PC0 = !PC0;
    cpl _PC0
00104$:
;   isr_mcs51_test.c:42: TF2 = 0;
;   assignBit
    clr _TF2
;   isr_mcs51_test.c:43: }
    pop psw
    pop acc
    reti
;   eliminated unneeded mov psw,# (no regs used in bank)
;   eliminated unneeded push/pop dpl
;   eliminated unneeded push/pop dph
;   eliminated unneeded push/pop b

[Benedikt Freisen]

Can you retry with a more recent version of SDCC?
The most recent stable release is 4.2.0.
Alternatively, you could try an even more recent snapshot version. (currently 4.2.9)

[Martin Homuth-Rosemann]

Hello Benedikt,
Unfortunately, there is no newer version available on Debian - not even on Sid. Could you quickly try my attached test code and see if it still behaves the same. If the behaviour has already been fixed, please ignore my ticket.
Many thanks!
Martin Homuth-Rosemann

[Benedikt Freisen]

I can confirm that my local 4.2.9 build generates the same code as 4.0.0.

[Martin Homuth-Rosemann]

Thank you very much for the review. Since I have already found a workaround, I mainly wanted to give a hint on possible regressions or future optimisation potential.
BTW: Do you know if I can optimize the code execution speed for Cypress EZ-USB (CY7C68013A), --opt-code-speed did not make any difference?

[Philipp Klaus Krause]

In general, the optimization goals (default vs. --opt-code-size vs. --opt-code-speed) don't have a strong effect on mcs51. E.g. https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/dhrystone-mcs51-score.svg shows speed optimization giving 0.5% higher Dhrystone score than size optimization on mcs51, while https://sourceforge.net/p/sdcc/code/HEAD/tree/trunk/sdcc-extra/historygraphs/dhrystone-stm8-score.svg shows a 6.1% higher score for stm8.
And while --max-allocs-per-node provides a trade-off between compilation time and code quality for most ports, that one doesn't do much on mcs51 either.
IMO, lack of optimization is today the main weakness of the mcs51 backend.

[Maarten Brock]

Not sure what kind of optimization you're looking for, but since SDCC doesn't use the second DPTR nor auto-incrementing, you may find places where manual assembly can improve things a lot. And in my experience it also helped to manually loop unroll the handling of USB packets with 64 bytes exactly in a larger transaction.