Gcc 4.3.3 (WinAVR 20100110) while loop misoptimization

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

Hi,
Can you confirm that in some situations gcc misoptimize while loop?
The code:

Quote:
uint8_t portmask=1;
uint8_t tmpADCNo=variable;
while(tmpADCNo--) portmask<<=1;

produces:

+00002362:   E091        LDI       R25,0x01       Load immediate
+00002363:   C002        RJMP      PC+0x0003      Relative jump
59:       	while(tmpADCNo--) portmask<<=1;
+00002364:   0F99        LSL       R25            Logical Shift Left
+00002365:   5081        SUBI      R24,0x01       Subtract immediate
+00002366:   2388        TST       R24            Test for Zero or Minus
+00002367:   F7E1        BRNE      PC-0x03        Branch if not equal

In above code TST R24 is not necessary because Z flag has correct status after SUBI instruction. Can you confirm this, so I can send gcc bug?

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

...except if r24/tmpADCNo/variable is 0 when it enters (the rjmp to tst r24). So you need a tst somewhere.

Show a better optimization, then submit whatever you got to the gcc people.

(you could probably play musical chairs with those instructions, but in the end I think everyone is sitting down)

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

So it the case the variable==0 it is still better to move tst outside the loop - we will not get any size benefits, but the loop will be executed faster.

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

Don't you need to provide version information and optimization level?

It might be interesting to compare to the straightforward "portmask <<= frog;". I try to avoid variable shifts as I know it is expensive on an AVR, but sometimes it is just plain needed. I tend to use the <<= instead of constructing the explicit loop.

But it is hard to tell out of context. The code you gave has the variables in registers already. With a "bigger context" there may be some register-washing.

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

curtvm wrote:
...except if r24/tmpADCNo/variable is 0 when it enters (the rjmp to tst r24). So you need a tst somewhere.

Show a better optimization, then submit whatever you got to the gcc people.

(you could probably play musical chairs with those instructions, but in the end I think everyone is sitting down)


   LDI R25, 0x01
   SUBI R24, -1
   RJMP 2f
1: LSL R25
2: SUBI R24, 1
   BRNE 1b
;  R24==0
;  One more instruction is necessary if one wants 0xFF
   LDI R25, 0x01
   TST R24
   BREQ 2f
1: LSL R25
   SUBI R24, 1
   BRNE 1b
2:
; R24==0
; One more instruction is necessary if one wants 0xFF.

Moderation in all things. -- ancient proverb

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

Why would you complain about the compiler not optimizing well when the code you put into it is clearly not optimal? Just use:

portmask <<= variable;

and let the compiler optimize that.

Regards,
Steve A.

The Board helps those that help themselves.

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

Koshchi wrote:
Why would you complain about the compiler not optimizing well when the code you put into it is clearly not optimal? Just use:

portmask <<= variable;

and let the compiler optimize that.

Good question, maybe because portmask<<=variable; is even worse:

	uint8_t portmask=1;
	portmask<<=ADCNo;
    443c:	81 e0       	ldi	r24, 0x01	; 1
    443e:	90 e0       	ldi	r25, 0x00	; 0
    4440:	06 2e       	mov	r0, r22
    4442:	02 c0       	rjmp	.+4      	; 0x4448 <_ZN10ADCHandler20TestIfADCInputIsOpenEh+0x10>
    4444:	88 0f       	add	r24, r24
    4446:	99 1f       	adc	r25, r25
    4448:	0a 94       	dec	r0
    444a:	e2 f7       	brpl	.-8      	; 0x4444 <_ZN10ADCHandler20TestIfADCInputIsOpenEh+0xc>
	PORTF=portmask;		//Enable pull up resistor on appropiate pin of PORTF (ADC channel)
    444c:	80 93 62 00 	sts	0x0062, r24

As you can see now portmask is automatically promoted to integer... I tried to avoid this using typecast, but without any results...
I use -Os optimization level.

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

Quote:

<_ZN10ADCHandler20TestIfADCInputIsOpenEh

Ah they don't write function names like that any more!

(what a joy C++ name mangling is)

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

Quote:

TestIfADCInputIsOpenEh

Must be Canadian. Or the Upper Peninsula of Michigan.

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.