#3592 Byte Swap works ideally only in simplest cases

[Jim Granville]

Issue : More 8051s have XDATA SFRs and some look like this

#define     PWM1_ARR                (*(unsigned  int volatile xdata *)0xfed2)
#define     PWM1_ARRH               (*(unsigned char volatile xdata *)0xfed2)
#define     PWM1_ARRL               (*(unsigned char volatile xdata *)0xfed3)

The ideal here is to have big-endian SFR support, but meantime I've been testing SWAP variants

The classic byte swap of ((j << 8) | (j >> 8)) works for simple constants, #define SWAP_16b(j) ((j << 8) | (j >> 8))

SDCC 4.2.0 Swap(CONST) creates correct code in all cases I tested
data = ok, idata=ok, xdata=ok, pdata= ok

but users may want to load variables into SFRs, not just simple init constants

data = Swap(data) stumbles a little, with the unexpected register hop

; direct load form
    mov _wDPTR,_Quad16
    mov (_wDPTR + 1),(_Quad16 + 1)
; swap load does a register hop
    mov r6,(_Quad16 + 1)
    mov r7,_Quad16
    mov _wDPTR,r6
    mov (_wDPTR + 1),r7

idata = Swap(data) gets worse

; direct idata load as expected 
    mov r0,#_iData
    mov @r0,_wDPTR
    inc r0
    mov @r0,(_wDPTR + 1)
; swap idata load misses the simple data swap, and appears to be doing 16b pre-loads ?!  Not sure what tips it into 16b loads ?
    mov r5,_wDPTR
    mov r4,#0x00
    mov r2,(_wDPTR + 1)
    mov r3,#0x00
    mov a,r2
    orl ar4,a
    mov a,r3
    orl ar5,a
    mov r0,#_iData
    mov @r0,ar4
    inc r0
    mov @r0,ar5

pdata is strangely better than idata, as it at least loads single bytes, tho still does a needless register hop.

    mov r6,(_Quad16 + 1)
    mov r7,_Quad16
    mov r0,#_pData
    mov a,r6
    movx    @r0,a
    mov a,r7
    inc r0
    movx    @r0,a

even stranger, is the first instance of a xdata reference does find the correct swap
It looks like maybe the register content info is also tangling things here ?

; SfrXdata = SWAP_16b(Quad16b);
    mov dptr,#_SfrXdata
    mov a,(_Quad16b + 1)
    movx    @dptr,a
    mov a,_Quad16b
    inc dptr
    movx    @dptr,a

tho I can never get idata to look like that ?

SfrXdata = SWAP_16b(iData); is a bit more of a challenge,
The ideal here would be to load iData+1, and then DEC R0, with a +/-1 R0 content check, so the line-count is identical to the non-swap case.

; SfrXdata = iData;
    mov r0,#_iData
    mov dptr,#_SfrXdata
    mov a,@r0
    movx    @dptr,a
    inc r0
    mov a,@r0
    inc dptr
    movx    @dptr,a
; SfrXdata = SWAP_16b(iData);
    mov r0,#_iData
    mov ar7,@r0
    mov r6,#0x00
    mov r0,#(_iData + 1) ; << hmm.. this also missed INC R0 ?
    mov ar4,@r0
    mov r5,#0x00
    mov dptr,#_SfrXdata
    mov a,r4
    orl a,r6
    movx    @dptr,a
    mov a,r5
    orl a,r7
    inc dptr
    movx    @dptr,a

The best idata Swap form I can get

; iData = SWAP_16b(Quad16b);
    mov r6,(_Quad16b + 1)
    mov r7,_Quad16b
    mov r0,#_iData
    mov @r0,ar6
    inc r0
    mov @r0,ar7

but It can generate this

    mov r0,#_iData
    mov @r0,_Quad16
    inc r0
    mov @r0,(_Quad16 + 1)
; so it does manage to push data thru to @Ri,data opcodes, 
; which means this should be possible too ?
    mov r0,#_iData
    mov @r0,_Quad16b + 1
    inc r0
    mov @r0,_Quad16b

[Jim Granville]

Further to the above "even stranger, is the first instance of a xdata reference does find the correct swap"

That's not quite the trigger issue, It seems how the data variable is declared matters.
The good/bad instances were both data vars but how they were declared was slightly different.

_DataC::
    .ds 2   ; a variable allocate.
_wDPTR  =   0x0078  ; an SFR like absolute declaration 

;   SfrXdata = SWAP_16b(DataC); < this works nicely
    mov dptr,#_SfrXdata
    mov a,(_DataC + 1)
    movx    @dptr,a
    mov a,_DataC
    inc dptr
    movx    @dptr,a

; but the same data space var, just declared differently, 'goes to town'
; SfrXdata = SWAP_16b(wDPTR);
    mov r7,_wDPTR
    mov r6,#0x00
    mov r4,(_wDPTR + 1)
    mov r5,#0x00
    mov a,r4
    orl ar6,a
    mov a,r5
    orl ar7,a
    mov dptr,#_SfrXdata
    mov a,r6
    movx    @dptr,a
    mov a,r7
    inc dptr
    movx    @dptr,a

Because C uses a verbose construct to create a swap, I wonder if detecting that should flag the var as 'swapped', for later treatment, which means the way the variable was declared matters less, and it could also skip over any later attempts to expand the verbose construct fully literally in code ?
Right now, SDCC only sometimes manages to get this 'clean swap' right.

[Philipp Klaus Krause]

In trunk (and thus up to SDCC 4.3.0), SDCC tries to detect swaps, and create a SWAP iCode for them (if the backend supports it; mcs51 does for 8-bit and 16-bit values). In the next branch this has been generalized (we now have a ROT iCode instead, and a byte swap for a 16 bit value is now considered a rotation by 8 bits), but for the mcs51 backend, we still optimize the same swaps as before: swaps of 8-bit and 16-bit values. See [feature-requests:#877].

[Philipp Klaus Krause]

I've tried the small sample below, to see for which cases SDCC (from next branch) recognizes the swap:

#define SWAP(x) (((x) << 8) | ((x) >> 8))

__idata unsigned int int_i;
__data unsigned int int_d;
__pdata unsigned int int_p;
__xdata unsigned int int_x;
__code unsigned int int_c = 0x5a5a;
__sfr16 int_f;

void f_i_d(void)
{
    int_i = SWAP(int_d);
}

void f_i_x(void)
{
    int_i = SWAP(int_x);
}

void f_i_f(void)
{
    int_i = SWAP(int_f);
}

void f_d_i(void)
{
    int_d = SWAP(int_i);
}

void f_x_i(void)
{
    int_x = SWAP(int_i);
}

void f_f_i(void)
{
    int_f = SWAP(int_i);
}

And I see the optimization happen for all cases here except for f_i_f (which cannot be optimized). SDCC recognizes the C code as a swap, and treats it as such in the iCode. Now, register allocation and code generation might not generate optimal code, even for a recognized swap, though.

[Maarten Brock]

What does it do for int_c? In the most optimal case it would make it an assignment, even when the initializer is not symmetrical.
And what does it do for extern __code unsigned int eint_c; ? Here no constant propagation can take place.

[Philipp Klaus Krause]

For int_c:

#define SWAP(x) (((x) << 8) | ((x) >> 8))

__idata unsigned int int_i;
__code unsigned int int_c = 0x5a5a;

void f_i_c(void)
{
    int_i = SWAP(int_c);
}

In the next branch, it generates a read from memory, followed by a swap:

;   genAssign
    mov dptr,#_int_c
    clr a
    movc    a,@a+dptr
    mov r6,a
    mov a,#0x01
    movc    a,@a+dptr
    mov r7,a
;   genSwap
    mov r0,#_int_i
    mov @r0,ar7
    inc r0
    mov @r0,ar6

In the genconstprop branch, much shorter code is generated, though it is still a ROT iCode:

;   genSwap
    mov r0,#_int_i
    mov @r0,#0x5a
    inc r0
    mov @r0,#0x5a

Philipp

[Maarten Brock]

The first is exactly what one wants for eint_c.
And the second looks very good with constant propagation.

[Philipp Klaus Krause]

The work in those branches has been merged to trunk a while ago. Byte swaps are being optimized much better now.