Use SFR_DATA_U32 in REG_SET()

[vDorst]

While improving #368, I saw that using SFR_DATA_U32 directly, gives better code.

Somehow the compiler seems to generate way better code which saves +300 bytes.

eg.storing zeros to SFR_DATA-registers.

Instead of 4x 3-byte instructions = 12 bytes.
CPU-cycles: 4x 3 = 12 cycles.

SFR_DATA_24 = 0x00;
SFR_DATA_16 = 0x00;
SFR_DATA_8 = 0x00;
SFR_DATA_0 = 0x00;

It uses 1 + 4x 2-bytes instructions = 9 bytes.
CPU-cycles: 1 + 4x 2 = 9 cycles.

clr a;
SFR_DATA_24 = a;
SFR_DATA_16 = a;
SFR_DATA_8 = a;
SFR_DATA_0 = a;

So it saves on bytes and runtime.

[feelfree69]

Cool, also the code looks much cleaner now.

[vDorst]

It is this patch that breaks the l2-command.
See also #368 (comment)

[vDorst]

Found it,
The compiler places the wrong register location for the highest byte.

Main-assembly

                                4619 ;   rtlplayground.c:1120: REG_SET(RTL837X_REG_NIC_TXCMD, 1);                   
  4620        001280 75 A4 00         [24] 4620     mov _SFR_DATA_24,#0x00                                                     
  4621        001283 75 A5 00         [24] 4621     mov _SFR_DATA_16,#0x00                                                     
  4622        001286 75 A6 00         [24] 4622     mov _SFR_DATA_8,#0x00                                                      
  4623        001289 75 A7 01         [24] 4623     mov _SFR_DATA_0,#0x01                                                      
  4624        00128C 90 78 50         [24] 4624     mov dptr,#0x7850                                                           
  4625                                     4625 ;   rtlplayground.c:1121: }

This PR assembly

                                  4619 ;   rtlplayground.c:1120: REG_SET(RTL837X_REG_NIC_TXCMD, 1);                   
  4620        001280 75 A7 01         [24] 4620     mov ((_SFR_DATA_U32 >> 0) & 0xFF),#0x01                                    
  4621        001283 E4               [12] 4621     clr a                                                                      
  4622        001284 F5 A6            [12] 4622     mov ((_SFR_DATA_U32 >> 8) & 0xFF),a                                        
  4623        001286 F5 A5            [12] 4623     mov ((_SFR_DATA_U32 >> 16) & 0xFF),a                                       
  4624        001288 F5 FF            [12] 4624     mov ((_SFR_DATA_U32 >> 24) & 0xFF),a                                       
  4625        00128A 90 78 50         [24] 4625     mov dptr,#0x7850    

Every SFR_DATA_U32 should be MOV RegX, a instruction.
So byte sequence should be F5 Ax where x = 7, 6, 5, 4. But the compiler makes for (_SFR_DATA_U32 >> 24) F5 FF. So FF should be A4.

[vDorst]

Made a SDCC bug report

[DrDoof]

The byte mapping is right, I checked __at(0xa4a5) against how SFR_REG_ADDR_U16 sits over ADDRH and ADDRL.

One thing though: v is not parenthesised, and >> binds tighter than |, so an argument like IGMP_MAX_GROUP | IGMP_PROTOCOL_ENABLE | IGMP_FLOOD only shifts the last term. I built both trees and compared the asm for that call:

main:  a4=00  a5=ff  a6=7c  a7=15   ->  0x00ff7c15
this:  a4=7c  a5=00  a6=7c  a7=15   ->  0x7c007c15

The max group field is lost. (uint16_t)(((uint32_t)(v)) >> 16) restores it, I tested that same call again after. The cast also keeps the shift defined when the argument is only 16 bit wide.

Good catch on the SDCC bug, and thank you for filing it upstream.

[vDorst]

Fixed the bit-shift issue.

Read for review and test.

[logicog]

In your bug report to sdcc you write:

__sfr32 __at(0xa3a5a6a7) SFR_DATA_U32;

i.e. a3, where I would expect a4 is that on purpose?

[vDorst]

See https://sourceforge.net/p/sdcc/bugs/4070/#07f5
I was trying to see if a other value also has the same effect.
But uploaded that sample.

[DrDoof]

Had a proper look at the shift fix — didn't want to just trust a green build, so I went down to the asm.

Rebuilt main (aded077) and the same tree with this on top, then looked at the two call sites that pass an OR wider than 16 bits:

REG_SET(RTL837X_IGMP_PORT_CFG + (i << 2), IGMP_MAX_GROUP | IGMP_PROTOCOL_ENABLE | IGMP_FLOOD);

; main
	mov	_SFR_DATA_24,#0x00
	mov	_SFR_DATA_16,#0xff
	mov	_SFR_DATA_8,#0x7c
	mov	_SFR_DATA_0,#0x15

; here - the symbols hold the address literal, so >>0 is 0xa5 and >>8 is 0xa4
	mov	((_SFR_DATA_U16_UPPER >> 0) & 0xFF),#0xff
	mov	((_SFR_DATA_U16_UPPER >> 8) & 0xFF),#0x00
	mov	((_SFR_DATA_U16 >> 0) & 0xFF),#0x15
	mov	((_SFR_DATA_U16 >> 8) & 0xFF),#0x7c

Both land on 0x00ff7c15, and line 149 on 0x00ff7c2a. IGMP_MAX_GROUP is back where it belongs.

Two things I couldn't match with the description, though.

I only get 153 bytes of ROM back, not ~300, and xdata and the stack don't move at all. I'm on sdcc 4.6.0 — were you on 4.5.0? Could well be just the compiler, but the number in the description could use a refresh either way.

The other one is write order: 24, 16, 8, 0 becomes 16, 24, 0, 8. I'd assume that's harmless since reg_write(r) is what kicks off the transaction, but SFR_SMI_PHY is an alias for SFR_DATA_16, so I'd rather ask than guess.

Minor: the title still says SFR_DATA_U32 while the code settled on two __sfr16. Worth a rename before this lands.