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

[David Brown]

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;
>   cli();
>   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;
>   cli();
>   *((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.