Having my code "optimized away"

Go To Last Post
25 posts / 0 new
Author
Message
#1
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Disclaimer: I have already read some topics about optimization on these forums, but didn't find what I looked for.

Hello, I've been writing some code related to strings and it seems that the gcc compiler has some trouble with pointers. In my case, a pointer got completely removed after optimization and so was the code using it.

Here's the subroutine:

void speedToText(){
	char * tmpStr = "---------";
	char tmpChar;
	char * pntr;

	pntr = tmpStr + 2;
	uint16_t spd = 312;
	itoa(spd, tmpStr, 10);
	
	tmpChar = *pntr;
	*pntr = '.';
	pntr++;
	*pntr = tmpChar;
	pntr++;
	*pntr = 0;	

	strcat(pntr, " km/h");
	msg = tmpStr;
}

The purpose of the function is to take a 3-digit integer and turn it into a string as such:
312 -> "31.2 km/h"

this code produces the following assembly where everything related to *pntr is killed and the string produced is simply "312":

void speedToText(){
 a3e:	0f 93       	push	r16
 a40:	1f 93       	push	r17
	char tmpChar;
	char * pntr;

	pntr = tmpStr + 2;
	uint16_t spd = 312;
	itoa(spd, tmpStr, 10);
 a42:	00 e0       	ldi	r16, 0x00	; 0
 a44:	11 e0       	ldi	r17, 0x01	; 1
 a46:	88 e3       	ldi	r24, 0x38	; 56
 a48:	91 e0       	ldi	r25, 0x01	; 1
 a4a:	b8 01       	movw	r22, r16
 a4c:	4a e0       	ldi	r20, 0x0A	; 10
 a4e:	50 e0       	ldi	r21, 0x00	; 0
 a50:	0e 94 ad 05 	call	0xb5a	; 0xb5a 
	pntr++;
	*pntr = tmpChar;
	pntr++;
	*pntr = 0;	

	strcat(pntr, " km/h");
 a54:	c8 01       	movw	r24, r16
 a56:	04 96       	adiw	r24, 0x04	; 4
 a58:	6a e0       	ldi	r22, 0x0A	; 10
 a5a:	71 e0       	ldi	r23, 0x01	; 1
 a5c:	0e 94 a2 05 	call	0xb44	; 0xb44 
	msg = tmpStr;
 a60:	10 93 1d 01 	sts	0x011D, r17
 a64:	00 93 1c 01 	sts	0x011C, r16
}
 a68:	1f 91       	pop	r17
 a6a:	0f 91       	pop	r16
 a6c:	08 95       	ret

As a comparision, here is the same subroutine, but with *pntr defined as a global variable (simply marking pntr as volatile didn't help). It produces the right string.

void speedToText(){
 a3e:	0f 93       	push	r16
 a40:	1f 93       	push	r17
	char * tmpStr = "---------";
	char tmpChar;

	pntr = tmpStr + 2;
 a42:	02 e0       	ldi	r16, 0x02	; 2
 a44:	11 e0       	ldi	r17, 0x01	; 1
 a46:	10 93 ec 01 	sts	0x01EC, r17
 a4a:	00 93 eb 01 	sts	0x01EB, r16
	uint16_t spd = 312;
	itoa(spd, tmpStr, 10);
 a4e:	02 50       	subi	r16, 0x02	; 2
 a50:	10 40       	sbci	r17, 0x00	; 0
 a52:	88 e3       	ldi	r24, 0x38	; 56
 a54:	91 e0       	ldi	r25, 0x01	; 1
 a56:	b8 01       	movw	r22, r16
 a58:	4a e0       	ldi	r20, 0x0A	; 10
 a5a:	50 e0       	ldi	r21, 0x00	; 0
 a5c:	0e 94 d2 05 	call	0xba4	; 0xba4 
	
	tmpChar = *pntr;
 a60:	e0 91 eb 01 	lds	r30, 0x01EB
 a64:	f0 91 ec 01 	lds	r31, 0x01EC
 a68:	90 81       	ld	r25, Z
	*pntr = '.';
 a6a:	8e e2       	ldi	r24, 0x2E	; 46
 a6c:	80 83       	st	Z, r24
	pntr++;
 a6e:	e0 91 eb 01 	lds	r30, 0x01EB
 a72:	f0 91 ec 01 	lds	r31, 0x01EC
 a76:	31 96       	adiw	r30, 0x01	; 1
 a78:	f0 93 ec 01 	sts	0x01EC, r31
 a7c:	e0 93 eb 01 	sts	0x01EB, r30
 a80:	31 97       	sbiw	r30, 0x01	; 1
	*pntr = tmpChar;
 a82:	91 83       	std	Z+1, r25	; 0x01
	pntr++;
 a84:	e0 91 eb 01 	lds	r30, 0x01EB
 a88:	f0 91 ec 01 	lds	r31, 0x01EC
 a8c:	31 96       	adiw	r30, 0x01	; 1
 a8e:	f0 93 ec 01 	sts	0x01EC, r31
 a92:	e0 93 eb 01 	sts	0x01EB, r30
 a96:	31 97       	sbiw	r30, 0x01	; 1
	*pntr = 0;	
 a98:	11 82       	std	Z+1, r1	; 0x01

	strcat(pntr, " km/h");
 a9a:	80 91 eb 01 	lds	r24, 0x01EB
 a9e:	90 91 ec 01 	lds	r25, 0x01EC
 aa2:	6a e0       	ldi	r22, 0x0A	; 10
 aa4:	71 e0       	ldi	r23, 0x01	; 1
 aa6:	0e 94 c7 05 	call	0xb8e	; 0xb8e 
	msg = tmpStr;
 aaa:	10 93 1d 01 	sts	0x011D, r17
 aae:	00 93 1c 01 	sts	0x011C, r16
}
 ab2:	1f 91       	pop	r17
 ab4:	0f 91       	pop	r16
 ab6:	08 95       	ret

Making *pntr a global variable is an easy workaround but I'd like to know for future references why is the compiler acting so. As a side note, I'm not familiar at all with compiling parameters.

Any comments would be appreciated

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

You are breaking the rules of C by assigning tmpStr to the global "msg" at the end of the function. I assume "msg" is a pointer to char but what do you think it's going to be pointing at when that function returns? Remember that locals (unless defined 'static') only exist for as long as the function is in context. So tmpStr (and the characters it points to), tmpChar and pntr are created on the stack when the function is entered and destroyed (or rather "lost") when it exits.

So "msg" will be pointing to something that no longer exists.

While it doesn't matter if tmpChar and pntr are destroyed. You need to make tmpStr 'static' When I do this I get:

000000ac :
#include 
#include 

char * msg;

void speedToText(){ 
  ac:	0f 93       	push	r16
  ae:	1f 93       	push	r17
   static char * tmpStr = "---------"; 
   char tmpChar; 
   char * pntr; 

   pntr = tmpStr + 2; 
  b0:	00 91 10 01 	lds	r16, 0x0110
  b4:	10 91 11 01 	lds	r17, 0x0111
   uint16_t spd = 312; 
   itoa(spd, tmpStr, 10); 
  b8:	88 e3       	ldi	r24, 0x38	; 56
  ba:	91 e0       	ldi	r25, 0x01	; 1
  bc:	b8 01       	movw	r22, r16
  be:	4a e0       	ldi	r20, 0x0A	; 10
  c0:	50 e0       	ldi	r21, 0x00	; 0
  c2:	0e 94 83 00 	call	0x106	; 0x106 
    
   tmpChar = *pntr; 
  c6:	f8 01       	movw	r30, r16
  c8:	92 81       	ldd	r25, Z+2	; 0x02
   *pntr = '.'; 
  ca:	8e e2       	ldi	r24, 0x2E	; 46
  cc:	82 83       	std	Z+2, r24	; 0x02
   pntr++; 
   *pntr = tmpChar; 
  ce:	93 83       	std	Z+3, r25	; 0x03
   pntr++; 
   *pntr = 0;    
  d0:	14 82       	std	Z+4, r1	; 0x04

   strcat(pntr, " km/h"); 
  d2:	c8 01       	movw	r24, r16
  d4:	04 96       	adiw	r24, 0x04	; 4
  d6:	60 e0       	ldi	r22, 0x00	; 0
  d8:	71 e0       	ldi	r23, 0x01	; 1
  da:	0e 94 78 00 	call	0xf0	; 0xf0 
   msg = tmpStr; 
  de:	10 93 13 01 	sts	0x0113, r17
  e2:	00 93 12 01 	sts	0x0112, r16
} 
  e6:	1f 91       	pop	r17
  e8:	0f 91       	pop	r16
  ea:	08 95       	ret

which looks similar to your "working" version using a global.

The only difference between making tmpStr 'static' within the function of making it global (along with the storage its pointing to) is simply a question of name scope. In both cases the pointer and the array will be in .data (because you initialise with "-------") but when it's a static local nothing outside the function can refer to 'tmpStr' by name while as a global other functions/files can refer to it.

To be honest unless name pollution is an issue or you are one of those computer scientists with their head up their rear end when in comes to data hiding (I think the term is "C++ programmers" ;-)) I'd just go with your original solution of making it global (in which case 'msg' is not needed as it'd just be a copy of the tmpStr pointer).

Cliff

PS on the more general question of optimization and code being discarded I recently wrote an article about that in the Tutorial Forum.

Last Edited: Sun. Sep 12, 2010 - 07:47 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Why not just say

dstrtof(31.2, 4, 1, tmpstr);
strcat(tmpstr, "km/h");

If you really want to reflect a variable speed, you will be using a variable. The Compiler would not be able to optimise it. e.g.

void speedToText(int16_t spd)
{
    ...

Completely untested.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Thank you very much for the explanation Clawson, it indeed makes a lot of sense.
I was thrown of by the way gcc works, since it keeps all strings in memory, even out of scope.

david, spd is meant to be assigned to the value of a global variable "speed". 312 is just a test value.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
david, spd is meant to be assigned to the value of a global variable "speed". 312 is just a test value.

Yes. But the Compiler does not know that. It just sees that you are manipulating a constant value. So it knows that your buffer holds "3 1 2 \0" at Compile time.

You have to think very carefully when you produce 'test' code for a compiler. It can see if you are doing something trivial, and act accordingly. When you are assuming it is stupid, you get caught out!

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Handi3 wrote:

	char * tmpStr = "---------";

Already that code is wrong. You are setting up the pointer so that you later write to the memory space of a literal, which is supposed to be constant.

char tmpStr[9];

As for the rest of the code: Needlessly convoluted. Get a good C textbook and read it.

Stealing Proteus doesn't make you an engineer.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

ArnoldB wrote:
Get a good C textbook and read it.

Could you recommend me one? Preferably available online (for free).

Quote:
As for the rest of the code: Needlessly convoluted.

Could you please post a cleaner version. It's really the first time I'm working with strings in avr, maybe even strings in c. I used the first way that I found to do what I needed.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
Could you please post a cleaner version. It's really the first time I'm working with strings in avr, maybe even strings in c. I used the first way that I found to do what I needed.

char *speedToText(int16_t spd)
{
    static char tmpstr[10];           // permanent space
    dstrtof(0.1 * spd, 4, 1, tmpstr); // format it
    strcat(tmpstr, "km/h");           // append the text
    return tmpstr;                    // return the result
}

You could do it more neatly with sprintf(). n.b. you would need to link with the big f-p printf library.

In the unlikely event that you do not have enough flash space, you can avoid the floating point. You could also supply the buffer to the function as well. So you could use the buffer for other purposes.

There will be people who scream about code size and efficiency. Why worry? You want something that is easy to understand. Always works perfectly. Easy to maintain.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Already that code is wrong. You are setting up the pointer so that you later write to the memory space of a literal, which is supposed to be constant.

What in that says the string array it creates is constant? I don't really see what the difference is whether I:

char * str = "Hello";

or

char str[] = "Hello";

it's just as valid to then say:

str[1] = 'a';

Admittedly, personally I'd never use either if this was a character buffer, I'd dimension it with a known size so that I could later ensure the index never gets beyond the known dimension.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
I don't really see what the difference is whether I:

char * str = "Hello";

or

char str[] = "Hello";


The difference is substantial.

http://c-faq.com/decl/strlitinit...

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

But Jan that answer applies to Von Neumann where it's true that the compiler probably has a data section called something like .rom and may be mapped to invariable storage within the memory map.

This is a Harvard AVR and the array will be in RAM (you have to go through hoops to persuade the compiler to keep data in ROM) so it's just as modifiable as the array.

I guess there's an argument for saying that avr-gcc should be modified to locate such constant text into flash and access through LPM but at the point of access there'd need to be a marker byte to say whether to access with LD/LDS or LPM.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
But Jan that answer applies to Von Neumann where it's true that the compiler probably has a data section called something like .rom and may be mapped to invariable storage within the memory map.

This is a Harvard AVR and the array will be in RAM (you have to go through hoops to persuade the compiler to keep data in ROM) so it's just as modifiable as the array.

char *s = "string"; tells the compiler to create a *pointer*, and initialise it to beginning of a string (zero termintated blah blah blah). Subsequent read access to *s should return 's', *(s+1) should return 't' etc. However, the compiler is NOT obliged to create that string in RAM or anywhere!

There are (at least) two cases when the compiler (optimiser, rather, if you like) might NOT create that string in RAM (none of them is von Neumann/Harvard specific):

1. such string already exists - many compilers do that; writing through the pointer then modifies unexpectedly other strings
2. the optimiser can infer directly all accesses, then it can optimise away the string altogether - e.g. if only *(s+1) is read, it can replace it with a 't' constant.

Write accesses through such pointer are thus undefined; which could be rephrased to "these strings are const" (bearing in mind the specific meaning of "const" in C).

C99: 6.4.5#6, 6.7.8#32 (Example 8)

This is the consequence of that both the strings and arrays are only mocked up in C. (Oh, yes. I DO hate C.)

JW

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Yes that's what the theory says but this will work on avr-gcc. I do concede that your point (1) may be valid here IFF they ever persuade the optimiser to congolmerate common strings. But as it stands this works. (though I do agree it is ill advised to do it).

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

clawson wrote:
Yes that's what the theory says but this will work
Well, in my opinion the EU should have adopted a directive to ceremoniously burn diplomas of engineers saying things like this... ;-)

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

If that ever comes true wek, then I think we run out of engineers in the EU extremely fast......

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

meslomp wrote:
If that ever comes true wek, then I think we run out of engineers in the EU extremely fast......
Maybe.

In fact, it may quite well happen that I will be the very first in the row... ;-)

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, I can see an interesting discussion arising here.

Quote:
static char tmpstr[10]; // permanent space

Done. That does indeed look better than phantom pointers.

Quote:
dtostrf(0.1 * spd, 4, 1, tmpstr); // format it

Meh, it is a neat little function indeed, but performance wise it I'd rather avoid it when not needed, along with floating point arithmetic on an 8-bit mcu.

So I got this version now (814 cycles):

void speedToText(){
	static char tmpStr[9];
	char * pntr = tmpStr + 2;
	char tmpChar;

	uint16_t spd = speed;
	if (spd>999) spd = 999;
	if (spd<100) spd = 100;
	itoa(spd, tmpStr, 10);
	
	tmpChar = *pntr;
	*pntr = '.';
	pntr++;
	*pntr = tmpChar;
	pntr++;
	*pntr = 0;

	strcat(pntr, " KM/H");
	msg = tmpStr;
}

vs the cleaner, but much slower (3533 cycles):

void speedToText2(){
  static char tmpstr[10];           // permanent space 
	uint16_t spd = speed;
	if (spd>999) spd = 999;
	if (spd<100) spd = 100;
  dtostrf(0.1 * spd, 4, 1, tmpstr); // format it 
  strcat(tmpstr, " KM/H");           // append the text 
	msg = tmpstr;
}

In my case I could as well do with those 2500 extra cycles, but I do not see how is the above version "Needlessly convoluted".

P.S.: I made some changes to the code above: dstrtof -> dtostrf and turned the function into a subroutine.

EDIT: it also seems like speedToText2 took 274 bytes out of my RAM.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0
sprintf(buf, "%04.1f KM/H", 0.1 * speed);

looks pretty neat to me.

Yes. Of course it will use f-p and a large # cycles. But so what?

sprintf(buf, "%02u.%01u KM/H", speed/10, speed % 10);

will avoid floating point.

I would agree that if you are trying to squeeze a program into a Tiny2313, you may be tight for flash space.

In which case you have to do things the hard way.

But be realistic. You are probably going to display on a LCD or RS232. You are expecting a human to read it. So when you can read and absorb text in milliseconds the efficiency begins to matter. My tired old eyes do not work that fast. And nor does my brain.

David.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Meh, it is a neat little function indeed, but performance wise it I'd rather avoid it when not needed, along with floating point arithmetic on an 8-bit mcu.

So I got this version now (814 cycles):
...
vs the cleaner, but much slower (3533 cycles):

Quote:

Well, I can see an interesting discussion arising here.

It has already arisen. Digest this thread:
https://www.avrfreaks.net/index.p...

Also see
https://www.avrfreaks.net/index.p...

and the posting of my "utoaz2()" routine in
https://www.avrfreaks.net/index.p...

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
However, the compiler is NOT obliged to create that string in RAM or anywhere!
Untrue. Read-only memory can only be used if the literal is const-qualified and non-volatile:
Quote:
Like string literals, const-qualified compound literals can be placed into read-only memory and can even be shared.
...
The implementation may place a const object that is not volatile in a read-only region of storage.

Regards,
Steve A.

The Board helps those that help themselves.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:
You are probably going to display on a LCD or RS232
Nope, on a bike wheel :).

I don't mind using more flash space or extra 2700 cycles every second, but I *do mind* the extra 274 bytes of my RAM disappearing god knows where. The resolution/size/amount of colors I can display is directly affected by the amount of RAM I have and 1k isn't what I'd call luxury when it comes to bitmaps.

Anyway, what I really meant to say is: Why use complex mathematics (mcu-wise) when all I want to do is to insert a point between 2 characters!

@theusch. The interesting discussion I was referring to was the one between wek and clawson. Thanks for those links anyway, they might provide useful at some point

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Koshchi wrote:
Quote:
However, the compiler is NOT obliged to create that string in RAM or anywhere!
Untrue. Read-only memory can only be used if the literal is const-qualified and non-volatile:

Steve,

There is some merit in the "volatile", so let's put that aside. As far as const is concerned, you incorrectly reversed the implication.

Quote:
Like string literals, const-qualified compound literals can be placed into read-only memory and can even be shared.

Which means, that not only const qualified compound literals, but also [any qualified or unqualified] string literals can be placed in ROM and shared.

Quote:
The implementation may place a const object that is not volatile in a read-only region of storage.
Thus, it may place also other objects in read-only storage.

None of the above is an imperative, this is why those are quotes from examples and footnotes, which are mainly intended as compiler implementation guidelines and hints.

There are more arguments for my assertion, above the verses I listed above the main one is of course the general "allowance of optimisations" in 5.1.2.3.

Jan

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

Anyway, what I really meant to say is: Why use complex mathematics (mcu-wise) when all I want to do is to insert a point between 2 characters!

@theusch. The interesting discussion I was referring to was the one between wek and clawson. Thanks for those links anyway, they might provide useful at some point


Duh--didja click through as suggested, and use the utoaz/utoaz2 routines? Are they fast/small enough for you? What about danni's elegant routine?

BTW, somewhere there is a very elegant and VERY tiny recursive routine to do the job:
https://www.avrfreaks.net/index.p...

This discussion has a link to a technical treatise:
https://www.avrfreaks.net/index.p...
http://www.cs.uiowa.edu/~jones/b...

For LCD/LED display I almost always want things right-justified and leading-zero suppressed. Thus, rather than having a "post processing task" to do that (don't forget to count those cycles and bytes) I do it during the conversion.

The actual conversion of utoa_() is very straightforward, and not over-efficient. (Didja digest the thread as suggested?) But overall, it lets one easily insert the decimal point--as you are crying for! A utoaz3(), for example, could take a variable in millivolts and display the reading. (The "u" is for "unsigned"; I have variants for singed work and floating minus sign.)

You seem to be decrying cycles/code space but don't seem willing to digest previous work. I'm out.

Lee

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Quote:

I have variants for singed work and floating minus sign.

...but I might have gotten burnt by them.

You can put lipstick on a pig, but it is still a pig.

I've never met a pig I didn't like, as long as you have some salt and pepper.

  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

@theush: I've read through those threads since you seemed to be emphasizing so much on them. Your utoaz2 seems to do the job well, and while I appreciate when people share stuff in the community I usually follow a simple rule: If it takes me more time to find and learn how to use existing functions than to write my own ones, I rather write my own ones.

Besides, I didn't really have any problems with that part of my code to start with, the discussion came there only because Arnold pointed out that my code needed some clean-up. I corrected the part that was logically wrong and I believe it is now fine as is, so why fix what ain't broken?