#2435 [z80] bug with peephole operandsNotRelated() ?

[alvin]

I think this is a bug in the operandsNotRelated() qualifier in the peepholer for the z80 target.

sdcc -v
3.5.5 #9392

test.c

#include <stdbool.h>

volatile bool PauseRequested;

void nmi_isr(void) __critical __interrupt
{
  PauseRequested=true;
}

sdcc -mz80 -S test.c

_nmi_isr::
    push    af
    push    bc
    push    de
    push    hl
    push    iy
    ld  hl,#_PauseRequested + 0
    ld  (hl), #0x01
    pop iy
    pop hl
    pop de
    pop bc
    pop af
    retn

This code is fine. Now add some peephole rules in a separate file.

peep.def

replace restart {
    ld  hl,#%1
    ld  (hl),#%2
} by {
    ld  a,#%2
    ld  (#%1),a
}

replace restart {
    ld  hl,#%1 + 0
} by {
    ld  hl,#%1
}

replace restart {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
} by {
    ld  %2,%3
    ld  %4,%5
} if operandsNotRelated(%2 %1), operandsNotRelated(%4 %1)

replace restart {
    push    %6
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
    pop %6
} by {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
} if operandsNotRelated(%2 %6), operandsNotRelated(%4 %6)

The rules are meant to eliminate superfluous push/pop around some assignment instructions that occurs occasionally in sdcc output. They are incorrect because they will eliminate pushaf/popaf as-is and code not naming register A can change the flags register.

I added rules at the beginning to change to store to memory to use the A register instead of HL to illustrate the problem. The rules above cause the pushaf/popaf pair to be eliminated despite the use of operandsNotRelated() to prevent that case.

sdcc -mz80 -S --peep-file peep.def test.c

_nmi_isr::
    ld  a,#0x01
    ld  (#_PauseRequested),a
    retn

pushaf/popaf around the load/store involving A has been eliminated despite the use of operandsNotRelated().

[alvin]

There doesn't seem to be any way to edit the above. It seems there has to be some blank space around the tildes to get quoted code to work and for the peephole file the tabs are important so I will repost here:

test.c

#include <stdbool.h>

volatile bool PauseRequested;

void nmi_isr(void) __critical __interrupt
{
  PauseRequested=true;
}

peep.def

replace restart {
    ld  hl,#%1
    ld  (hl),#%2
} by {
    ld  a,#%2
    ld  (#%1),a
}

replace restart {
    ld  hl,#%1 + 0
} by {
    ld  hl,#%1
}

replace restart {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
} by {
    ld  %2,%3
    ld  %4,%5
} if operandsNotRelated(%2 %1), operandsNotRelated(%4 %1)

replace restart {
    push    %6
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
    pop %6
} by {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
} if operandsNotRelated(%2 %6), operandsNotRelated(%4 %6)

[Maarten Brock]

I've modified the post for you. Next time please use the preview button (the eye) to verify. The start of a preformatted block indeed seems to require an empty line before it.

I numbered your rules 1 to 4 and I think your rules are applied in the following order:
3: remove push/pop iy
4: remove push/pop de
4: remove push/pop bc
4: remove push/pop af
1: use a instead of hl
3: remove push/pop hl

[alvin]

To rule that out I changed the rules to this:

replace restart {
    ld  hl,#%1 + 0
    ld  (hl),#%2
} by {
    ld  a,#%2
    ld  (#%1),a
    ; 1
}

replace restart {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
} by {
    ld  %2,%3
    ld  %4,%5
    ; 3
} if operandsNotRelated(%2 %1), operandsNotRelated(%4 %1)

replace restart {
    push    %6
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
    pop %6
} by {
    push    %1
    ld  %2,%3
    ld  %4,%5
    pop %1
    ; 4
} if operandsNotRelated(%2 %6), operandsNotRelated(%4 %6)

The very first rule should be applied first, changing use of HL to A. The same output results.

With "--fverbose-asm" applied, the output shows "133434" in text order but when rules apply to the same code I'm not sure it's easy to know what order they are applied in from this info.

[Philipp Klaus Krause]

Seems like handling of af is missing in operandBaseName() (unlike the other register pairs).

Philipp

[Philipp Klaus Krause]

Fixed in revision #9411.

Philipp