[avr-gcc-list] Re: Optimisation of bit set/clear in uint32_t

[David Brown]

Dale Whitfield wrote:
> Hi Alex                                          >@2009.04.22_09:51:59_+0200
>
> > Hi,
> >
> > > > > That's way too much code... Its fairly obvious where the optimisations
> > > > > should be, although I can see that some have been done already.
> > > > I can't see much possibility for improving the above code (except by  
> > > > removing the push and zeroing of r1).  You asked for "status" to be a  
> > > > volatile 32-bit int, so that's what you got.  The code below is  
> > > > semantically different, and thus compiles differently.
> > > I don't believe it is semantically different. Which is one reason I
> > > raised this. I am using the version below in existing code and it
> > > behaves correctly.
> > >
> > > Loading 4 registers and then storing back 3 that are unchanged makes no
> > > sense at all.
> > >
> > > Where volatile comes in here is that the optimisation shouldn't use any
> > > previous loads or modify register values more than once without
> > > reloading/storing etc. Here, the value is loaded once, one byte is
> > > changed and then all 4 are stored. That's wasteful.
> > but that is what you demand with volatile. No read and no write
> > can be removed. On some Hardware there will be a special reaction
> > if you write a byte, so it would be semantical different when the
> > compiler removes it.
> >
> > Maybe you can change it to use something like (not tested)
> >
> > union status_union
> > {
> >   volatile uint32_t s;
> >   volatile uint8_t  b[4];
> > } status;
> >
> >
> > then you can operate on single volatile bytes with:
>
> That's effectively the same as I changed it to by using the pointer.
>
> > //Set Bit 17 in Status
> > status.b[1] |= _BV(2);
> >
> > without writing the other bytes. An you can use status.s for
> > 32-Bit Access.
> >
> > Maybe it would be easyier to use 4 seperate status bytes, most
> > of the time it is better to think more 8-bitish if you write
> > Atmelcode.
>
> I replied to some of these points in David's post.
>
> And yes, I agree its probably more about staying close to 8-bit
> operations than anything else.
>
> Reads or writes, I believe, can be removed when updating a volatile that
> is greater than 8-bits. 

Your code might work when such writes are removed, but it will no longer 
be a correct implementation of the C volatile 32-bit write access. 
That's why you have to specifically tell the compiler to use 8-bit 
volatile writes, or 32-bit non-volatile writes - it can't make that 
decision on its own.

Remember also that the compiler doesn't know that only one byte needs to 
be changed - for all it knows, the data changed some time between when 
it read the 32 bits, and before it started to write out the new data. 
After all, it's volatile - the compiler can assume *nothing* about it.

> There are other issues that may make this code unsafe when used outside
> of an interrupt routine. This applies to any volatile data that is
> greater than 8-bits used on an 8-bit processor.
>
> I guess that its not enough to declare the 32-bit variable volatile. It
> needs to be wrapped in an atomic cli/sei section.

8-bit access must also be wrapped to be atomic, if it is a 
read-write-modify access (read-only, or write-only, are fine).  And 
wrapping in a cli/sei sequence is not enough - you need a memory clobber 
as well (if you're good at searching, you might find an archived thread 
on the subject in this group).

> If the load sequence of 4 registers is interrupted halfway and an
> interrupt changes one of the bytes that was already loaded into a
> register, then the whole volatile concept is meaningless anyway.

Correct, although that applies to an 8-bit read-write-modify sequence as 
well.

> Perhaps the warning should be, that if a volatile is more than 8-bits,
> and its used both in and outside of an interrupt, it should always be
> protected by a block of code that is guaranteed atomic.

Such a warning would be more than a little difficult to implement!

The problem boils down to a misunderstanding of "volatile".  It's 
something that a lot of people have trouble with - they often believe it 
implies atomic, and often that it means that the access in question must 
be done exactly at the given point in the program code.  However, I 
think this is the first time I've met someone who is convinced that the 
compiler should implement a 32-bit volatile write as an 8-bit write.