[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [avr-libc-dev] [bug #34423] util/crc16.h: with -Os option inline fun

From: David Brown
Subject: Re: [avr-libc-dev] [bug #34423] util/crc16.h: with -Os option inline functions are called causing registers value loss
Date: Thu, 29 Sep 2011 09:45:31 +0200
User-agent: Mozilla/5.0 (Windows NT 5.1; rv:6.0.1) Gecko/20110830 Thunderbird/6.0.1

On 28/09/2011 22:33, Karol Grzybowski wrote:

                  Summary: util/crc16.h: with -Os option inline functions are
called causing registers value loss
                  Project: AVR C Runtime Library
             Submitted by: karol_grzybowski
             Submitted on: Wed 28 Sep 2011 08:33:29 PM GMT
                 Category: Header
                 Severity: 3 - Normal
                 Priority: 5 - Normal
               Item Group: libc code
                   Status: None
         Percent Complete: 0%
              Assigned to: None
              Open/Closed: Open
          Discussion Lock: Any
                  Release: 1.7.1
            Fixed Release: None



gcc 4.5.1, AVR_8_bit_GNU_Toolchain_3.2.3_315
gcc options: -funsigned-char -funsigned-bitfields -Os -fpack-struct
-fshort-enums -Wall -c -std=gnu99  -mmcu=atmega16

static __inline__ uint8_t
_crc_ibutton_update(uint8_t __crc, uint8_t __data)
        uint8_t __i, __pattern;
        __asm__ __volatile__ (
                "  eor     %0, %4" "\n\t"
                "  ldi     %1, 8" "\n\t"
                "  ldi     %2, 0x8C" "\n\t"
                "1:        lsr     %0" "\n\t"
                "  brcc    2f" "\n\t"
                "  eor     %0, %2" "\n\t"
                "2:        dec     %1" "\n\t"
                "  brne    1b" "\n\t"
                : "=r" (__crc), "=d" (__i), "=d" (__pattern)
                : "0" (__crc), "r" (__data));
        return __crc;

should be always inlined. In my code reading DS18B20 temperature:
        // LSB of temp
        data = ds_read_byte();
        temp = (int16_t)data;
        crc = _crc_ibutton_update(0, data);
        // MSB of temp
        data = ds_read_byte();
        temp |= (int16_t)(data)<<  8;
        crc = _crc_ibutton_update(crc, data);
doesn't and that's why the MSB of temp is lost:
0e 94 44 00     call    0x88    ; 0x88<_crc_ibutton_update>

Adding fallowing line to header seems to be the simplest solution  of this
static __inline__ uint8_t _crc_ibutton_update(uint8_t __crc, uint8_t __data)

Probably the same problem may occur with others inline Assembly.

Using "__inline__" is a hint to the compiler, it is not a command. The compiler is free to implement the call as a normal function call even if it is declared "inline", and free to inline the function call even if it is /not/ "inline". When deciding on the inline heuristics, it will take the hint into account, along things like optimisation levels, and usage patterns. You can use the "-Winline" flag to give you some information about why an "inline" function is not inlined. With "-Os", the compiler is much less inclined to inline code than with "-O2". It is a known problem with gcc, and avr-gcc in particular, that the heuristics for determining the size cost of inlining is not accurate (it is especially difficult for the compiler to predict how it will affect the size of the rest of the code in the calling function). While this has improved in later versions of gcc, it is still useful to use "always_inline" when you know better than the compiler.


__attribute__((always_inline)), on the other hand, /is/ a command - the compiler should follow it unless it has an overwhelming reason not to (and that should probably give an error message). If you are interested in forcing the compiler's inlining on code, it's worth learning about the "flatten" and "noinline" attributes as well.

If you are going to write inline assembly like this, I've a couple of hints that will make it easier and improve the maintainability of the code. First, the use of "%0", "%1", etc., within the assembly is poor style, except for very small sequences - use "%[crc]" style:

(Perhaps the "avrlibc: Inline Assembler Cookbook" could be updated to include this, if anyone thinks it is worth the effort.)

Secondly, inline functions are not macros - you don't have to worry about namespace conflicts. You should use names like "crc", not "__crc".

You also don't need to declare your assembly code "volatile" - indeed, you should not do so unless you have good reason for it (such as side effects that the compiler doesn't know about). Your crc_ibutton_update function is a function that takes two parameters and returns a value that depends solely on those inputs, has no side effects and makes no read or write access to memory. Not only should the "volatile" be removed, but you could even declare the function with __attribute__((const)) to give the compiler more freedom to optimise.

Of course, if you have the code space (256 bytes), it's faster to do these CRC's using a lookup table :-)

reply via email to

[Prev in Thread] Current Thread [Next in Thread]