GCC erzeugt unlogischen Code

Druckbare Version

22.04.2010, 17:42
s.frings

GCC erzeugt unlogischen Code
Ich staune gerade, wie bescheuert der C Compiler diesen Code optimiert:

Code:

ISR(TIMER0_COMP_vect) { if (++ICR1L==100) { ICR1L=0; system_time++; } }

Ergibt compiliert:
- push r1
  push r0
  in r0, 0x3f
  push r0
  eor r1, r1
  push r24
  push r25
  push r26
  push r27
  in r24, 0x26
  subi r24, 0xFF
  out 0x26, r24
  in r24, 0x26
  cpi r24, 0x64
  brne .+40
  out 0x26, r1
  lds r24, 0x006A
  lds r25, 0x006B
  lds r26, 0x006C
  lds r27, 0x006D
  adiw r24, 0x01
  adc r26, r1
  adc r27, r1
  sts 0x006A, r24
  sts 0x006B, r25
  sts 0x006C, r26
  sts 0x006D, r27
  pop r2
  pop r26
  pop r25
  pop r24
  pop r0
  out 0x3f, r0
  pop r0
  pop r1
  reti
1) Mehrfache Ein/Ausgabe in das Register ICR1L (rot):
Wieso wird r24 zuerst in das I/O Register geschrieben und direkt danach wieder eingelesen? Der zweite Befehl ist doch total überflüssig, oder?

2) Schlecht platzierte Push/Pop für Register (blau):
Solange mein prescaler (also ICR1L bzw. r24) nicht 100 ist, werden die Register r25-r27 gar nicht verwendet. Dennoch werden sie schon bei Eintritt in die Funktion auf dem Stapel gesichert.

Das ganze ärgert mich deswegen, weil diese Interrupt Routine mit 100.000 mal pro Sekunde aufgerufen wird, da tut jeder unnötige Befehl richtig weh.

Hat vielleicht jemand eine Idee, wie man den Compiler zu etwas mehr Intelligenz bewegen kann?
22.04.2010, 18:19
Felix G

Wenn es auf jeden Befehl ankommt würde ich sowas nicht dem Compiler überlassen, sondern die ISR gleich selbst in Assembler schreiben
22.04.2010, 18:24
sternst

Re: GCC erzeugt unlogischen Code

Zitat:

Zitat von s.frings

Ich staune gerade, wie bescheuert der C Compiler diesen Code optimiert:

Und ich staune gerade, wie leichtfertig du die dir kostenlos von anderen zur Verfügung gestellte Arbeit beschimpfst. Schreibe doch mal selber einen Compiler, vielleicht kann der das dann ja besser.

Zitat:

1) Mehrfache Ein/Ausgabe in das Register ICR1L (rot):
Wieso wird r24 zuerst in das I/O Register geschrieben und direkt danach wieder eingelesen? Der zweite Befehl ist doch total überflüssig, oder?

Register sind grundsätzlich volatile deklariert. Wenn du die sich daraus ergebenden zusätzlichen Zugriffe vermeiden willst, dann arbeite mit einer temporären Kopie.

Zitat:

2) Schlecht platzierte Push/Pop für Register (blau):
Solange mein prescaler (also ICR1L bzw. r24) nicht 100 ist, werden die Register r25-r27 gar nicht verwendet. Dennoch werden sie schon bei Eintritt in die Funktion auf dem Stapel gesichert.

So arbeitet der Compiler nun mal. Es gibt nur Pro-/Epiloge am Anfang und Ende der Funktion, keine irgendwo mittendrin.

Zitat:

Das ganze ärgert mich deswegen, weil diese Interrupt Routine mit 100.000 mal pro Sekunde aufgerufen wird, da tut jeder unnötige Befehl richtig weh.

Das hatten wir doch schon im anderen Thread. Dann schreibe halt die ISR als ASM-Funktion selber. Bei dieser simplen ISR ist das doch in ein paar Minuten erledigt.
22.04.2010, 19:17
p_mork

@s.frings

Versuchs mal mit dem Code

Code:

ISR(TIMER0_COMP_vect) { unsiged char tmp = ICR1L; ICR1L = ++tmp; if (tmp==100) { ICR1L=0; system_time++; } }

Das sollte zumindest das out-in eliminieren, weil auf ICR1L in diesem fall nur zweimal zugegriffen wird. Das pushen/poppen der Register lässt sich AFAIK nicht so einfach umgehen, weil der GCC grundsätzlich alle möglicherweise benötigten Register bereits beim Funktionseintritt sichert (glaub ich zumindest irgendwo gelesen zu haben).

Ist es denn zwingend notwendig, dass system_time 32 Bit breit ist? Mit 16 Bit könnten 10 Takte eingespart werden.

Ansonsten bleibt eben nur das mehrfach erwähnte Schreiben der Funktion per Hand in Asm.

MfG Mark
22.04.2010, 19:28
sternst

Zitat:

Zitat von p_mork

Versuchs mal mit dem Code

Wenn schon temporäre Kopie, dann doch gleich komplett:

Code:

ISR (TIMER0_COMP_vect) { unsiged char tmp = ICR1L; if (++tmp == 100) { tmp = 0; system_time++; } ICR1L = tmp; }
23.04.2010, 09:43
s.frings

Mir ist nochwas aufgefallen: Im Prolog und Epilog wird Register r0 auf dem Stack gesichert, obwohl es innerhalb der Routine nicht verändert wird.

Ich erwäge tatsächlich, dieses Stück in Assembler umzuschreiben (back to the roots). Ist ja wirklich nicht schwer.

Wegen gcc: Klar bin ich dankbar, daß es dieses Programm gibt. Für den preis (kostenlos) sollte man wirkluch nicht höchste Perfektion erwarten. Da hast Du schon recht, sternst. Ich hätte mich etwas gewählter ausdrücken sollen.
23.04.2010, 10:39
markusj

Edit: Müll gelöscht ... ich hatte r0 (Scratch) und r1 (0-Register) zusammengeschmissen.

Zitat:

Zitat von avr-libc: Inline Assembler Cookbook

Register r0 may be freely used by your assembler code and need not be restored at the end of your code. It's a good idea to use __tmp_reg__ and __zero_reg__ instead of r0 or r1, just in case a new compiler version changes the register usage definitions.

Da die ISR während der Bearbeitung eines Blocks angesprungen werden kann, der r0 verwendet, muss der Wert bei Eintritt gesichert und bei Austritt wiederhergestellt werden, sofern man r0 benutzen möchte.

mfG
Markus
23.04.2010, 20:24
s.frings

Ich habe mal versucht, meine Interrupt Service Routine in Assembler umzuschreiben. Ich würde mich freuen, wenn mal jemand drüber schauen kann, ob sie korrekt ist. Mein erster Funktionstest war erfolgreich, aber das muss ja nichts bedeuten. Ich bin etwas unsicher, weil ich bisher noch nie Assembler und C kombiniert habe.

Code:

// System Zeitmesser in Millisekunden volatile uint32_t system_time; // Vorteiler für den System Zeitmesser register uint8_t prescaler __asm("r2"); // Timer 0 Unterbrechung (75khz). Inkrementiert system_time bei jedem 75ten Interrupt. // Wurde durch die folgende Assembler Version ersetzt. /* ISR(TIMER0_COMP_vect) { if (--prescaler==0) { prescaler=75; system_time++; } } */ // Timer 0 Unterbrechung (75khz). Inkrementiert system_time bei jedem 75ten Interrupt. // Diese Assembler Version ist wesentlich effizienter, als die obige C Version. void __attribute__ ((naked)) TIMER0_COMP_vect (void) { __asm__ __volatile ( "push r0" "\n\t" "in r0,__SREG__" "\n\t" // Sichere SREG in r0 "dec r2" "\n\t" // --prescaler "brne return" "\n\t" // if (prescaler!=0) goto return: "\t" "push r1" "\n\t" "\t" "push r24" "\n\t" "\t" "push r25" "\n\t" "\t" "push r26" "\n\t" "\t" "push r27" "\n\t" "\t" "ldi r24,75" "\n\t" // prescaler=75 "\t" "mov r2,r24" "\n\t" "\t" "lds r24,%0" "\n\t" // system_time++ "\t" "lds r25,%0+1" "\n\t" "\t" "lds r26,%0+2" "\n\t" "\t" "lds r27,%0+3" "\n\t" "\t" "eor r1,r1" "\n\t" "\t" "adiw r24,1" "\n\t" "\t" "adc r26,r1" "\n\t" "\t" "adc r27,r1" "\n\t" "\t" "sts %0,r24" "\n\t" "\t" "sts %0+1,r25" "\n\t" "\t" "sts %0+2,r26" "\n\t" "\t" "sts %0+3,r27" "\n\t" "\t" "pop r27" "\n\t" "\t" "pop r26" "\n\t" "\t" "pop r25" "\n\t" "\t" "pop r24" "\n\t" "\t" "pop r1" "\n\t" "return:" "\n\t" "out __SREG__,r0" "\n\t" "pop r0" "\n\t" "reti" "\n\t" :: "i" (&system_time) ); }

Verglichen mit der ersten Variante habe ich den Prescaler als RegisterVariable deklariert und die Taktfrequenz von 100khz auf 75khz reduziert (auf diese Idee hätte ich eigentlich auch eher kommen können. Bei den ersten 74 Interrupts wird lediglich der Prescaler r2 decrementiert. Nur beim 75ten Interrupt wird die 32 Bit Variable incrementiert und die dazu benutzten Register auf dem Stack gesichert (der C Compiler hatte sie schon im Prolog gesichert). Außerdem konnte ich bei meinem manuellen "Compilieren" ein paar Zeilen einsparen.
Ich verspreche mir davon, dass mein System-Timer nun erheblich weniger Grund-Last produziert (pi mal Daumen nur noch ein viertel).

Sind meine Gedanken dazu korrekt?
23.04.2010, 20:38
sternst

Ich vermisse ein "adc r25,r1".

Außerdem sehe ich ein Einsparpotenzial von weiteren 12 Takten (Hinweis: lds und sts haben keinen Einfluss auf das Carry-Flag).
23.04.2010, 20:39
Besserwessi

Ich kann da auch noch keinen Fehler finden.

Man könnte das Hochzählen von System Time noch mit weniger Registern machen, man muß ja nicht alle Bytes zu gleich ein Registern haben, 1 Byte zur Zeit reicht. Damit könnte man sich das PUSH und POP für 3 Register sparen.
Man kann auch noch R1 sparen: Hochgezählt wird ja nur wenn r2 = 0 ist, wozu also noch R1 mit 0 laden. Die Zuweisung R2 = 75 kann man ja auch am Ende machen.
23.04.2010, 20:47
s.frings

Tolle Tips, ich bin euch echt dankbar.

adc r25,1 brauche ich nicht, weil der davor stehende adiw Befehl 16 Bit incrementiert.
23.04.2010, 20:56
s.frings

Was das so gemeint, oder kann ich noch weiter optimieren?:

Code:

void __attribute__ ((naked)) TIMER0_COMP_vect (void) { __asm__ __volatile ( "push r0" "\n\t" "in r0,__SREG__" "\n\t" // Sichere SREG in r0 "dec r2" "\n\t" // --prescaler "brne return" "\n\t" // if (prescaler!=0) goto return: "push r24" "\n\t" "sec" "\n\t" // system_time++ "lds r24,%0" "\n\t" "adc r24,r2" "\n\t" "sts %0,r24" "\n\t" "lds r24,%0+1" "\n\t" "adc r24,r2" "\n\t" "sts %0+1,r24" "\n\t" "lds r24,%0+2" "\n\t" "adc r24,r2" "\n\t" "sts %0+2,r24" "\n\t" "lds r24,%0+3" "\n\t" "adc r24,r2" "\n\t" "sts %0+3,r24" "\n\t" "ldi r24,75" "\n\t" // prescaler=75 "mov r2,r24" "\n\t" "pop r24" "\n\t" "return:" "\n\t" "out __SREG__,r0" "\n\t" "pop r0" "\n\t" "reti" "\n\t" :: "i" (&system_time) ); }
23.04.2010, 21:02
s.frings

Sorry, ich muss mich einfach nochmal dazu melden: Ich freue mir gerade einen Ast ab, wie toll wir diesen Code optimiert haben. Genau da wollte hin. Supidupi, lasst uns einen Trinken gehen.
24.04.2010, 09:18
Besserwessi

Einer geht noch:

Man kann das hochzählen noch mit hilfe der Befehle SUBI ...,FF und SBCI...,FF machen. Also sozusagen -1 abziehen statt einen dazuzählen. Das spart dann noch das SEC.
24.04.2010, 20:35
s.frings

Dankeschön, ich bin für eure Hilfe dankbar. So macht das Basteln Spaß.