[Top][All Lists]

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

Re: [avr-gcc-list] Avr-gcc Produces Incorrect Code with -Os

From: David Brown
Subject: Re: [avr-gcc-list] Avr-gcc Produces Incorrect Code with -Os
Date: Thu, 22 May 2008 13:09:31 +0200
User-agent: Thunderbird (Windows/20080421)

Paulo Marques wrote:
Weddington, Eric wrote:
If ATOMIC_BLOCK() is *the* way to do it, perhaps the docs
could be improved a little by emphasizing the use of
ATOMIC_BLOCK() as a necessity rather than as a convenience.

I'll go a step further: Is there any reason to NOT include the memory
barrier in the interrupt macros? Would that even work?
Like this,

# define sei()  __asm__ __volatile__ ("sei" ::: "memory")
# define cli()  __asm__ __volatile__ ("cli" ::: "memory")

This has been discussed before (on the avr-libc list, December 2005, just search for '"cli" and "sei" should clobber memory'), and this exact solution was actually proposed by me.

The downside is that a memory clobber like that drops all optimizations of global variables that the compiler might be temporarily storing in registers. So, it actually hurts performance for code unrelated to the critical section.

Another thing we could do to help the common cases of reading / writing multi-byte vars (or even single byte non-volatile vars) was to write accessors like (totaly untested):

static inline uint16_t atomic_read16(uint16_t *var)
  uint16_t ret;
  uint8_t old_sreg = SREG;
  ret = *((volatile uint16_t *)var);
  SREG = old_sreg;

<pedantic> You forgot the "return ret". </pedantic>

static inline void atomic_write16(uint16_t *var, uint16_t value)
  uint8_t old_sreg = SREG;
  *((volatile uint16_t *)var) = value;
  SREG = old_sreg;

....(repeat for other sizes)....

I'd have to check this code better, because I always end up placing the volatile in the wrong place and casting the pointer to volatile instead of the variable ;)

Anyway, the advantage of something like this over ATOMIC_BLOCK() is that it doesn't need any memory clobbers, since all operations are already volatile. So, this should produce slightly better code.

One idea worth trying for simplifying this might be to use macros rather than static inlines (although normally I prefer static inlines to macros) along with gcc's "typeof" operator:

#define volatileAccess(v) *((volatile typeof((v)) *) &(v))
#define atomic_read(v) ({ \
        uint8_t old_sreg = SREG; \
        cli(); \
        typeof(v) ret = volatileAccess(v); \
        SREG = old_sreg; \
        ret; \

#define atomic_write(v, x) ({ \
        uint8_t old_sreg = SREG; \
        cli(); \
        volatileAccess(v) = x; \
        SREG = old_sreg; \

An alternative way to handle this sort of thing would be to implement a "critical" function attribute like in the msp430 port of gcc - a function with this attribute has interrupts disabled at the start, and restored at the end. I find it very convenient when programming for the msp430.


reply via email to

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