GCC-Bug umschiffen

[DerAlbi]

Hallo Leute,
ich entwickle aufm ARM mit dem GCC und der hat nen absoluten Horror-Optimierungsbug für volatile-Lesezugriffe von Datentypen kleiner der nativen Bitanzahl.
Sprich: alles was kleiner ist als volatile unsigned int dauert doppelt so lange im Zugriff.

Szenario: ich toiggle ein Bit in einem Memory mapped Register:

Code: Alles auswählen

GPIOA->ODR ^= 1<<Pin;

wobei GPIOA->ODR ein volatile unsigned short ist.
Beim Lesezugriff geschieht nun folgendes:

Code: Alles auswählen

Laden eines 16bit Wertes
zero-extent 16 bit auf 32 bit weil Register nunmal 32bit groß sind
XOR (auf die unteren 16bit btw)
Speichern des 16 bit wertes.

Das Problerm: beim Laden eines 16Bit Wertes wird es schon implizit auf 32Bit zero-extended, sodass das zusätzliche explizite zero-extenten komplett redundant ist. GCC bug-Menschen sagen, dass ist eine fehlende Optimierung bei volatile-Variablen (wohl weil viele Leute denken, das volatile bedeutet, dass man nicht optimieren darf :-/ )

Mich interessiert an diesem Punkt der workaround:

Code: Alles auswählen

GPIOA->ODR = (*const_cast<unsigned short*>(&GPIOA->ODR)) ^ (1u<<Pin);

Das lässt das volatile beim Lesezugriff verschwinden... für meinen Testfall erzeugt der GCC immernoch den richtigen (und endlich optimalen) Code (mit 50% mehr Perfoamnce), aber wohl ist mir im Magen dabei nicht, weil der Lesezugriff jetzt eigentlich frei optimierbar ist und keinerlei Barriere mehr darstellt.
Also das volatile wieder hinmachen:

Code: Alles auswählen

GPIOA->ODR = ((volatile unsigned int)*const_cast<unsigned short*>(&GPIOA->ODR)) ^ (1u<<Pin);

So. Zumindest ich bin zufrieden. Der erzeugte Code ist optimal und immernoch volatile. Aber aaaalter so viel Aufwand.. WTF.

Frage an die Experten:
a) ist das volatile jetzt immernoch volatile? (oder mach ich das durch weg- und wieder hin casten irgendwie immernoch kaputt?)
b) geht das nich (abgesehen von Makros) einfacher -.- Das ist doch ein Witz.

Irgendwie will ich grade den Lade-Operator für unsigned short und unsigned char überladen... wenn es sowas gäbe :lol:


Derzeitiges Makro:

Code: Alles auswählen

#define IO_READ(x)  ( static_cast<decltype(x)>(*const_cast< std::remove_volatile<decltype(x)>::type*>(&(x)))  )

Es stinkt. Und ich will nicht immer dieses IO_READ schreiben müssen.

[Jörg]

Hallo,

a) könntest Du im Groben testen - schau einfach ob die Optimierung einer doppelten Zuweisung vor und nach Deinem bit-flip greift oder unterdrueckt wird.
b) Habe hier einen 5.4er GCC fuer ARM, und der erzeugt ein anderes, ebenfalls sub-optimales Codemuster sobald volatile ins Spiel kommt:

Code: Alles auswählen

Laden 16 bit
XOR 32 bit
SHL 16
SHR 16
Schreiben 16 bit

Ich würde ja auf den inline-Assembler ausweichen, da gibt es dann wenigstens keine versteckten Tretminen mehr. Etwa so:

Code: Alles auswählen

void toggle(volatile unsigned short int *reg, unsigned int pin)
{
unsigned int mask = 1u;
unsigned int tmp;
asm volatile ("ldrh    %[tmp], [%[addr]]\n"
              "eor     %[tmp], %[tmp], %[mask], lsl %[pin]\n"
              "strh    %[tmp], [%[addr]]":[tmp]"=&r"(tmp): [mask]"r"(mask), [pin]"r"(pin), [addr]"r"(reg): "memory");
}

Ist natuerlich immer noch Schreibarbeit fuer jeden Zugriff, und der globale memory clobber macht u.U. mehr kaputt als er hilft da alle temporären Werte aus den Registern gesichert werden.
Falls letzteres ein Problem darstellt - einzelne Wrapper fuer Lesen und Schreiben basteln. Sowas macht sich spätestens dann bezahlt, wenn Du mal was umleiten musst (z.b. alle HW Zugriffe in eine Textdatei).

Joerg

[xq]

Ich habe eine Idee, woran das Zero-Extend liegen kann und wie man es umschiffen könnte, ohne so komische Workarounds zu machen:

Code: Alles auswählen

GPIOA->ODR ^= ((uint16_t)1) << ((uint16_t)Pin);

Wenn Pin nämlich uint32_t oder int oder ähnliches ist, bekommst du bei typeof(1 << Pin) = uint32_t, daraus folgt, dass dein Snippet eigentlich das hier ist:

Code: Alles auswählen

uint16_t val = GPIOA->ODR;
typename Pin tmp = val ^ (1 << Pin);
GPIOA->ODR = (uint16_t)tmp;

Damit bekommst du einen impliziten Doppelcast rein.

Grüße
Felix

[DerAlbi]

Hallo Jörg,
ich habe auch schon an inline asm gedacht, aber wie du schon sagtest: sowas macht dummerweiße oft mehr kaputt als es hilft, zumal, wenn es einen HighLevel-Workaround gibt.
Dein asm-Output b) ist ja noch schlimmer als die jetzige Version :-D. Das kommt aber vermutlich auf das Instruction-Set an.
Was dein a) angeht, weiß ich im moment grade nicht, was du konkret meinst. Ich soll mehrmals bits flippen? Du meinst, der Comiler würde normalerweiße erkennen, das "a ^= 1; a ^= 1;" das originale "a" ist, sodass beide Instruktionen wegfallen?
Das geschieht zumindest in meinem Testfall nicht. Bedeuted das aber nun wirklich, dass mein Workaround für immer klappt ?

MasterQ32:
soweit ich weiß wird absolut jede Operation auf Grundtypen auf mindestens der Architekturbreite ausgeführt, weil so breit nunmal die Register sind. Egal, was du machst, es wird also immer und grundsätzlich in 32bit gerechnet [bei meinem ARM].
Es ist Aufgabe der Optimierung diese Unnötigkeit wieder auf die wirklich benutzte Bitbreite runter zu schrumpfen. Und genau diese Optimierung klappt in meinem Falle aufgrund des volatiles nicht.
Auch ein

Code: Alles auswählen

GPIOA->ODR ^= ((uint16_t)1) << ((uint16_t)Pin);

wird den 16Bit-Wert erstmal in ein 32bit Register laden, dann wird mit 32bit ver-XOR-t (egal, auch wenns nur 16bit sind..) und dann die unteren 16 bit des Ergebnises zurück schreiben.
Diese Variante habe ich quasi als erstes versucht :-) Zumindest habe ich es mit "(uint16_t)(1u<<Pin)" probiert. Sollte das gleiche sein.

[dot]

Mich interessiert an diesem Punkt der workaround:

Code: Alles auswählen

GPIOA->ODR = (*const_cast<unsigned short*>(&GPIOA->ODR)) ^ (1u<<Pin);

Das lässt das volatile beim Lesezugriff verschwinden... für meinen Testfall erzeugt der GCC immernoch den richtigen (und endlich optimalen) Code (mit 50% mehr Perfoamnce), aber wohl ist mir im Magen dabei nicht, weil der Lesezugriff jetzt eigentlich frei optimierbar ist und keinerlei Barriere mehr darstellt.

Das ist definitiv undefiniertes Verhalten, du referenzierst ein als volatile deklariertes Objekt durch ein non-volatile lvalue. Abgesehen davon: volatile stellt keine "Barriere" in irgendeinem Sinne dar; auch wenn es in manchen anderen Sprachen diesen Zweck erfüllen mag (und MSVC diese zusätzliche Semantik garantiert): In Standard C++ hat volatile nichts mit Synchronization oder sonstwas zu tun. Insbesondere können volatile Loads/Stores relativ zu anderen, independent Loads/Stores reordered werden. Und nebenläufiger Zugriff auf non-atomic volatile Objekte ist genauso undefiniertes Verhalten, als es auch wäre, wenn da kein volatile stünde...

Also das volatile wieder hinmachen:

Code: Alles auswählen

GPIOA->ODR = ((volatile unsigned int)*const_cast<unsigned short*>(&GPIOA->ODR)) ^ (1u<<Pin);

So. Zumindest ich bin zufrieden. Der erzeugte Code ist optimal und immernoch volatile.

Das ist extrem borderline. Ich bin mir im Moment nicht 100% sicher ob das UB ist oder nicht, würde aber dazu neigen, es als UB zu interpretieren. Problem ist, dass du dereferenzierst bevor das volatile wieder dran ist. Der nachfolgende Cast sollte imo nur einen Temporary erzeugen, der mit dem wieder über ein non-volatile lvalue geladenen Wert von GPIOA->ODR initialisiert wird...

Es stinkt. Und ich will nicht immer dieses IO_READ schreiben müssen.

Eigentlich willst du aber nur einzelne Bits in diesem Register togglen oder? Und: Wieso muss es ausgerechnet ein Makro sein?

Code: Alles auswählen

template <unsigned int MASK>
void toggleBits(volatile unsigned short& reg)
{
  reg = static_cast<volatile unsigned short&>(*const_cast<unsigned short*>(&reg)) ^ MASK;
}

template <unsigned int i>
void toggleBit(volatile unsigned short& reg)
{
  toggleBits<(1U << i)>(reg);
}

toggleBit<2>(GPIOA->ODR);

Es ist Aufgabe der Optimierung diese Unnötigkeit wieder auf die wirklich benutzte Bitbreite runter zu schrumpfen. Und genau diese Optimierung klappt in meinem Falle aufgrund des volatiles nicht.
Auch ein

Code: Alles auswählen

GPIOA->ODR ^= ((uint16_t)1) << ((uint16_t)Pin);

wird den 16Bit-Wert erstmal in ein 32bit Register laden, dann mit 32bit ver-XOR-en und dann die unteren 16 bit des Ergebnises zurück schreiben.
Diese Variante habe ich quasi als erstes versucht :-)

Rein prinzipiell: Nur weil etwas mehr Instructions erzeugt ist es nicht unbedingt weniger effizient. Ich kenn mich mit ARM nicht aus, aber beispielsweise auf x86 wäre die Variante die volle 32-Bit Register benutzt vermutlich zu bevorzugen, da Partial-Register-Access beispielsweise potentiellen Instruction-Level-Parallelism ungenutzt lassen kann. Der einzige Weg, um festzustellen, dass etwas langsamer ist, ist, nachzumessen. Nur den Assemblercode anschauen allein ist dafür im Allgemeinen nicht ausreichend...


Edit: Ich würde es wenn dann so machen:

Code: Alles auswählen

GPIOA->ODR = static_cast<volatile unsigned short&>(*const_cast<unsigned short*>(&GPIOA->ODR)) ^ (1u<<Pin);

das ist zwar immer noch extrem borderline, sollte imo aber sogar potentiell OK sein (der erzeugte Temporary ist selbst eine lvalue Reference, die — mit einer lvalue Reference initialisiert — in dem Fall an das ursprüngliche lvalue gebunden werden sollte), die Frage ist, ob es noch den gewünschten Code erzeugt...

[DerAlbi]

Also ich habe deine Version gerade geprüft.. und mit der Referenz wird dummerweise die unnötige Instruktion wieder eingefügt.

Code: Alles auswählen

#define IO_READ(x)  ( static_cast<decltype(x) >( *const_cast< typename std::remove_volatile<decltype(x)>::type* >(&(x)) ))  // Optimal
#define IO_READ(x)  ( static_cast<decltype(x)&>( *const_cast< typename std::remove_volatile<decltype(x)>::type* >(&(x)) ))  // mit sinnloser Zero-Extension

Und ja, die zusätzliche Instruktion führt zu einem deutlichen Verlust. (gemessen)

Insbesondere können volatile Loads/Stores relativ zu anderen, independent Loads/Stores reordered werden.

Das ist auch ok so. Nur voltaile und volatile darf nicht in der reihenfolge Vermischt werden.. und das ist auch das Wichtige hinsichtlich Memory-Mapped-IO.
Ich weiß zur Zeit gar nicht, was ich mit der obigen Zeile eigentlich mache. Wenn es so ist, wie du sagst dürfte der IOREAD() mit einem anderen IOREAD() nicht in der Reihenfolge vertauscht werden können (wegen der volatile temporary).
Allerdings kann ein anderer Volatile-zugriff zwischen den IO-Zugriff [der sonst volatile wäre] und den Volatile-Cast rutschen? Das wäre ungut.

[dot]

Insbesondere können volatile Loads/Stores relativ zu anderen, independent Loads/Stores reordered werden.

Das ist auch ok so. Nur voltaile und volatile darf nicht in der reihenfolge Vermischt werden.. und das ist auch das Wichtige hinsichtlich Memory-Mapped-IO.

Ja, volatile und volatile sollten nicht reordered werden dürfen (wobei die genaue Semantik von volatile leider so eine Sache ist, bei der selbst die Compilerhersteller sich nicht ganz einig sind).

Ich weiß zur Zeit gar nicht, was ich mit der obigen Zeile eigentlich mache. Wenn es so ist, wie du sagst dürfte der IOREAD() mit einem anderen IOREAD() nicht in der Reihenfolge vertauscht werden können (wegen der volatile temporary).

Ich bin mir mittlerweile ziemlich sicher, dass deine Version leider undefined Behavior sein sollte, eben weil sie ein volatile Objekt über einen non-volatile lvalue referenziert...

[Jörg]

Der Vollständigkeit halber (damit keiner mehr vermuten muss, ob es nicht doch am short-cast liegt, oder der Code nicht vielleicht doch super toll ist) hier beide Varianten heruntergebrochen auf den problematischen volatile-store:

Code: Alles auswählen

void toggle(volatile unsigned short int *reg, unsigned int pin)
{
        unsigned short int mask = 1u << pin;
        unsigned short int v = *reg;
        unsigned short int temp = v ^mask;
        *reg = temp;
}

ergibt mit -O3 und dem GCC 5.4

Code: Alles auswählen

        mov     r2, #1
        ldrh    r3, [r0]
        eor     r1, r3, r2, lsl r1
        lsl     r1, r1, #16             <<<!!!
        lsr     r1, r1, #16             <<<!!!
        strh    r1, [r0]
        bx      lr

Mit einem non-volatile store:

Code: Alles auswählen

        mov     r2, #1
        ldrh    r3, [r0]
        eor     r1, r3, r2, lsl r1
        strh    r1, [r0]
        bx      lr

Ein non-volatile read aendert nichts an der Situation (mit dem GCC 5.4), das Snippet von DerAlbi mit dem Vermerk "Zufrieden" , exemplarisch als

Code: Alles auswählen

void toggle(volatile unsigned short int *p, int pin)
{
*p = *(unsigned short int *)p ^ (unsigned short int)1 << pin;
}

getestet, erzeugt den gleichen nicht optimalen Code.

Bleibt also der fade Beigeschmack dass es möglicherweise Probleme mit Lese-(re)ordering gibt und es mit einem anderen Compiler wieder schlechter wird.
Über den Test kann ich jetzt nicht viel sagen, aber ich nehme an, dass die verwendete Variable a ebenfalls volatile short war - damit ist a ^=1; eine Lese-Schreib Operationen, das Schreiben wird in der richtigen Reihenfolge stattfinden (volatile), und damit zwangsweise das Lesen (program order), es sagt also nicht viel aus darüber wie nur-Lese Zugriffe behandelt werden koennten. Mein Fehler mit dem Testvorschlag - ich dachte es waere ein non-volatiles Schreiben m|

Bleibt eigentlich nur IO_READ, IO_WRITE mit inline assembly zu abstrahieren. Das sollte auch ohne den memory-clobber gehen wenn du alle IO zugriffe abkapselst (ich weiss nicht, ob du nur bei 16 bit Handarbeit anlegst, weil es anders nicht schnell genug war, oder ob dein System nichts anderes (z.B. 32 bit) verwendet).

[DerAlbi]

"zufrieden" wäre aber:

Code: Alles auswählen

void toggle(volatile unsigned short int *p, int pin)
{
*p = ((volatile unsigned short int)(*(unsigned short int *)p)) ^ (unsigned short int)1 << pin;
}

[Jörg]

Korrekt - Copy&Paste Fehler (ich wollte versuchen ob wenigstens Deine "Basisprimitive" ohne das fragwuerde volatile-fuer-temporaeres-Objekt funktioniert, nachdem das exakte Beispiel es nicht tat).

Ein Thema, was ein bisschen untergegangen ist - wie gedenkst Du mit den Speicher-Barrieren umzugehen, die Du auf ARM benoetigst wenn der IO Speicher nicht als "strongly ordered" beschrieben wurde?

[DerAlbi]

ICh weiß nicht im ansatz, wovon du redest :oops: Wozu speicher-Barrieren?

[dot]

Nun, wie gesagt, volatile an sich gibt nicht unbedingt Garantien über die relative Ordnung von Speicherzugriffen ab. Nach meiner Interpretation des Standard müsste ein standardkonformer Compiler zu volatile Loads/Stores auch etwaig notwendige Fences emitten (alles andere würde imho die As-if-rule verletzen), aber diese Auffassung variiert je nachdem, wen man fragt. Ich kenn mich mit ARM nicht aus, aber es ist gut möglich, dass volatile deinen Compiler nur davon abhält, Instructions wegzuoptimieren/zu reorderen. Aufgrund von Dingen wie Load/Store Buffers, Caches, etc. kann es durchaus sein, dass die Effekte von Loads/Stores auf Hardware Ebene dennoch reordered werden. Aber das sind Dinge, die du nur den Manuals deiner Architecture und Compiler entnehmen kannst...

[Jörg]

Der Compiler weiss nicht, ob der Prozessor "hinten raus" (oder aus Sicht von anderen Busteilnehmern) die gleiche Reihenfolge bei Lese oder Schreibzugriffen einhaelt, wie sie im Programmcode verwendet sind.
Das erfordert etwas manuelle Arbeit, sowohl bei Einsatz von Threads unter SMP (mehrere CPUs), als auch bei Geraeteansteuerung.
Fuer letzteres solltest Du:
- Keinen gepufferten Speicher verwenden ("uncached")
- Wissen, ob Du ungepufferten Speicher mit dem Attribut "Device Memory" oder "Strongly Ordered" verwendest

Eine "Strongly ordered" Ansicht (MMU page table setting) garantiert das jeder Speicherzugriff exakt in der Reihenfolge ausgefuehrt (und beendet) wird wie im Programm angegeben (Vorraussetzung natuerlich dass der Compiler nicht optimiert hat, aber dafuer verwendest Du volatile oder inline asm).
Leider ist diese Speicheransicht ziemlich unperformant wenn man mehr als ein memory-mapped Register konfigurieren moechte.
Auf ARM findest Du dann die "Device Memory" Ansicht, welche es der HW erlaubt, einen Schreib-Puffer zu verwenden, allerdings ist es dann Aufgabe des Programms, durch geeignete Befehle eine korrekte Transaktionsreihenfolge sicherzustellen (im richtigen Moment, moeglichst wenig).

Stichwort: DMB (oder CP15 store buffer drain).
http://www.embedded.com/print/4437925
http://infocenter.arm.com/help/index.js ... hcffj.html (ist fuer ARMv6 instrution set, aber vom Prinzip her gueltig fuer alle Nachfolger, klick DIch durch falls Du eine spezielle Architektur brauchst)

[DerAlbi]

Achso, also wenn es da um die CPU geht, die ist bei mir im Moment nicht so komplex, dassda überhaupt Cache drin ist.
Ich hatte so eine CPU aber schon mal... das ist hardwaretechnisch so gelöst, dass viele Speicherbereichen doppelt auf Adressen gemappt sind.. einmal gecacht und einmal "roh". In der Adresse hat sich das letztlich über ein Bit ausgedrückt.
Memory Mapped IO ist generell ungecasht, von daher stellen sich solche Probleme kaum. Wichtig wurde es bei DMA.. da musste man den Cache flushen / invalidaten und dann auf rein ungecashten adressen arbeiten.
Desweiteren können CPUs mehere Peripheriebusse haben. Die laufen unter Umständen auch mit grob verschiedenen Geschwindigkeiten. Aufeinanderfolgende Zugriffe auf einen Bus sind da immer in der Reihenfolge, wie die CPU es fordert, aber wenn man mehere Busse anspricht, kann es da zu Problemen kommen, da der eine schneller reagieren kann als der andere. Für sowas hat der Prozessor dann instruktionen wie "sync" oder so... die muss man aber händisch einfügen - der Compiler ist bei sowas generell dumm.. für den sind das alles nur Speicherzugriffe und die Spezifikation, wie die zu erfolgen haben, hört quasi beim Volatile-Wissen auf.

[Jörg]

Cortex-M0..4 kommen ohne DMB aus. Solange Du nie vor hast, Cortex-A zu bedienen ist es ok.
http://infocenter.arm.com/help/index.js ... GJICF.html
Viel Erfolg!