#884 mcs51: deadMove check in peephole not always working

[Oleg Endo]

When determining whether a move insn is a dead move or not, the code in removeDeadMove, doTermScan, scan4op (mcs51/peep.c) scans through the code following branches to find uses of the set register. It mostly works, but not in all cases it seems.

Examples have been compiled on 4.1.6 (svn r12557) with:
sdcc -c --std-sdcc2x -mmcs51 --model-large --stack-auto --fomit-frame-pointer

__data unsigned char xxx;

unsigned char bleh3 (void)
{
  if ((xxx & 0x1F) == 0x10)
    return 1;
  else
    return 3;
}

compiles to:

_bleh3:
    mov a,#0x1f
    anl a,_xxx
    mov r7,a                  << unnecessary use of temporary 'r7'
    cjne    r7,#0x10,00102$   << 
    mov dpl,#0x01
    ret
00102$:
    mov dpl,#0x03
    ret

adding the following peephole pattern to try to remove the unnecessary temporary register use above:

replace {
    mov r%1,a
    cjne    r%1,%2,%3
} by {
    ; Peephole 800.4
    mov r%1,a
    cjne    a,%2,%3
}

compiles to:

_bleh3:
    mov a,#0x1f
    anl a,_xxx
    mov r7,a                 << dead move after peephole not eliminated
    cjne    a,#0x10,00102$   << peephole is effective
    mov dpl,#0x01
    ret
00102$:
    mov dpl,#0x03
    ret

duplicating Peephole 302 pattern as a last peephole pattern, or setting the original Peephole 302 pattern as restart will eliminate the dead move:

// duplicate simple dead move pattern as last peephole pattern
replace {
    mov r%1,%2
} by {
    ;   Peephole 302.1  mov r%1,%2 removed
} if notVolatile(%2), deadMove(%1)

compiles to:

_bleh3:
    mov a,#0x1f
    anl a,_xxx
    cjne    a,#0x10,00102$
    mov dpl,#0x01
    ret
00102$:
    mov dpl,#0x03
    ret

Although the dead move is eliminated in the above case, in a real firmware project I can still see a lot of cases such as:

    mov r0,#_my_data1
    mov a,@r0
    anl a,#0x1f
    mov r7,a        << dead move remains
    cjne    a,#0x01,00102$
    mov dpl,#0x00
    lcall   _xxxxx
    mov dpl,#0x00
    ret
00102$:
    mov dpl,#0xff
    ret



    mov dptr,#(_my_data + 0x0006)
    movx    a,@dptr
    mov r7,a        << dead move remains
    cjne    a,#0x02,00357$
    sjmp    00126$


    mov dptr,#(_my_data + 0x0006)
    movx    a,@dptr
    mov r7,a
    cjne    a,#0x02,00117$
    push    ar6
    lcall   _xxxxxx
    pop ar6
    sjmp    00126$

    ...

It looks this can be also reproduced, by taking the original example at the top and let it use a 16-bit comparison (caused by integer promotion of the and operation):

__data unsigned char xxx;

unsigned char bleh3 (void)
{
  if ((xxx & 0x1F) == 0x10)
    return 1;
  else
    return 3;
}

With all peepholes above included, complies to:

_bleh3:
    mov r6,_xxx
    anl ar6,#0x1f
    mov r7,#0x00
    cjne    r6,#0x10,00102$
    cjne    r7,#0x00,00102$   << useless comparision of zero-extended high byte
    mov dpl,#0x01
    ret
00102$:
    mov dpl,#0x03
    ret

We can try to remove the useless comparison of 0 == 0 by adding the following peephole (before the duplicated peephole 302 to deal with the dead store):

replace restart {
    mov     %1,#0x00
    cjne    %2,%3,%4
    cjne    %1,#0x00,%4
} by {
    ; Peephole 800.5
    mov     %1,#0x00
    cjne    %2,%3,%4
}

It will eliminate the useless comparison, but the dead move will remain:

_bleh3:
    mov r6,_xxx
    anl ar6,#0x1f
    mov     r7,#0x00      << dead move remains
    cjne    r6,#0x10,00102$
    mov dpl,#0x01
    ret
00102$:
    mov dpl,#0x03
    ret

The check !checkLabelRef in removeDeadMove fails for some reason and it prevents it from realizing that the move is dead.

[Oleg Endo]

Looking at a smaller snippet of a larger function:

    ....
    mov dptr,#(_var + 0x0014)
    movx    a,@dptr
    mov r6,a               
    cjne    a,#0x02,00276$
    sjmp    00124$
00276$:
    mov dptr,#(_var + 0x0014)
    clr a
    movx    @dptr,a
    sjmp    00124$
00109$:
    mov dptr,#(_var + 0x000d)
    movx    a,@dptr
    mov r6,a                   <<<<<<<<<<<<<< dead move 
    cjne    a,#0x01,00277$
    sjmp    00124$
00277$:
    mov dptr,#(_var + 0x000d)
    clr a
    movx    @dptr,a
00124$:
    mov dpl,r7
    ret

When the above marked mov r6,a is checked whether it's a dead move or not, the doTermScan / scan4op code visits labels 00277 and 00124.

'checkLabelRef' bails out at label 00124 because at that point (for this particular dead-move check) the label ref count = 10 and the jump-to-count is only 1, as inspected by this dead-move check. There are many other parts higher up in the function that jump to label 00124, hence its high ref count.

It looks like the checkLabelRef check in removeDeadMove is superflous.
In the above case we can see that the approach doesn't work as it is anticipated in the code.