AVR-Chat

OK Thanks for the clarifications about volatile... what I was distracted by was that the variable in question was not being *changed* by an interrupt routine, it was only changed in mainline code. However it was being *read* in an interrupt routine. As the compiler couldn't see the variable being used in the current scope it optimised the code out.

However - the strange thing is why the compiler has only optimised part of it, it is still loading the value into a register, but then has removed the store instruction. If it had optimised the whole thing away it would seem a slightly more valid thing to do.

----Original Message----
From: AVR-Chat@yahoogroups.com
[mailto:AVR-Chat@yahoogroups.com] On Behalf Of Tim Mitchell
Sent: 13 April 2012 11:35 To: AVR-Chat@yahoogroups.com
Subject: [AVR-Chat] gcc compiler bad behaviour

> The GCC compiler optimiser seems to be doing a weird
> thing to me... I'd like to understand it. it seems to
> require a volatile qualifier when I am not expecting it.  
> 
> I have the following variables defined
> unsigned char LedLevel0=0;
> unsigned char LedLevel1=0;
> volatile unsigned char AdcVal[4];
> 
> AdcVal holds readings from the ADC and is loaded in an
> interrupt routine 
> 
> main loop:
> 
> 	//the main loop
> 	while(1)
> 	{
> 		LedLevel0=AdcVal[0];
> 		LedLevel1=AdcVal[1];
> 	}
> 
> compiler output:
> 
> 	//the main loop
> 	while(1)
> 	{
> 		LedLevel0=AdcVal[0];
>  3ea:	80 91 86 01 	lds	r24, 0x0186
> 		LedLevel1=AdcVal[1];
>  3ee:	80 91 87 01 	lds	r24, 0x0187
>  3f2:	fb cf       	rjmp	.-10     	; 0x3ea <main+0x8>
> 
> i.e. it's loading the AdcVal into r24, but never stores
> it into the other variable. 
> 
> if I change the main loop to add a call to a delay
> routine then it works correctly. Also if I declare
> LedLevel0/1 as volatile.  
> But in my mind it shouldn't need to be declared volatile,
> nothing else is changing it. 
> 
> 	//the main loop
> 	while(1)
> 	{
> 		LedLevel0=AdcVal[0];
> 		LedLevel1=AdcVal[1];
> 		TimeWait(1);
> 	}
> 
> compiler output:
> 	//the main loop
> 	while(1)
> 	{
> 		LedLevel0=AdcVal[0];
>  3ea:	80 91 86 01 	lds	r24, 0x0186
>  3ee:	80 93 06 01 	sts	0x0106, r24
> 		LedLevel1=AdcVal[1];
>  3f2:	80 91 87 01 	lds	r24, 0x0187
>  3f6:	80 93 07 01 	sts	0x0107, r24
> 		TimeWait(1);
>  3fa:	81 e0       	ldi	r24, 0x01	; 1
>  3fc:	90 e0       	ldi	r25, 0x00	; 0
>  3fe:	f4 cf       	rjmp	.-24     	; 0x3e8 <main+0x6>



-- 
Tim Mitchell

Tim Mitchell wrote:

> OK Thanks for the clarifications about volatile ... the
> variable in question was ... being ... changed in mainline
> code ... [and] being *read* in an interrupt routine. As
> the compiler couldn't see the variable being used in the
> current scope it optimised the code out.

Great.  Your explanation is exactly correct.

> ... the compiler has only optimised part of it, it is
> still loading the value into a register, but then has
> removed the store instruction. If it had optimised the
> whole thing away it would seem a slightly more valid
> thing to do.

By declaring AdcVal[] as volatile, you are instructing the compiler to evaluate accesses to elements of this array "strictly according to the rules of the abstract machine".  In terms we can understand, this means not performing any optimizations that would remove or re-order accesses to the object.  So, although the compiler is not interested in the value of AdcVal[0] and AdcVal[1], it is obliged to read the objects.  This would be important if, for example, the read operations had hardware side-effects, such as clearing the A-to-D interrupt.  This is why you should always declare hardware registers (or more accurately, special function registers) as volatile, without trying to out-think Don's simple guidelines.

Graham Davies
ECROS Technology
www.ecrostech.com