eldarkg / emdr1986x-std-per-lib

Milandr MCU 1986x Standard Peripherals Library. Mirror:
https://code.launchpad.net/~eldar/emdr1986x-std-per-lib/+git/emdr1986x-std-per-lib
46 stars 29 forks source link

Division in system_MDR1986VE1T.c #35

Closed garou-g closed 6 years ago

garou-g commented 6 years ago

I would like to offer a small improvement for the function void SystemCoreClockUpdate (void) from module system_MDR1986VE1T.c

It uses integer division in two places where one can do without it, because both operations manipulates with divisor power of two.

Line 86: cpu_c1_freq /= 2; Line 114: cpu_c3_freq = cpu_c2_freq / ((uint32_t)2 << tmp);

Since most of the supported microcontrollers does not have division operations, I suggest replacing it with shift operations. If user app does not use integer divisions, it will reduce code size by approximately 300 bytes or more. Also, these operations are performed faster. For the first operations compiler use shift by default, but second is difficult for it.

cpu_c1_freq >>= 1; cpu_c3_freq = cpu_c2_freq >> (tmp + 1);

If you will be agree with it, I can create pull request.

eldarkg commented 6 years ago

@xgaroux I agree with you. Can you do a pull request for all CM0 and CM1 devices?

garou-g commented 6 years ago

Yes, I'l do it

Amomum commented 6 years ago

I always thought that almost any compiler can replace divisions with shifts by itself, so I went and checked the assembly.

Here's Keil5, with -O0:

division:

        86:     cpu_c1_freq /= 2; 
        87:   } 
        88:  
        89:   /* Determine CPU_C2 frequency */ 
    0x00003258 0864      LSRS     r4,r4,#1

Shift:

    0x00003256 D000      BEQ      0x0000325A
        86:     cpu_c1_freq >>= 1; 
        87:   } 
        88:  
        89:   /* Determine CPU_C2 frequency */ 
    0x00003258 0840      LSRS     r0,r0,#1

LSRS in both places.

However the second one:

Division:

   114:         cpu_c3_freq = cpu_c2_freq / ((uint32_t)2 << tmp); 
   115:       } 
   116:       else 
   117:       { 
0x000032AA 2002      MOVS     r0,#0x02
0x000032AC 40B8      LSLS     r0,r0,r7
0x000032AE 4601      MOV      r1,r0
0x000032B0 4628      MOV      r0,r5
0x000032B2 F000F8FF  BL.W     __aeabi_uidiv (0x000034B4)
0x000032B6 4606      MOV      r6,r0
0x000032B8 E000      B        0x000032BC

Shift:

       114:         cpu_c3_freq = cpu_c2_freq >> (tmp + 1); 
   115:       } 
   116:       else 
   117:       { 
0x000032AA 1C55      ADDS     r5,r2,#1
0x000032AC 460B      MOV      r3,r1
0x000032AE 40EB      LSRS     r3,r3,r5

This call to uidiv is not replaced by shift even with -O3. I find it kinda strange.

However, replacing simple /=2 with shift is unnecessary.

garou-g commented 6 years ago

I thought the same way, but when I started compilation new project with almost empty files I find that my project uses udiv and was very surprised. May be the expression 2 << tmp in brackets and casted to uint32_t too complicated for compiler :) About /=2, I agree, but it seems to me more correct that no one can think that this processor can divide

eldarkg commented 6 years ago

@Amomum we shouldn't to think what compiler and version of compiler used. Some compilers maybe without optimization use divide for /=2

Amomum commented 6 years ago

@eldarkg it's very unlikely but it's a very minor thing, so I guess it's not a big deal.