Aggressive Loop Optimization? [solved]

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

Not sure exactly where this belongs. I'm getting the following warning message when I compile.

 

 

The suspect code is:

static uint8_t EEMEM waveTable[] = {128,140,152,165,176,188,198,208,218,226,237,253,250, /*Snipped for brevity*/ 245,240,234,226,218,208,198,188,176,165,128,115,103,90}; //400 Bytes Total

static uint16_t indxTable[]={0,64,128,192,256,272,304,352};

static uint16_t DAC_16[nSAMPLES];


uint8_t *init_table(uint8_t n, uint8_t p, uint8_t bright, uint8_t dark, uint16_t yrefPeak)
{
	uint16_t m;
	
	for (uint8_t k=0; k<p; k++) {
		m=indxTable[k+n];
		//DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[n][k]);   // 2D layout.
		DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[m]);        // 1D layout.
	}
	for (uint8_t n=0; n<nSAMPLES; n++) {
		DAC_16[n]= (bright - dark)*DAC_16[n] + dark*yrefPeak;	// Modify data to adjust brightness.
	}
	return (uint8_t*)&DAC_16[0];
}

Apparently, the optimizer -0s doesn't like the way I'm taking the address of a specific byte from the waveTable data. Am I correct in believing it's legal to take the address of a specific element of a one dimensional array like this?

I originally had the data arranged as a 2 dimensional array and everything was fine when I did this: DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[n][k]); // 2D layout. I rearranged the data into a 1D array because I was running out of EEPROM. It's choking on: DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[m]); // 1D layout.

 

It's only a warning, but the program no longer works correctly. looking at the watch window if I type in &waveTable[n], where n is any integer form 0 to the max extent of the waveform data it is returning the correct address in EEPROM. So I can't figure why it's not working. Any ideas? Thanks.

 

Greg

 

 

 

This topic has a solution.
Last Edited: Fri. Sep 26, 2014 - 03:12 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

The compiler is correct!

 

when you do this :

m=indxTable[k+n];

n is undefined!

 

so just move 

uint8_t n=0;

up

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

so just move 

Or just change:

	for (uint8_t k=0; k<p; k++) {

to be

	for (uint8_t k=0, n=0; k<p; k++) {

 

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

n is not undefined. It's the first argument passed to the function.  int8_t *init_table(uint8_t n, uint8_t p, uint8_t bright, uint8_t dark, uint16_t yrefPeak)

I probably should post the .lss output too. By the way, it's a ATmega 162.

uint8_t *init_table(uint8_t n, uint8_t p, uint8_t dark, uint16_t yrefPeak)
{
 d58:	9d e5       	ldi	r25, 0x5D	; 93
 d5a:	e9 2e       	mov	r14, r25
 d5c:	93 e0       	ldi	r25, 0x03	; 3
 d5e:	f9 2e       	mov	r15, r25
 d60:	00 e0       	ldi	r16, 0x00	; 0
 d62:	10 e0       	ldi	r17, 0x00	; 0
 d64:	f5 01       	movw	r30, r10
 d66:	e0 0f       	add	r30, r16
 d68:	f1 1f       	adc	r31, r17
	uint16_t m;
	
	for (uint8_t k=0; k<nSAMPLES; k++) {
		m=indxTable[k+n];
		//DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[n][k]);
		DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[m]);
 d6a:	80 81       	ld	r24, Z
 d6c:	91 81       	ldd	r25, Z+1	; 0x01
 d6e:	80 50       	subi	r24, 0x00	; 0
 d70:	90 40       	sbci	r25, 0x00	; 0
 d72:	4a 83       	std	Y+2, r20	; 0x02
 d74:	69 83       	std	Y+1, r22	; 0x01
 d76:	0e 94 3e 07 	call	0xe7c	; 0xe7c <__eerd_byte_m162>
 d7a:	90 e0       	ldi	r25, 0x00	; 0
 d7c:	f7 01       	movw	r30, r14
 d7e:	81 93       	st	Z+, r24
 d80:	91 93       	st	Z+, r25
 d82:	7f 01       	movw	r14, r30
 d84:	0e 5f       	subi	r16, 0xFE	; 254
 d86:	1f 4f       	sbci	r17, 0xFF	; 255
		
	uint16_t m;
	
	for (uint8_t k=0; k<nSAMPLES; k++) {
 d88:	4a 81       	ldd	r20, Y+2	; 0x02
 d8a:	69 81       	ldd	r22, Y+1	; 0x01
 d8c:	00 38       	cpi	r16, 0x80	; 128
 d8e:	11 05       	cpc	r17, r1
 d90:	49 f7       	brne	.-46     	; 0xd64 <init_table+0x46>
		m=indxTable[k+n];
		//DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[n][k]);
		DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[m]);
	}
	for (uint8_t n=0; n<nSAMPLES; n++) {
		DAC_16[n]= (p - dark)*DAC_16[n] + dark*yrefPeak;	// Modify data to adjust brightness.
 d92:	50 e0       	ldi	r21, 0x00	; 0
 d94:	4c 9d       	mul	r20, r12
 d96:	90 01       	movw	r18, r0
 d98:	4d 9d       	mul	r20, r13
 d9a:	30 0d       	add	r19, r0
 d9c:	5c 9d       	mul	r21, r12
 d9e:	30 0d       	add	r19, r0
 da0:	11 24       	eor	r1, r1
 da2:	ed e5       	ldi	r30, 0x5D	; 93
 da4:	f3 e0       	ldi	r31, 0x03	; 3
 da6:	70 e0       	ldi	r23, 0x00	; 0
 da8:	64 1b       	sub	r22, r20
 daa:	75 0b       	sbc	r23, r21
 dac:	40 81       	ld	r20, Z
 dae:	51 81       	ldd	r21, Z+1	; 0x01
 db0:	64 9f       	mul	r22, r20
 db2:	c0 01       	movw	r24, r0
 db4:	65 9f       	mul	r22, r21
 db6:	90 0d       	add	r25, r0
 db8:	74 9f       	mul	r23, r20
 dba:	90 0d       	add	r25, r0
 dbc:	11 24       	eor	r1, r1
 dbe:	82 0f       	add	r24, r18
 dc0:	93 1f       	adc	r25, r19
 dc2:	81 93       	st	Z+, r24
 dc4:	91 93       	st	Z+, r25
	for (uint8_t k=0; k<nSAMPLES; k++) {
		m=indxTable[k+n];
		//DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[n][k]);
		DAC_16[k] = (uint16_t)eeprom_read_byte (&waveTable[m]);
	}
	for (uint8_t n=0; n<nSAMPLES; n++) {
 dc6:	83 e0       	ldi	r24, 0x03	; 3
 dc8:	ed 3d       	cpi	r30, 0xDD	; 221
 dca:	f8 07       	cpc	r31, r24
 dcc:	79 f7       	brne	.-34     	; 0xdac <init_table+0x8e>
		DAC_16[n]= (p - dark)*DAC_16[n] + dark*yrefPeak;	// Modify data to adjust brightness.
	}
	
	return (uint8_t*)&DAC_16[0];

}
dce:    8d e5           ldi    r24, 0x5D    ; 93
 dd0:    93 e0           ldi    r25, 0x03    ; 3
 dd2:    0f 90           pop    r0
 dd4:    0f 90           pop    r0
 dd6:    df 91           pop    r29
 dd8:    cf 91           pop    r28
 dda:    1f 91           pop    r17
 ddc:    0f 91           pop    r16
 dde:    ff 90           pop    r15
 de0:    ef 90           pop    r14
 de2:    df 90           pop    r13
 de4:    cf 90           pop    r12
 de6:    bf 90           pop    r11
 de8:    af 90           pop    r10
 dea:    08 95           ret

 

Greg

Last Edited: Fri. Sep 26, 2014 - 12:14 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Can you post a test program with the stuff above and simple main() that makes a typical invocation of init_table() so we can all "play" with this?

 

Also rather than showing Studio's "error list" can you show the raw build output? The warning when it first appears should be of the form:

warning foo.c:17:23 iteration 8u invoked undefined behaviour

The 17 is the line on which the error was noticed and the 23 is the character position on that line. Then tell us which line in your source 17:23 occurs on.

This reply has been marked as the solution. 
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Well, this is embarrassing. While putting together a test program I found the problem. Thanks for motivating me to post a test program. Upon clearing out some of the forest, the problem trees stick out.

 

This was the problem: m=indxTable[n+k]; should have been m=indxTable[n] + k;  putting k inside the brackets caused indxTable to be indexed WAY beyond its extent. Thanks.

 

Greg

Last Edited: Fri. Sep 26, 2014 - 01:04 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

Clawson, this has me mystified:

for (uint8_t k=0, n=0; k<p; k++)

I realize it's not hard to confuse me, but I've never seen a for loop with 4 expressions.

 

I've seen plenty of faulty for loops with commas between the expressions causing kiloerrors.

274,207,281-1 The largest known Mersenne Prime

Measure twice, cry, go back to the hardware store

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

commas aren't limited to for() loops. C lets you use them all over the place but a typical example is:

for (int x=0, y=100; x < 100; x++, y--) {
  plot(x,y);
}

That would draw you a sort of "diagonal" line as both x and y change at each iteration.

 

Another example with , is:

#include <stdio.h>

int a,b;

int main(void) {
    a=7, b=15;
    printf("a=%d b=%d\n", a, b);
}

 

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

The comma in this context is not a separator, it is an operator.

 

From K&R:

Quote:
A.7.18 Comma Operator

 

  expression:
    assignment-expression
    expression , assignment-expression

 

A pair of expressions separated by a comma is evaluated left-to-right, and the value of the left expression is discarded. The type and value of the result are the type and value of the right operand. All side effects from the evaluation of the left-operand are completed before beginning the evaluation of the right operand. In contexts where comma is given a special meaning, for example in lists of function arguments (Par.A.7.3.2) and lists of initializers (Par.A.8.7), the required syntactic unit is an assignment expression, so the comma operator appears only in a parenthetical grouping, for example,

 

f(a, (t=3, t+2), c)

has three arguments, the second of which has the value 5.

In C, the comma operator is also a sequence point.

 

So in the for loop above, the comma doesn't make a 4th expression.  The

k=0, n=0

counts as one expression, the for initialiser.

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]

 

Last Edited: Fri. Sep 26, 2014 - 04:03 PM
  • 1
  • 2
  • 3
  • 4
  • 5
Total votes: 0

I had no idea you could do such a thing. Cool.

 

Glad you found it, Greg! Amazing how often I discover the cause while trying to explain the problem.

274,207,281-1 The largest known Mersenne Prime

Measure twice, cry, go back to the hardware store

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

Time to read K&R front to back, Torby? ;-)

Happy 75th anniversary to one of the best movies ever made! Rick Blane [Bogart]: "Of all the gin joints, in all the towns, in all the world, she walks into mine."

 

"Some questions have no answers."[C Baird] "There comes a point where the spoon-feeding has to stop and the independent thinking has to start." [C Lawson] "There are always ways to disagree, without being disagreeable."[E Weddington] "Words represent concepts. Use the wrong words, communicate the wrong concept." [J Morin] "Persistence only goes so far if you set yourself up for failure." [Kartman]

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

joeymorin wrote:
So in the for loop above, the comma doesn't make a 4th expression.  The

k=0, n=0

counts as one expression, the for initialiser.

In Torby's example, the comma is not a comma operator:

for (uint8_t k=0, n=0; k<p; k++)

uint8_t k=0, n=0 is an internal definition of two local variables.  Its only expressions are 0 and 0 .

Being part of the definition, the equal signs are not operators either.

International Theophysical Year seems to have been forgotten..
Anyone remember the song Jukebox Band?

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

K&R disagree:

In Chapter 3, K&R wrote:
One final C operator is the comma ",'", which most often finds use in the for statement. A pair of expressions separated by a comma is evaluated left to right, and the type and value of the result are the type and value of the right operand. Thus in a for statement, it is possible to place multiple expressions in the various parts, for example to process two indices in parallel.  This is illustrated in the function reverse(s), which reverses the string s in place.

#include <string.h>

/* reverse: reverse string s in place */
void reverse(char s[])
{
    int c, i, j;
    for (i = 0, j = strlen(s)-1; i < j; i++, j--) {
        c = s[i];
        s[i] = s[j];
        s[j] = c;
    }
}

The commas that separate function arguments, variables in declarations, etc., are not comma operators, and do not guarantee left to right evaluation.

Comma operators should be used sparingly. The most suitable uses are for constructs strongly related to each other, as in the for loop in reverse, and in macros where a multistep computation has to be a single expression. A comma expression might also be appropriate for the exchange of elements in reverse, where the exchange can be thought of a single operation:

for (i = 0, j = strlen(s)-1; i < j; i++, j--)
    c = s[i], s[i] = s[j], s[j] = c;

"Experience is what enables you to recognise a mistake the second time you make it."

"Good judgement comes from experience.  Experience comes from bad judgement."

"When you hear hoofbeats, think horses, not unicorns."

"Fast.  Cheap.  Good.  Pick two."

"Read a lot.  Write a lot."

"We see a lot of arses on handlebars around here." - [J Ekdahl]