#3263 Incorrect codegen on __sfr 4.1.0 [GBZ80]

[Daid]

The GBZ80 codegen can generate incorrect code when __sfr registers are used.

Example:

static volatile __sfr __at(0x43) reg;

int f() {
    if (reg == 0) { return 1; }
    return 0;
}

https://godbolt.org/z/6xna4oovc

Will generate:

_reg    =       0x0043

ld      hl, #_reg
ld      a, (hl)
or      a, a

But, for GBZ80, the __sfr registers are at 0xFFxx not at 0x00xx. Trying to use 0xFF43 will result in a warning but "correct" (suboptimal) codegen.

I've also seen hl being optimized away into:

ld a, (#_reg)
or a, a

Tested on release 4.1.0

[Daid]

Actually, looking better at it. I think the warning on __sfr addresses for GBZ80 is wrong, which causes GBDK to use the range 0x00-0xFF as __at values, which then get translated to ldh instructions, which accept a 0x00-0xFF as an 0xFFxx address.

[Tony Pavlov]

that is not a bug, you must use 0xFF43 as address: __at(0xFF43)

[Daid]

So in that case, the bug is a the warning 182. As using the 0xFF43 gives a warning.

[Tony Pavlov]

yes, in 4.1.6 #12533 __sfr seems to be very broken. several problems were introduced recently.

1. __sfr without __at() emits SFR's out of any areas:

volatile __sfr reg;

result:

;--------------------------------------------------------
; special function registers
;--------------------------------------------------------
_reg::
    .ds 1
;--------------------------------------------------------
; ram data
;--------------------------------------------------------
    .area _DATA
;--------------------------------------------------------
; ram data
;--------------------------------------------------------
    .area _INITIALIZED
_font_bitmaps::
    .ds 448

[Tony Pavlov]

2.

defined SFR's do not use advantage of LDH instruction and also address is either wrong, or generates a warning, as said by Daid.

volatile __sfr __at (0xFFFC) reg;

int f() {
    if (reg == 0) { return 1; }
    return 0;
}

which emits this code:

;   ---------------------------------
; Function f
; ---------------------------------
_f::
;test0.c:42: if (reg == 0) { return 1; }
    ld  a, (#_reg)
    or  a, a
    jr  NZ, 00102$
    ld  de, #0x0001
    ret
00102$:
;test0.c:43: return 0;
    ld  de, #0x0000
;test0.c:44: }
    ret

[Philipp Klaus Krause]

How about just removing __sfr support for gbz80?

IMO, __sfr only adds unnecessary complexity to gbz80.

The following work fine:

volatile unsigned char __at(0xff01) c1;
volatile unsigned char __at(0xf801) c2;

void f(void)
{
    c1 = c2;
}

#define C1 (*(volatile unsigned char *)0xff01)
#define C2 (*(volatile unsigned char *)0xf801)

void g(void)
{
    C1 = C2;
}

[Tony Pavlov]

i don't think that is a good idea, because LDH instructions are faster than LD. GB has A LOT OF hardware registers and they are frequently used, so there is much sense in them. that will reduce performance or C-written game boy games.

it is actually i/o. ldh are equivalents to in/out z80 instructions. also there is ldh (c), a which is in (c), a and for out as well. only it is mapped onto CPU address space. maybe that should be handled somehow else, more like i/o?

volatile __sfr __at(01) __mapped(0xff01) c1;

[Tony Pavlov]

or maybe:
volatile unsigned char __at(0xff01) __io(0x01) c1;

[Philipp Klaus Krause]

But it is just memory-mapped I/O, so from the compiler perspective, it ist just volatile-qualified accesses to memory. And the SM83 happens to have a special instruction for efficient access to part of that memory.

In the above example code, current SDCC does use ldh instructions for the access to C1, but not for c1.

P.S.: I made a small change in the use of ldh today: Now in some cases, where ldh was previously used by the peephole optimizer, it is emitted by codegen instead. Still, both before and from today, SDCC uses ldh for the access to C1.

[Tony Pavlov]

if __sfr will simply do nothing, but sdcc will emit ldh for volatile variables in the range 0xff00...0xffff, then it is probably ok. but i don't see how that will simplify gbz80 port.

also, now __sfr is a hint that allow to emit ldh when the declaration is extern without address. that is important feature, because we have 3 game-boy targets: game boy itself, mega duck and analogue pocket. they have different hardware registers layout. there is compiled sfr.o in the library where addresses come from, and the definition of the hardware registers in headers is the same. that will not be possible anymore.

[Philipp Klaus Krause]

I see the latter point. without __sfr, indeed accesses from a different module, where the address is not known to the compiler would result in ldh not being used (and plain ld is 16 cycles vs. 12 for ldh, so a bit slower).
Anyway, I think we'd want to improve use of ldh in codegen a bit further before making a decision on removing __sfr. And improving use of ldh is IMO worth it even if __sfr stays.

[Philipp Klaus Krause]

I think requiring the user to specify the __sfr address with full 16 bits is the best solution here.

In [r12833] (SDCC 4.2.13), I changed the documentation and warning accordingly.