Bug 20288 - AVR assignment of a value through a 16 bit pointer generates out of order code
Summary: AVR assignment of a value through a 16 bit pointer generates out of order code
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 3.4.3
: P2 enhancement
Target Milestone: 3.4.4
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2005-03-02 18:16 UTC by Bob Paddock
Modified: 2005-07-23 22:49 UTC (History)
4 users (show)

See Also:
Host:
Target: AVR
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Bob Paddock 2005-03-02 18:16:44 UTC
On the AVR when one makes an assignment of a value through a 16 bit pointer,
avr-gcc is assigning the low byte, then high byte.  However, many uses of a
pointer like this are for situations such as pointing at 16-bit registers.  In
such cases, the assignment should be made high byte first, then low byte, as
per Atmel data-sheets due to the sequence of the latching mechanism.

[The follow emphasis is Atmel's from the data-sheet]:

"On the AVR to do a 16-bit write, *THE HIGH BYTE MUST BE WRITTEN BEFORE THE LOW
BYTE*. For a 16-bit read, THE LOW BYTE MUST BE READ BEFORE THE HIGH BYTE."

This applies to all internal 16 bit I/O registers.
 
The following example generates incorrect code if the pointer points to 16-bit
I/O register mapped into SRAM (outside of I/O space):

main.i:
# 1 "main.c"
# 1 "C:\\BKU\\Projects\\SC200\\SourceCode\\GCCTests//"
# 1 "<built-in>"
# 1 "<command line>"
# 1 "main.c"
# 28 "main.c"
volatile unsigned short *ptr_u16;

int main( void )
{
    ptr_u16 = (unsigned short *) (*(volatile unsigned short *)0x0086);

    *ptr_u16 = 0x1234;
}

Bad output:

  34               	.LM3:
  35 0018 84E3      		ldi r24,lo8(4660)
  36 001a 92E1      		ldi r25,hi8(4660)
  37 001c 8083      		st Z,r24 <----- low byte assigned first [INCORRECT]
  38 001e 9183      		std Z+1,r25

Command Line:
avr-gcc -c -mmcu=atmega162 -I. -gdwarf-2 -DF_CPU=8000000UL  -Os -funsigned-char
-funsigned-bitfields -fpack-struct -fshort-enums -Wall -Wstrict-prototypes
-Wa,-adhlns=main.lst  -std=gnu99 -v --save-temps -MD -MP -MF .dep/main.o.d
main.c -o main.o 

Build specs:

Reading specs from C:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/specs
Configured with: ../gcc-3.4.3/configure --prefix=m:/WinAVR --build=mingw32
--host=mingw32 --target=avr --enable-languages=c,c++ --with-dwarf2
Thread model: single
gcc version 3.4.3
 C:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/../../../../avr/bin/ld.exe -m avr5 -Tdata
0x800100 -o main.elf
C:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/../../../../avr/lib/avr5/crtm162.o
-LC:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/avr5
-LC:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3 -LC:/WINAVR~2/BIN/../lib/gcc
-LC:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/../../../../avr/lib/avr5
-LC:/WINAVR~2/BIN/../lib/gcc/avr/3.4.3/../../../../avr/lib main.o -Map=main.map
--cref -lm -lgcc -lc -lgcc
Comment 1 Eric Weddington 2005-03-02 18:19:34 UTC
Please note that this was discussed heavily on avr-gcc-list recently, and it is
a desired feature to have. This should be marked as an enhancement (since I
don't have sufficient permissions).
Comment 2 Eric Weddington 2005-03-02 18:20:21 UTC
And the Component should be marked "target".
Comment 3 Giovanni Bajo 2005-03-02 21:24:13 UTC
Can you provide a link to the discussion in the avr-gcc list?
Comment 4 Eric Weddington 2005-03-02 22:01:12 UTC
Link to discussion on avr-gcc-list:
<http://lists.nongnu.org/archive/html/avr-gcc-list/2005-02/msg00220.html>
Comment 5 Paul Schlie 2005-03-02 23:07:24 UTC
(In reply to comment #0)
> [The follow emphasis is Atmel's from the data-sheet]:
> 
> "On the AVR to do a 16-bit write, *THE HIGH BYTE MUST BE WRITTEN BEFORE THE LOW
> BYTE*. For a 16-bit read, THE LOW BYTE MUST BE READ BEFORE THE HIGH BYTE."
> 
> This applies to all internal 16 bit I/O registers.

Personally see no problem.  The quote above has specifically to do with how to write a
logical 16-bit timer/counter value into a pair of otherwise distinct 8-bit registers, which
controls a specific peripheral function which presumes a specific write sequence to maintain
the logical integrety of that specific timer/counter value; it has nothing to with 16-bit pointers,
the sequencing of multi-byte logical data type transactions, or the compiler in general.

Comment 6 Bob Paddock 2005-03-03 13:13:14 UTC
(In reply to comment #5)

>Personally see no problem.  The quote above has specifically to do with how to
>write a logical 16-bit timer/counter value into a pair of otherwise distinct
>8-bit registers, which controls a specific peripheral function which presumes a
>specific write sequence to maintainthe logical integrety of that specific
timer/counter value; it has nothing to with 16-bit pointers,
>the sequencing of multi-byte logical data type transactions, or the compiler in
>general.

It is specific to the pointer case because the correct order of operations is
generated in all other cases.  Due to the limited Flash space of the AVR it is
common to have structures of pointers pointing to the hardware registers, so
that the same code will work with both Output Compare One, and Output Compare
Three for example.

The Input Capture and Output Compare registers are truly 16-bits in length,
they are not distinct 8-bit registers.  They are accessed 8-bits at a time.

To access the 16-bit registers 8-bits at a time without use of the hardware
latching mechanism would require that interrupts be disabled.  In many embedded
applications this is unacceptable, hence the hardware is provided to deal with
this problem.

By not disabling interrupts and not using the hardware latching mechanism the
embedded device, be it a Cruse Control or Phase Control System, is going to
have outputs that are off by a factor of 256 at apparently "random" times.

If the complier can not be made to generate correct code for this case then the
compile should fail.  Consider that if the the order of 16-bit operations I/O
was wrong in all cases then the GCC would not be usable on the AVR.

"Asynchronous Hardware/Firmware" by Jack Ganssle at
http://www.ganssle.com/articles/asynchf.htm goes into the gory details of why
this type of failure in a embedded device is down right dangerous.
Comment 7 Paul Schlie 2005-03-03 19:47:07 UTC
(In reply to comment #6)
Nope, these are peripheral i/o registers, and like any pheripheral interface may have
access sequence requirements which need to be satsifyed within it's driver. These
perpheral register's access sequence requirements have nothing to do with the avr's
ISA or impled compiler requirments, just simply the conventions which need to be
followed when attempting to access manipulate dynamically active 16-bit counter
values through a pair of 8-bit i/o registers (which happen to be mapped in data
address space, which isn't an uncommon, but implies nothing otherwise, other than
you shouldn't assume that the compiler need sequence indirect access to arbitrary
multi-word/byte transactions data to satisfy a peripheral's interface sequence
that's the job of the author of the it's interface driver to guarantee; and by the way,
you still need to disable interrupts if an interrupt routine may access the same
registers, as all the sequence does is read/write upper 8-bit latched value when
ever the lower 8-bit value is accessed, so therefore if you write the high value,
get an interupt which writes both, then write the low value, you get the interrupt
routines high value, combined with your new low value written, which isn't likely
what you want.


Comment 8 Eric Weddington 2005-03-03 19:49:59 UTC
Subject: Re:  AVR assignment of a value through a 16 bit
 pointer generates out of order code

schlie at comcast dot net wrote:

>------- Additional Comments From schlie at comcast dot net  2005-03-03 19:47 -------
>(In reply to comment #6)
>Nope, these are peripheral i/o registers, and like any pheripheral interface may have
>access sequence requirements which need to be satsifyed within it's driver. These
>perpheral register's access sequence requirements have nothing to do with the avr's
>ISA or impled compiler requirments, just simply the conventions which need to be
>followed 
>  
>

That's why this is an enhancement. This is a request to change those 
access conventions.

Comment 9 Björn Haase 2005-03-03 22:21:52 UTC
Hi, 
 
in order to completely resolve this issue, IIUC, one would have to sacrifice 
the post-increment addressing modes. In case of the X-Register, forcing the 
high-byte first rule allways would result in much less efficient code. For this 
reason, I think that enforcing the high-byte first rule always is not a good 
solution. 
 
There has been the suggestion to 1.) distinguish between pointer variables that 
are marked "volatile" and pointer variables that are not declared "volatile" 
and 2.) disable all post increment operations for such variables.  
In my opinion, this would not really be a clean solution, since IIUC,  
"volatile uint16_t pointer" is meant to be used for a pointer that, e.g. could 
be altered by an IRQ function, and the key-word "volatile" not meant to be used 
for classifying the variable the pointer is refering to. In fact, in the 
specific case no one would require the pointer no to be hold in a register 
variable. 
 
I also disagree that accessing IO-Memory by use of pointers is a very common 
case. Possibly it is a useful solution if one does not know at compile time 
which register will actually be in use. But I'd like to suggest, that this is 
sufficiently infrequently used to justify to require that sw developers use a 
special solution for this case, like the following: 
 
In order to enforce the byte-ordering, one could define an inline-asm 
instruction like 
 
#define WRITE_WORD_TO_MEMORY_MSB_FIRST(pointer_to_iom,value_to_be_stored) \ 
asm volatile ("/* Storing %A1:%B1 to the memory address %A0:%B0 is pointing to, 
high byte first */\n\t" \ 
              "st z+1,%B1 \n\t" \ 
              "st z,%A1 \n\t" \ 
              : \ 
              : "z" (pointer_to_iom), \ 
                "r" (value_to_be_stored) ); \ 
 
Such an inline directive could be defined in one of the header files of 
avr-libc. Anybody that *really* could not avoid to access IOM-Space by pointers 
then would have the opportunity to make use of this makro. 
 
Summing up, my suggestion is not to change the compiler in order to avoid 
code-size and performace regressions. Instead, I consider it to be best to 
clearly document the issue and to provide a suitable macro definition in one of 
the avr-libc headers. 
 
Yours, 
 
Björn 
Comment 10 Jörg Wunsch 2005-03-03 22:49:18 UTC
(In reply to comment #9)

> There has been the suggestion to 1.) distinguish between pointer variables that 
> are marked "volatile" and pointer variables that are not declared "volatile" 
> and 2.) disable all post increment operations for such variables.  
> In my opinion, this would not really be a clean solution, since IIUC,  
> "volatile uint16_t pointer" is meant to be used for a pointer that, e.g. could 
> be altered by an IRQ function, and the key-word "volatile" not meant to be used 
> for classifying the variable the pointer is refering to. In fact, in the 
> specific case no one would require the pointer no to be hold in a register 
> variable. 

(It should be noted that this suggestion originally came from Marek
Michalkiewicz, one of the original developers who ported GCC to the AVR.)

While you're technically right on this, the point was that for a pointer
variable marked "volatile", sacrificing the post-increment addressing mode
would not hurt at all, as by qualifying it "volatile" one already essentially
gave up any and all expectations about getting the most effective code
generated for it.  OTOH, access to all IO resources is always done through
volatile-qualified pointers, so this solution still appears to be the most
logical one.

Either way, I agree the behaviour should be documented.

This is not to say the suggested macro doesn't have a valid point, too.  But
the wording of Björn's reply sounded a bit too dramatic in my opinion, in
particular to those GCC developers who aren't familiar with the AVR.  To me,
Marek's suggestion (which is even supported by his patch that is known to
work for many people) still sounds like the best compromise.

Btw., it's not only accessing timer registers.  Atmel requires the high-byte
first access for a few other 16-bit IO registers as well (IIRC e.g the ADC
registers).
Comment 11 Paul Schlie 2005-03-04 14:13:48 UTC
(In reply to comment #10)
Upon further thought, and agreeing that the explicit use of an asm macro is likely
the most appropriate near term solution; it would appear the most ideal longer
term solution would be to figure out how to comprehensively enable both explictly
and implicitly declared objects optionally tagged with target specific attributes to
influence the selection of the access method (generated code) used to access them.

Thereby allocated objects may be identified as potentially being allocated to either
ROM(progmem), EEPROM, RAM(by defalut), or even requiring a particular access
sequence (which in theory may even include the automatic wraping of interrupt
disable/enable around the access automatically).

(but it appears that a few things wihtin GCC may need to be refined to broadly
 enable the use of attributes to accomplish this, which I'm attempting to document).
Comment 12 Eric Weddington 2005-03-04 14:19:17 UTC
(In reply to comment #11)

Paul,

Everybody who works on the AVR toolchain knows that it would be desirable to
have attributes to allow objects to be put in and accessed in different address
spaces. This has nothing to do with this bug report. Who are you trying to
convince? Please refrain from muddying the waters with comments that have
nothing to do with the issue at hand. You're just wasting bandwith.
Comment 13 Paul Schlie 2005-03-04 15:26:41 UTC
(In reply to comment #12)
> Everybody who works on the AVR toolchain knows that it would be desirable to
> have attributes to allow objects to be put in and accessed in different address
> spaces. This has nothing to do with this bug report. Who are you trying to
> convince? Please refrain from muddying the waters with comments that have
> nothing to do with the issue at hand. You're just wasting bandwith.

(Although I know I should simply ignore ignorant comments, I feel compelled
to respond.  I pre-apologize, and will not continue the response further than):

Eric, I'm glad it's obvious to you; maybe you'd consider attempting to technically
contribute to the refinement of AVR's GCC port; as opposed to feeling overly
empowered playing secretary for the WINAVR product in which you've obviously
never technically contributed to the refinement of the GCC port which it's based
upon, but instead apparently feel compelled to exert influence over what appears
to obviously be beyond your intimate familiarity, and likely skill set to productively
technically contribute towards yourself.

(Admittedly I am no GCC expert by any stretch of imagination, but observe that
 in my relatively short tenure attempting to familiarize myself with both GCC's
 architecture and the AVR port sufficiently to productively contribute to their
 refinement, I've likely already contributed more (which isn't saying much) than
 you would appear to ever likely accomplish; so please try to tone down your
 misplaced opinions on subjects and people  you're clearly predominantly
 ignorant of.)

Comment 14 Björn Haase 2005-03-04 19:38:22 UTC
In reply to comment #10. 
 
I agree with you Jörg, it is not a dramatic loss if you have a bit less 
efficient use of volatile pointers :-) and IMHO anybody in the avr community 
could live with it. I think it's possibly rather a question of style. 
 
I'd like to suggest that the explicit use of a macro while sticking to the 
standard convention for use of the volatile keyword might make it more 
transparent *why* and *that* an explicit treatment is required for 16 bit 
registers. Imagine the case of a person who is reading code written by someone 
else. Just seeing the "volatile" keyword would not make the possible danger 
clear. 
 
Maybe the "volatile" solution makes it easier for some programers and I agree 
with anybody that using macros tends to turn the code uglier. Possibly, 
however, there is the danger that one makes "things easier than they are in 
fact": 
 
With either approach, the user will be required to be avare of the possible 
problem and would have to handle the code accordingly. My personal opinion is: 
If there is a pitfall with 16 bit registers, write code that names it by it's 
real name. 
 
Anyway I could also live with a solution that patches the compiler. 
 
Yours, 
 
Björn 
Comment 15 CVS Commits 2005-03-06 21:50:41 UTC
Subject: Bug 20288

CVSROOT:	/cvs/gcc
Module name:	gcc
Changes by:	marekm@gcc.gnu.org	2005-03-06 21:50:37

Modified files:
	gcc            : ChangeLog 
	gcc/config/avr : avr.c avr.md 

Log message:
	PR target/20288
	* config/avr/avr.c (print_operand): Add 'p' and 'r'.
	(out_movhi_r_mr): Read low byte of volatile MEM first.
	(out_movhi_mr_r): Write high byte of volatile MEM first.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&r1=2.7691&r2=2.7692
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.c.diff?cvsroot=gcc&r1=1.129&r2=1.130
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.md.diff?cvsroot=gcc&r1=1.49&r2=1.50

Comment 16 Andrew Pinski 2005-03-06 23:55:57 UTC
Fixed so closing.
Comment 17 CVS Commits 2005-03-13 21:47:22 UTC
Subject: Bug 20288

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-4_0-branch
Changes by:	marekm@gcc.gnu.org	2005-03-13 21:47:09

Modified files:
	gcc            : ChangeLog 
	gcc/config/avr : avr.c avr.md 

Log message:
	PR target/20288
	* config/avr/avr.c (print_operand): Add 'p' and 'r'.
	(out_movhi_r_mr): Read low byte of volatile MEM first.
	(out_movhi_mr_r): Write high byte of volatile MEM first.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=2.7592.2.44&r2=2.7592.2.45
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.c.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.129&r2=1.129.6.1
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.md.diff?cvsroot=gcc&only_with_tag=gcc-4_0-branch&r1=1.49&r2=1.49.8.1

Comment 18 CVS Commits 2005-03-13 21:49:53 UTC
Subject: Bug 20288

CVSROOT:	/cvs/gcc
Module name:	gcc
Branch: 	gcc-3_4-branch
Changes by:	marekm@gcc.gnu.org	2005-03-13 21:49:45

Modified files:
	gcc            : ChangeLog 
	gcc/config/avr : avr.c avr.md 

Log message:
	PR target/20288
	* config/avr/avr.c (print_operand): Add 'p' and 'r'.
	(out_movhi_r_mr): Read low byte of volatile MEM first.
	(out_movhi_mr_r): Write high byte of volatile MEM first.

Patches:
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/ChangeLog.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=2.2326.2.815&r2=2.2326.2.816
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.c.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.108.4.4&r2=1.108.4.5
http://gcc.gnu.org/cgi-bin/cvsweb.cgi/gcc/gcc/config/avr/avr.md.diff?cvsroot=gcc&only_with_tag=gcc-3_4-branch&r1=1.42.4.2&r2=1.42.4.3