|
From: | David Brown |
Subject: | [avr-gcc-list] Re: Optimisation of bit set/clear in uint32_t |
Date: | Wed, 22 Apr 2009 12:56:42 +0200 |
User-agent: | Thunderbird 2.0.0.21 (Windows/20090302) |
Dale Whitfield wrote:
Hi Alex >@2009.04.22_09:51:59_+0200Hi,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.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 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 thatis 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.
[Prev in Thread] | Current Thread | [Next in Thread] |