-         

Seite 1 von 2 12 LetzteLetzte
Ergebnis 1 bis 10 von 15

Thema: GCC erzeugt unlogischen Code

  1. #1
    Erfahrener Benutzer Fleißiges Mitglied
    Registriert seit
    19.03.2010
    Beiträge
    161

    GCC erzeugt unlogischen Code

    Anzeige

    SMARTPHONES & TABLETS-bis zu 77% RABATT-Kostenlose Lieferung-Aktuell | Cool | Unentbehrlich
    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?

  2. #2
    Erfahrener Benutzer Robotik Einstein Avatar von Felix G
    Registriert seit
    29.06.2004
    Ort
    49°32'N 8°40'E
    Alter
    34
    Beiträge
    1.780
    Wenn es auf jeden Befehl ankommt würde ich sowas nicht dem Compiler überlassen, sondern die ISR gleich selbst in Assembler schreiben
    So viele Treppen und so wenig Zeit!

  3. #3
    Erfahrener Benutzer Roboter Experte Avatar von sternst
    Registriert seit
    07.07.2008
    Beiträge
    672

    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.

    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.

    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.

    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.
    MfG
    Stefan

  4. #4
    Erfahrener Benutzer Roboter Experte
    Registriert seit
    25.03.2006
    Ort
    Darmstadt
    Alter
    26
    Beiträge
    522
    @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

  5. #5
    Erfahrener Benutzer Roboter Experte Avatar von sternst
    Registriert seit
    07.07.2008
    Beiträge
    672
    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;
    }
    MfG
    Stefan

  6. #6
    Erfahrener Benutzer Fleißiges Mitglied
    Registriert seit
    19.03.2010
    Beiträge
    161
    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.

  7. #7
    Erfahrener Benutzer Roboter Genie
    Registriert seit
    20.08.2008
    Ort
    Karlsruhe
    Alter
    29
    Beiträge
    1.221
    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

  8. #8
    Erfahrener Benutzer Fleißiges Mitglied
    Registriert seit
    19.03.2010
    Beiträge
    161
    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?

  9. #9
    Erfahrener Benutzer Roboter Experte Avatar von sternst
    Registriert seit
    07.07.2008
    Beiträge
    672
    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).
    MfG
    Stefan

  10. #10
    Erfahrener Benutzer Robotik Visionär
    Registriert seit
    26.11.2005
    Ort
    bei Uelzen (Niedersachsen)
    Beiträge
    7.942
    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.

Seite 1 von 2 12 LetzteLetzte

Berechtigungen

  • Neue Themen erstellen: Nein
  • Themen beantworten: Nein
  • Anhänge hochladen: Nein
  • Beiträge bearbeiten: Nein
  •