AVR-GCC: Code wird totoptimiert

[Karl K. (karl_k69)]

Hi,

folgendes Codefragment optimiert mir der AVR-GCC kaputt:

    static unsigned int pulseOffCtr=0;

    main ()
    {
       ...
       while (((volatile unsigned int)pulseOffCtr)>0)
       {
           if (((volatile unsigned int)pulseOffCtr)>6)
           usbPoll();
       }
       ...
    }

pulseOffCtr wird in einer Timer-getriggerten ISR verändert. Aus 
Geschwindigkeitsgründen ist die Variable selber NICHT als volatile 
definiert. Der Compiler hält den Code in der while-Schleife jetzt für 
Unfug und rationalisiert diesen weg. Spannenderweise hilft es auch 
nicht, vor Ort auf volatile zu casten (so wie im Codebeispiel oben).

Wie kann ich den GCC davon abhalten, den Codeteil kaputtzuoptimieren? 
pulseOffCtr generell als volatile zu definieren oder den 
Optimierungslevel runterzuschalten kommt leider nicht in Frage.

[Matthias L. (Gast)]

>pulseOffCtr generell als volatile zu definieren

Dann lebe damit.

[PittyJ (Gast)]

> pulseOffCtr generell als volatile zu definieren
Warum?
Warum nicht gleich richtig machen?

[W.A. (Gast)]

Karl K. schrieb:
> Wie kann ich den GCC davon abhalten, den Codeteil kaputtzuoptimieren?

Inline Assembler?

[Karl K. (karl_k69)]

W.A. schrieb:
> Inline Assembler?

Coole Idee - Danke!

[Falk B. (falk)]

@ Karl K. (karl_k69)

>folgendes Codefragment optimiert mir der AVR-GCC kaputt:


>       while (((volatile unsigned int)pulseOffCtr)>0)

Solche Cast sind immer ein klares Zeichen von Murks.

>pulseOffCtr wird in einer Timer-getriggerten ISR verändert. Aus
>Geschwindigkeitsgründen ist die Variable selber NICHT als volatile
>definiert.

Das ist ein Oxymoron. Mach die Variable volatile und fertig. Es wird 
nicht nennenswert langsamer, denn deine ISR braucht so oder so einige 
Takte.

> Der Compiler hält den Code in der while-Schleife jetzt für
>Unfug und rationalisiert diesen weg.

Recht hat er.

>pulseOffCtr generell als volatile zu definieren oder den

Ja.

[Stefan E. (sternst)]

Falk B. schrieb:
> Mach die Variable volatile und fertig. Es wird
> nicht nennenswert langsamer, denn deine ISR braucht so oder so einige
> Takte.

Außerdem kann man sich auch in der ISR auf einfache Weise mit Hilfe 
einer lokalen Variable des volatile entledigen.

[Kaj (Gast)]

Karl K. schrieb:
> Aus Geschwindigkeitsgründen
Sinnlose Mikrooptimierung

Karl K. schrieb:
> folgendes Codefragment optimiert mir der AVR-GCC kaputt:
Wenn du kaputten Code schreibst, brauchst du dich nicht beschweren, wenn 
das, was dabei rauskommt, nicht funktioniert. Den GCC trifft da keine 
Schuld.

Wenn du eine Funktion (den GCC) mit falschen Daten (dein Code) fütterst, 
brauchst du dich nicht über ein falsches Ergebnis (Code, der nicht so 
funktioniert wie du das gerne hättest) wundern.

Der Compiler weiß nicht was du dir denkst, der Compiler wertet aus, 
was du da als Code hinschreibst. Wenn der geschriebene Code nicht das 
ist, was du denkst, warum sollte der Compiler schuld sein?

[(prx) A. K. (prx)]

Karl K. schrieb:
> Spannenderweise hilft es auch
> nicht, vor Ort auf volatile zu casten (so wie im Codebeispiel oben).

Nicht weiter erstaunlich. Du castest auf diese Art nicht die Variable 
selbst, sondern den gelesenen Wert. Das ist zu spät. Allenfalls
  *(volatile ... *)&variable
brächte dich weiter. Nur wirst du dann möglicherweise feststellen, dass 
du das an diversen anderen Stellen auch machen musst und dich jenem Code 
näherst, denn du auch mit "volatile" erhältst.

> Wie kann ich den GCC davon abhalten, den Codeteil kaputtzuoptimieren?

GCC passend für deine Interpretation von C umschreiben.

[Dr. Sommer (Gast)]

Karl K. schrieb:
>     static unsigned int pulseOffCtr=0;
>
>     main ()
>     {
>        ...
>        while (((volatile unsigned int)pulseOffCtr)>0)
>        {
>            if (((volatile unsigned int)pulseOffCtr)>6)
>            usbPoll();
>        }
>        ...
>     }
Du sollst auch nicht das Ergebnis der Variable nach dem Auslesen 
casten, sondern die Variable selbt nach volatile casten:

while (*((volatile unsigned int*) &pulseOffCtr) > 0) { ... }

Falls du C++ verwendest gehts etwas schöner:

while (const_cast<volatile unsigned int&> (pulseOffCtr) > 0) { ... }

Beim Verwenden von (unsigned) int auf 8-Bit-AVR ist natürlich besondere 
Vorsicht geboten, da Zugriffe darauf nicht atomar sind. Du musst vor dem 
Zugriff die Interrupts sperren.

[Stefan F. (Gast)]

Was Dir eventuell hilft, ist ein Hybrid aus beiden. Du deklarierst die 
Variable als volarile, verwendest aber für if() einen 
zwischengespeicherten Wert:

static volatine unsigned int pulseCtr=0;

main()
{
       unsigned int tmp;
       while (tmp=pulseCtr)
       {
           if (tmp>6)
           usbPoll();
       }
}

So wird die "langsame" Variable nur einmal pro Schleifendurchlauf 
gelesen. Bei der if-Abfrage wird der Wert verwendet, der im Kopf der 
Schleife gelesen wurde. Der Compiler wird tmp in ein Register speichern, 
was schneller erreichbar ist, als eine Variable im RAM. Der 
Kopiervorgang im Schleifenkopf ist zwangsläufg ohnehin nötig.

Hier ist noch ein kleiner Fehler: Die Variable ist größer als 8 Bit, du 
solltest den Lesezugriff daher Atomar machen. Sonst kann es passieren, 
dass der Counter während des Lesens verändert wird, und das führt je 
nach Wert zu völlig falscher Funktion.

Wenn zum Beispiel der Wert mittem beim Lesezugriff von 255 nach 256 
wechselt, bekommst du (tmp) unerwartet den Wert 511.

Den fehler hatte ich selbst neulich gemacht und wurde von anderen 
Forenmitgliedern korrigiert. Nun gebe ich das gelernte gerne weiter :-)

Korrekturvorschlag:

static volatine unsigned int pulseCtr=0;

unsigned int getPulseCounter()
{
    cli();
    unsinged int copy=pulseCtr;
    sei();
    return copy;
}

main()
{
       unsigned int tmp;
       while (tmp=getPulseCounter())
       {
           if (tmp>6)
           usbPoll();
       }
}

In diesem Zusammenhang könnte es interessant sein, sich in das Thema 
"inline" einzulesen.

[(prx) A. K. (prx)]

Übrigens kann der Code auch mit "volatile" immer noch ab und zu in die 
Hose gehen, sobald diese 2-Byte-Variable Werte ausserhalb 0..255 
annimmt. Weil der Zugriff darauf nicht atomar ist. Siehe
https://www.mikrocontroller.net/articles/Interrupt#Atomarer_Datenzugriff

[Tse (Gast)]

Karl K. schrieb:
> pulseOffCtr wird in einer Timer-getriggerten ISR verändert. Aus
> Geschwindigkeitsgründen ist die Variable selber NICHT als volatile
> definiert.
Schau mal in generierten ASM Code nach, ob die ISR einfach nur schneller 
wird weil der Compiler pulseOffCtr einfach rausschmeißt...

Zeig* uns mal deine ISR, vielleicht ist sie ja nur ungeschickt 
geschrieben.

*Mit C-Formatierung

[c] C-Code [/c]

[Peter D. (peda)]

Karl K. schrieb:
> pulseOffCtr wird in einer Timer-getriggerten ISR verändert.

Dann mußt Du den Zugriff atomar kapseln (int = 2 Byte).

[Bastler (Gast)]

Volatile bleibt volatile.
Wenn ich in einer ISR verhindern will, daß volatile den GCC am 
optimieren hindert, dann Kopier ich mir die Variable in eine lokale 
Variable und am Ende wieder zurück. GCC hält die in der Regel in einem 
Register und erzeugt damit sehr kompakten Code. Wichtig ist nur: keine 
andere ISR darf auch an der Variable herumspielen. Und die Hauptschleife 
sieht die Variable, wie sie ist: ohne erkennbaren Zusammenhang zur Zeit.

[Falk B. (falk)]

@Dr. Sommer (Gast)

>Du sollst auch nicht das Ergebnis der Variable nach dem Auslesen
>casten, sondern die Variable selbt nach volatile casten:

>while (*((volatile unsigned int*) &pulseOffCtr) > 0) { ... }

Und wo ist dann der Vorteil zu einer einfachen volatile Deklaration?

Wie man Probleme mit Casts löst, die man ohne sie nie hätte . . .

[Peter II (Gast)]

Falk B. schrieb:
> Und wo ist dann der Vorteil zu einer einfachen volatile Deklaration?

das sehen wir leider nicht, weil er die ISR nicht gepostet hat. Diese 
wird ohne das volatile zumindest schneller.

Ich mag diese globale volatile auch nicht, weil es viel zu viel von der 
vorhanden Optimierung verhindert.

[Tom (Gast)]

Peter II schrieb:
> Diese
> wird ohne das volatile zumindest schneller.

Da diese nicht unterbrochen werden kann, hilft der Trick mit der lokalen 
Variablen, wenn viel mit der globalen Variablen gerechnet wird.

[Peter II (Gast)]

Tom schrieb:
> Peter II schrieb:
>> Diese
>> wird ohne das volatile zumindest schneller.
>
> Da diese nicht unterbrochen werden kann, hilft der Trick mit der lokalen
> Variablen, wenn viel mit der globalen Variablen gerechnet wird.

ist Geschmacksache, 2 Variabel mit dem gleichen Inhalt die man nicht 
vergessen darf zurückzuschreiben (mal schnell ein return reingeschrieben 
und auf einmal geht nichts mehr) oder das hässliche cast.

[Bastler (Gast)]

Der Cast macht es auf alle Fälle leichter, sich in dessen Funktion zu 
verschätzen.
Wenn die ISR so unübersichtlich wird, daß man eingestreute return's 
nicht erkennt, ...
Zudem gibt es ja auch die Möglichkeit, daß man die globale Variable gar 
nicht immer verändern will.

[Henry P. (henrylexx)]

Bastler schrieb:
> Volatile bleibt volatile.
> Wenn ich in einer ISR verhindern will, daß volatile den GCC am
> optimieren hindert, dann Kopier ich mir die Variable in eine lokale
> Variable und am Ende wieder zurück. GCC hält die in der Regel in einem
> Register und erzeugt damit sehr kompakten Code. Wichtig ist nur: keine
> andere ISR darf auch an der Variable herumspielen. Und die Hauptschleife
> sieht die Variable, wie sie ist: ohne erkennbaren Zusammenhang zur Zeit.

Querleser: mir ist die Option eine Variable "automar" zusetzen noch nie 
untergekommen, um das Umzusetzen kann man sich hier an die Möglichkeit 
von Stefan U. halten?

Stefan U. schrieb:
> Korrekturvorschlag:
> static volatine unsigned int pulseCtr=0;
>
> unsigned int getPulseCounter()
> {
>     cli();
>     unsinged int copy=pulseCtr;
>     sei();
>     return copy;
> }
>
> main()
> {
>        unsigned int tmp;
>        while (tmp=getPulseCounter())
>        {
>            if (tmp>6)
>            usbPoll();
>        }
> }

?

[Markus F. (mfro)]

Stefan U. schrieb:
> unsinged int copy=pulseCtr;

sehr unmusikalischer Code ;)

[dunno.. (Gast)]

Henry P. schrieb:
> Querleser: mir ist die Option eine Variable "automar" zusetzen noch nie
> untergekommen, um das Umzusetzen kann man sich hier an die Möglichkeit
> von Stefan U. halten?

Atomarer Zugriff: Wenn meine Anweisung für einen (kritischen/parallelen) 
Variablenzugriff in mehr als einen ASM-Befehl übersetzt wird, muss ich 
diesen Zugriff gegen Interrupts schützen, also unteilbar / atomar machen 
- Richtig.

Bei AVRs mit

cli(); // Interrupts sperren
DoSomethingDangerous();
sei; // Interrupts freigeben

[Peter D. (peda)]

Besser so:

#include <util/atomic.h>

  ATOMIC_BLOCK(ATOMIC_FORCEON){
    return pulseCtr;
  }

[Ralf G. (ralg)]

Henry P. schrieb:
> Querleser: mir ist die Option eine Variable "automar" zusetzen noch nie
> untergekommen, um das Umzusetzen kann man sich hier an die Möglichkeit
> von Stefan U. halten?

Das ist so nicht ganz in Ordnung. Wenn man so eine Funktion ...
Stefan U. schrieb:
> unsigned int getPulseCounter()
..., z.B., in einer ISR aufrufen würde, hätte man danach die Interrupts 
eingeschaltet.

Die korrekte Herangehensweise ist:
- den Interruptstatus merken
- Interrupts abschalten
- de vorherigen Status wieder herstellen

Dafür gibt's (mit etwas Schreibarbeit) Makros in der <util/atomic.h>.

[Stefan F. (Gast)]

> Besser so:

Wieso besser? Das erzeugt exakt den gleichen Rahmen mit cli() und sei().

[Stefan F. (Gast)]

> in einer ISR aufrufen würde, hätte man danach die
> Interrupts eingeschaltet.

So war das ja nicht gedacht. In der ISR soll man natürlich direkt auf 
die Variable zugreifen.

[Peter D. (peda)]

Ralf G. schrieb:
> - den Interruptstatus merken

In der Regel weiß man ja, daß man gerade einen Interrupthandler schreibt 
und kann direkt auf die Variable zugreifen, da der AVR ohne dirty Tricks 
keine nested Interrupts kann.

Ich hab das ATOMIC_RESTORESTATE aber einmal gebraucht, da Code erst im 
Init vor der Interruptfreigabe und dann im Main aufgerufen wurde.

[Ralf G. (ralg)]

Ralf G. schrieb:
> ... würde, hätte ...

Stefan U. schrieb:
> ... natürlich ...

Es steht eben an der Funktion nicht dran, das die für ISRs strengstens 
verboten ist! (Abgesehen davon, dass Funktionsaufrufe in einer ISR 
eher 'nicht so optimal' sind.)

[Peter D. (peda)]

Stefan U. schrieb:
> Wieso besser?

Es erschlägt das volatile gleich mit.
Es läßt sich leichter auf andere CPUs portieren.

[Ralf G. (ralg)]

Peter D. schrieb:
> In der Regel weiß man ja, ...
Ja. Aber man muss wissen warum.

Stefan U. schrieb:
> Wieso besser?
Es geht nicht darum, mal einfach [ aus Spaß ;) ] cli() und sei() 
aufzurufen. Es soll ein atomarer Zugriff erfolgen. Und da helfen die 
Makros aus der <util/atomic.h>, das ordentlich zu dokumentieren!

[dunno.. (Gast)]

Ralf G. schrieb:
> Die korrekte Herangehensweise ist:
> - den Interruptstatus merken
> - Interrupts abschalten
> - de vorherigen Status wieder herstellen
>
> Dafür gibt's (mit etwas Schreibarbeit) Makros in der <util/atomic.h>.

Die lib kannte ich nicht.. Aber natürlich hast du vollkommen recht

Ich programmiere schon länger nur noch Ärmchen, da mache ich das 
tatsächlich auch immer genau so.

Vor allem wenn man irgendwelche Treiber für Peripheriezugriffe auch in 
ISRs nutzen muss, und evtl. übergeordnete Funktionen daher auch 
Interrupts ein/ausschalten müssen ists natürlich fatal, einfach immer 
sei(); zu nutzen.. Allerdings würde ich bei sowas dann lieber nur die 
tatsächlich gefährdeten Interrupts sperren..

[Peter D. (peda)]

dunno.. schrieb:
> Allerdings würde ich bei sowas dann lieber nur die
> tatsächlich gefährdeten Interrupts sperren..

Würde ich gerade nicht.
Die ARM können ja Interruptlevel. Würde man nur einen Interrupt sperren, 
könnte sich ein langsamer und unwichtiger Interrupt vordrängeln und so 
den gesperrten zu lange verzögern.
Nur die globale Sperre sichert, daß es nicht zu einer 
Prioritätsinversion kommt. Sie verkürzt außerdem die Sperre auf die 
minimal mögliche Dauer.

[(prx) A. K. (prx)]

Peter D. schrieb:
> Nur die globale Sperre sichert, daß es nicht zu einer
> Prioritätsinversion kommt. Sie verkürzt außerdem die Sperre auf die
> minimal mögliche Dauer.

Zumindest bei den Cortexen empfiehlt sich eine dritte Variante, die 
höher priorisierte Interrupts nicht behindert.

Die haben mit dem BASEPRI/BASEPRI_MAX Register eine sehr einfache 
Möglichkeit, den aktuellen Level auf den des betreffenden Interrupts zu 
setzen. Dann sind höher priorisierte Interrupts zulässig, alle anderen 
gesperrt.

Also sinngemäss:
  unsigned savedPRIO = BASEPRI;
  BASEPRI_MAX = meinePRIO;
  ... der betroffene Code ...
  BASEPRI = savedPRIO;
Dank spezieller Eigenschaften des BASEPRI_MAX Registers darf das auch 
verschachtelt werden, d.h. das funktioniert selbst dann, wenn im 
geschützten Code in einer aufgerufenen Funktion diese Sequenz mit einem 
anderen Level auftritt. Die Sequenz ist auch in ISRs zulässig.

[Peter D. (peda)]

A. K. schrieb:
> Dann sind
> höher priorisierte Interrupts zulässig, alle anderen gesperrt.

Ich wüßte nicht, was das für einen Vorteil hätte. Es würde aber das 
Atomic Macro unnötig verkomplizieren.

Das BASEPRIE ist allerdings eine elegante Möglichkeit, das Atomic Macro 
zu realisieren.
Soweit ich mich erinnere, ist das globale Enable Flag privilegiert, d.h. 
ein Zugriff darauf mit erheblichem Aufwand verbunden.

[(prx) A. K. (prx)]

Peter D. schrieb:
> Ich wüßte nicht, was das für einen Vorteil hätte.

Eigentlich denjenigen, den du selbst genannt hast: Wenn man alle 
Interrupts abschaltet, dann sind auch höher priorisierte Interrupts für 
diese Dauer gesperrt, d.h. werden entsprechend verzögert. Mit BASEPRI 
hingegen kommen die unverändert durch, d.h. werden in keinster Weise 
verzögert.

[Erleuchteter (Gast)]

Falk B. schrieb:
> Solche Cast sind immer ein klares Zeichen von Mur

Klar, der AVR-GCC ist ja auch Murks

[Peter D. (peda)]

A. K. schrieb:
> Wenn man alle
> Interrupts abschaltet, dann sind auch höher priorisierte Interrupts für
> diese Dauer gesperrt, d.h. werden entsprechend verzögert.

Ich nehme Atomic aber nicht dafür, ganze Unterfunktionen mit komplexen 
Berechnungen damit zu kapseln, sondern nur, um Daten konsistent zu 
halten, also einige LD oder ST Operationen lang. Da halte ich eine 
Unterbrechbarkeit für unnötig. Schon ein Interrupt-Prolog/-Epilog 
dürften länger brauchen, als nur ein paar LD/ST.