Re: [avr-libc-dev] poor optimisation

[Joerg Wunsch]

As Rob Ward wrote:

> Hi. I think I have an example of some very poor avr-gcc optimisation. I 

First, the more appropriate list would be address@hidden  You might
find that Denis Chertykov is also reading there, while he probably
isn't reading this list here.

> have pasted my c code below, followed by a .lst listing. I have also 
> included my makefile at the end.

> and here is the .lst output
> 
> note all the (lots of) wasted rom space because of these...

If you look once more, you'll notice that it's not wasted at all. ;-)

There are a lot of branch instructions jumping into that area, like

>   if (DataMid  & 0x20) SetDataHigh(); else SetDataLow();  SetClockHigh(); 
> SetClockLow();
>   ce: 85 ff           sbrs    r24, 5
>   d0: 3c c0           rjmp    .+120           ; 0x14a

...to look at the one matching your example:

>  14a: c0 98           cbi     0x18, 0 ; 24
>  14c: c3 cf           rjmp    .-122           ; 0xd4

You are simply fooled by the optimzer rearranging your code.  Remember
that source code comments in the .lst file are not generated by the
compiler, but rather added by the assembler based on the line number
information provided by the compiler.  However, since as you can see,
the compiler occasionally restructures the code (see
http://savannah.nongnu.org/download/avr-libc/doc/avr-libc-user-manual/FAQ.html#faq_gdboptimize
for an explanation), it sometimes cannot connect one source code line
number to one area of assembler code since it's rather two areas in
the assembler code that belong to this source line.  Thus, you
suddenly get all the pieces of assembler code that are remnants of
older source code lines listed under the last of all your C lines in
the .lst file.

Actually, it's not the compiler optimizing poorly but the programmer
who wrote that code. :-)  The compiler simply did his best to optimize
it anyway.

If i were you, i'd rewrite it similar to:

SIGNAL(SIG_OVERFLOW0) {
  uint8_t Data[3], *dp;   // please use the names from the
                          // C99 standard, see <inttypes.h>
  int8_t i, j;

  outp(Timer0Interval, TCNT0);

  if (InConfigMode == TRUE) {           // At startup, we are InConfigMode
                                        // so we can send the 19 config bits.
    InConfigMode = FALSE;               // Enter here only once at startup
    Data[2] = 0x80;
    Data[1] = 0x30;
    Data[0] = 0x01;
  }
  else {
    if (StepDirn == CCW) {
      Data[2] = PRG_RDB(&WordHighCCW[StepPos]);
      Data[1] = PRG_RDB(&WordMidCCW[StepPos]);
      Data[0] = PRG_RDB(&WordLowCCW[StepPos]);
    }
    else { // step direction is clockwise
      Data[2] = PRG_RDB(&WordHighCW[StepPos]);
      Data[1] = PRG_RDB(&WordMidCW[StepPos]);
      Data[0] = PRG_RDB(&WordLowCW[StepPos]);
    }
  }

  SetStrobeLow();                       // set strobe low to
                                        // initiate data transfer

  for (j = 2; j >= 0; j--)
    for (i = 0, dp = Data[j]; i < 8; i++, *dp <<= 1) {
      if (*dp & 0x80) SetDataHigh();
      else SetDataLow();
      SetClockHigh(); SetClockLow();
    }
  SetStrobeHigh();                      // reset strobe to high to 
                                        // terminate data transfer

  StepCompleted = TRUE;
}

This is a typical space vs. time compromise: the loop will cause some
additional processing time, but save a lot of code.

-- 
J"org Wunsch                                           Unix support engineer
address@hidden        http://www.interface-systems.de/~j/