AVR-spezifische Optimierung wirkt nicht in Member-Funktion

[ffonsel (Gast)]

Hallo,

Ich verwende avr-g++ 4.3.5
ich habe eine Klasse, die einen Port abstrahieren soll.
Die Klasse hat ein Attribut

volatile uint8_t* const port_address

und eine Funktion

inline bool Port::get_pin(uint8_t pin_number) const
{
    bool result = *port_address_ & (1 << pin_number);

    return result;
}

Die Funktion ist in einem Header-File

Ich würde erwarten, dass hier mit -Os optimiert wird zu

sbis [port_address], [pin_number]

stattdessen bekomme ich (für pin_number = 4)

in r24,55-32
lsr r24
lsr r24
lsr r24
sbrc r24,0

Es scheint also, dass der Compiler korrekt inferiert, aber nicht die 
AVR-spezifische Optimierung anwendet.
Kompiliere ich die Funktion außerhalb einer Klasse, also z.B. mit einer 
globalen port-Variable und rufe diese Funktion dann mit pin_number als 
Parameter auf, funktioniert alles wie erwartet.

Wie kann ich nun den Compiler dazu bringen, die Optimierung anzuwenden?

tschüss

[Rolf Magnus (Gast)]

Wie rufst du die Funktion denn auf?

[ffonsel (Gast)]

Ich habe mit einer normalen Variable, mit einer const-Variable und mit 
einer Zahl/Immediate (das dürfte ja das gleiche wie eine konstante 
Variable als Parameter sein.)

Ich möchte die Funktion möglichst so halten, dass ich eine Konstante 
übergeben kann, also z.B.

 MyPort.get_pin(5)

 als auch dynamisch, also zum Beispiel in einer for-Schleife aufrufen. 
Der Compiler hat zu entscheiden, was passiert. Der statische Fall sollte 
aber natürlich nicht langsamer sein, als

 if ( PORT.x & (1 << pin_number))

. Klar das ist eigentlich nichts anders. Aber in einem anderen Kontext 
übersetzt der Compiler das in ein sbis mit nachfolgendem conditional 
branch, hier aber komischerweise nicht. Ich meine zwei shifts nach 
links, ein logisches AND und ein test auf null sind genau das gleiche, 
aber ein sbis + branch sind natürlich weniger Instruktionen. Ich frage 
mich also, warum optimiert der Compiler hier nicht bis zum Ende?

Ich habe auch mal zum Test den Parameter für get_bit als const 
deklariert; das sollte ja auf jeden Fall abhelfen, aber falsch gedacht.

[holger (Gast)]

Versuchs doch mal so:

get_pin(1 << 4)


bool result = *port_address_ & pin_number;

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

Rolfs Frage gilt immer noch. Wenn der Compiler die Funktion so aufrufen 
muss, dass sie zwingend einen Wert 0/1 liefert, dann ist es Essig mit 
der von dir gewünschten Optimierung. Deshalb ebenso wichtig, wie die 
Funktion aufgerufen wird, wie deren Inhalt.

Sinnvollerweise baust du ein minimiertes(!) vollständiges Testbeispiel, 
dass idealerweise ohne zusätzliche Files compilierbar ist.

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

Minimiertes Gegenbeispiel, quick-and-dirty:

struct Port {
  Port (unsigned addr) : port((unsigned char *)addr) {}
  bool test(int bit) { return (*port & (1 << bit)) != 0; }
  volatile unsigned char *port;
};

extern void g();

void f()
{
  if (Port(55).test(4))
    g();
}

erzeugt mit avr-gcc 4.3.4

  sbic 55-32,4
  rcall _Z1gv

NB: Dass deine Funktion nicht 0/1 zurück gibt, sondern irgendwas 
ungleich 0, das könnte sich irgendwann rächen.

[ffonsel (Gast)]

Ich verstehe nicht, was damit gemeint ist

A. K. schrieb:
> Wenn der Compiler die Funktion so aufrufen
> muss, dass sie zwingend einen Wert 0/1 liefert

Meinst du damit, dass der Compiler nichts optimiert, sondern quasi den 
Rückgabewert in r24 lässt (siehe assembly unten) und dann der Prolog und 
alles durch das Inlining wegfällt?

Hier mein Code (ich habe den andern Krempel einfach mal weggelassen):

http://pastebin.com/ucNJfnw4

Diese Beispiel kompiliert zu:

main:
        in r24,55-32
        lsr r24
        lsr r24
        lsr r24
        sbrc r24,0
        rjmp .L2
        cbi 55-32,2
.L3:
.L5:
        rjmp .L5
.L2:
        sbi 55-32,2
        rjmp .L3

Dieses Beispiel hingegen:

#include <avr/io.h>

volatile uint8_t* const port = &PORTB;

inline bool func(uint8_t x)
{
    return *port & (1 << x);
}

int main(void)
{
    uint8_t x = 4;
    if (func(x))
    {
        *port = 0xFF;
    }

    while (true);
    return 0;
}

Kompiliert zu:

main:
        sbis 56-32,4
        rjmp .L2
        ldi r24,lo8(-1)
        out 56-32,r24
.L2:
.L4:
        rjmp .L4

Ich verstehe den Unterschied nicht. In beiden Fällen ist das 
PORT-register durch einen const pointer auf volatile uint8_t gegeben und 
der pin wird als (const) parameter übergeben.
Wo ist nun die Grenze für die Optimierung? Die Inline Funktionen sind 
übrigens in den *_impl.h Header-Dateien definiert, also sollte es keine 
"Übersetzungseinheitsgrenzen" geben

holger schrieb:
> Versuchs doch mal so:
>
> get_pin(1 << 4)
>
>
> bool result = *port_address_ & pin_number;

Das liefert mir tatsächlich ein sbic.
Wodran liegt das? In dem Beispiel das ich in diesem Beitrag gegeben 
habe, wird ja auch nur die Pinnummer übergeben und es wird zu einem 
sbis.

häh?

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

Könnte sein, dass der interessante Teil genau der ist:
    // hier wird geschaut, ob der Button aktiviert ist,
    // oder sich der Zustand des Buttons geändert hat...
Weil der Port volatile ist muss der Zugriff darauf davor erfolgen. Da 
bleibt dem Compiler meist nichts übrig, als sich den Stand in einem 
Register zu merken. Das Flag-Register hält nicht lang vor.

[ffonsel (Gast)]

A. K. schrieb im Beitrag #2228283:
> Wie bitteschön soll der Compiler beiinline void Button::Poll()
> ein sbic/sbis produzieren, wo darin ausdrücklich der Portzustand mit
> einer Variablen verglichen wird?

active_high_ ist als const bool deklariert, damit sollte es den 
Unterschied zwischen sbic und sbis machen, sonst nichts.

Aber wie gesagt, wenn ich

inline void Button::Poll()
{
    bool new_state = (port_->get_pin((1 << 3)) == active_high_);
    has_changed_ = (new_state != active_);
    active_ = new_state;
    return;
}

einsetze, (parameter geändert) und get_pin anpasse:

inline bool Port::get_pin(uint8_t pin_number) const
{
    bool result = *port_address_ & (pin_number);

    return result;
}

,dann bekomme ich mein sbic.

Wo liegt nun der unterschied, ob ich (1 << 3) übergebe, oder 1 und dann 
in der inline-Funktion (1 << pin_number) mache?

Nur die Pin-Nummer zu übergeben ist natürlich mein Ziel, da es logischer 
und bequemer ist.

[ffonsel (Gast)]

A. K. schrieb:
> Könnte sein, dass der interessante Teil genau der ist:
>     // hier wird geschaut, ob der Button aktiviert ist,
>     // oder sich der Zustand des Buttons geändert hat...
> Weil der Port volatile ist muss der Zugriff darauf davor erfolgen. Da
> bleibt dem Compiler meist nichts übrig, als sich den Stand in einem
> Register zu merken. Das Flag-Register hält nicht lang vor.

Dieser Teil sieht so aus:

    bool active = ButtonB2.is_active();
    bool has_changed = ButtonB2.has_changed();
    bool activated = ButtonB2.activated();
    bool deactivated = ButtonB2.deactivated();

    bool x = (active && has_changed && activated);

    PortB.change_pin(2, x);

einfach irgendein Schwachsinn, damit nichts wegoptimiert wird.

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

Da hast du doch die Antwort. Die diversen Port-Tests lassen sich anders 
als sonstige Dinge nicht umordnen und dorthin verzögern wo wirklich 
benötigt, weil volatile. Daher finden die in der gegebenen Reihenfolge 
an der Stelle statt, wo sie im Quelltext stehen, und landen somit 
zwangsläufig als Wert in Registern.

Bei if(func(...)) entsteht das Problem nicht, weil die Verwendung des 
Ergebnisses direkt auf den Test folgt.

PS: Bei den const propagations hast du recht, daher hatte ich den Kram 
bereits gelöscht.

[ffonsel (Gast)]

A. K. schrieb:
> Da hast du doch die Antwort. Die diversen Port-Tests lassen sich anders
> als sonstige Dinge nicht verzögern, weil volatile. Daher finden die in
> der gegebenen Reihenfolge statt und landen somit zwangsläufig als Wert
> in Registern.

Also, übergebe ich (1<<3) bekomme ich das sbic, übergebe nur die Drei 
(als inhalt einer konstanten Variable, erhalte ich folgendes assembly:

        in r24,55-32
        lsr r24
        lsr r24
        lsr r24   
        sbrc r24,0      
        rjmp .L2
        cbi 55-32,2
.L3:
.L5:
        rjmp .L5        ;bla bla und so weiter
.L2:
        sbi 55-32,2
        rjmp .L3

Drei mal left shift und dann wird gebrancht wenn der Inhalt vom Bit ganz 
rechts gleich 0 ist. Das kann doch ohne Probleme zu einem sbis r24, 3 
optimiert werden?

> PS: Bei den const propagations hast du recht, daher hatte ich den Kram
> bereits gelöscht.
Habe, ich dann auch gesehen, ist ja kein Ding.

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

> Drei mal left shift und dann wird gebrancht wenn der Inhalt vom Bit ganz
> rechts gleich 0 ist. Das kann doch ohne Probleme zu einem sbis r24, 3
> optimiert werden?

Kann. Aber ich habe bisher keinen Quellcode gesehen, der einen Pin 
ändert. NB: lsr schiebt rechts.

Wenn du dauernd unvollständige Codefragmente vorsetzt, dann artet das in 
Glaskugelspiele aus. Hier fehlt jetzt vermutlich change_pin.

Bring mal Code, der compilierbar ist (linkfähig muss er nicht sein), 
möglichst alles in einem File, keinerlei #includes (auch da ich grad 
keine Standardincludes parat habe). Und lass alles weg was unnötig.

[ffonsel (Gast)]

A. K. schrieb:
> NB: lsr schiebt rechts.

Richtig, ich kann das nicht auseinanderhalten.

A. K. schrieb:
> Aber ich habe bisher keinen Quellcode gesehen, der einen Pin
> ändert.

Das soll ja auch extern geschehen.

A. K. schrieb:
> Bring mal Code, der compilierbar ist (linkfähig muss er nicht sein),
> möglichst alles in einem File, keinerlei #includes. Und lass alles weg
> was unnötig.

Hier:

http://pastebin.com/rvxj8piZ

das Ergebnis ist bei mir (avr-g++ -S -Os -mmcu=atmega8 test.cc):

  .file  "test.cc"
__SREG__ = 0x3f
__SP_H__ = 0x3e
__SP_L__ = 0x3d
__CCP__  = 0x34
__tmp_reg__ = 0
__zero_reg__ = 1
  .text
.global  main
  .type  main, @function
main:
/* prologue: function */
/* frame size = 0 */
  in r24,55-32
  lsr r24
  lsr r24
  lsr r24
  sbrs r24,0
.L3:
.L5:
  rjmp .L5
.L2:
  sbi 55-32,2
  rjmp .L3
  .size  main, .-main

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

Bestätigt. Maske geht, Bit nicht.

Tja, da kann ich dich nur dafür beglückwünschen, dass es dir gelungen 
ist, die Optimierung vom GCC auszuhebeln. Nobody is perfect, auch der 
nicht.

[holger (Gast)]

>Wo liegt nun der unterschied, ob ich (1 << 3) übergebe, oder 1 und dann
>in der inline-Funktion (1 << pin_number) mache?

Du hast einen variablen Shift, und das ist immer Mist.

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

Der ist nicht variabel. Der erzeugte Code zeigt das ja auch.

[holger (Gast)]

>Der ist nicht variabel. Der erzeugte Code zeigt das ja auch.

Ja, für die 3. Gibt es auch ein Beispiel für 4 oder 5
zusätzlich im Code?

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

Mit nicht-konstanter Bitnummer ist das trivialerweise nicht optimierbar, 
aber darum geht es hier nicht. Wärs bei if(PINA & (1<<n)) auch nicht.

[ffonsel (Gast)]

holger schrieb:
> Ja, für die 3. Gibt es auch ein Beispiel für 4 oder 5
> zusätzlich im Code?

bis 3 wird lsr angewendet, dadrüber gibt es zuerst einen swap gefolgt 
von lsr.
swap vertauscht die nibble.

holger schrieb:
> Du hast einen variablen Shift, und das ist immer Mist.

Der Shift soll ja im Quelltext möglichst Variabel sein, so dass man die 
Möglichkeit hat Pins variabel zu setzen (also beispielsweise, abhängig 
von einer bestimmten Eingabe), oder eben statisch. Je nachdem was man 
möchte hat man damit das gleiche Interface, aber jeweils das richtige 
(statisch oder variabel) Resultat. Es gibt natürlich noch ca. 20 andere 
Funktionen um mehrer Pins auf einmal zu setzen, oder dem Port-Register 
einen anderen Zustand zuzuweisen, usw.

[Johann L. (gjlayde)]

Möglicherweise einer von

http://gcc.gnu.org/PR42210 (fixed in 4.7.0)
http://gcc.gnu.org/PR33049 (Patch verfügbar, nicht upstream)

[Johann L. (gjlayde)]

Ja, ist PR33049.

Wer besseren Code sehen will, muss sich also bis zu avr-gcc 4.7 
gedulden. Leider sind solche Patches nicht gern gesehen, bisher ist es 
noch nicht in GCC integriert. Gegebenenfalls wäre die erzeugte 
Instruktionsfolge

  bst r24, 3
  clr r24
  bld r24, 0

Oder das Pflasterchen [1] selbst einbauen; es funktioniert auch mit 
älteren Versionen von avr-gcc.

[1] http://gcc.gnu.org/bugzilla/attachment.cgi?id=24277&action=diff

[ffonsel (Gast)]

Vielen Dank!

[Johann L. (gjlayde)]

Da du avr-gcc offenbar selbst generierst:

1. Ist der Code mit dem Patch wie erwartet?

2. configure im Quellverzeichnis oder einem Unterverzeichnis
   davon wird nicht unterstützt. Rechne mit allen Sorten von
   seltsamen Effekten:
   http://gcc.gnu.org/install/configure.html

3. Ein Bugreport ohne Quellen ist nicht hilfreich, da niemand
   das Problem nachvollziehen kann — allein aus dem Bugreport
   noch nichtmal du selbst:
   http://gcc.gnu.org/bugs/#need

[ffonsel (Gast)]

Ich hatte nochmal etwas freie Zeit.

Johann L. schrieb:
> 1. Ist der Code mit dem Patch wie erwartet?

Nein, genauso wie vorher. Allerdings habe ich gcc-4.6.0 kompiliert. Da 
scheinen im avr.md file ca. 200 Zeilen zu fehlen. Ich werde 4.7 bei 
Gelegenheit nochmal testen.

Johann L. schrieb:
> 3. Ein Bugreport ohne Quellen ist nicht hilfreich, da niemand
>    das Problem nachvollziehen kann — allein aus dem Bugreport
>    noch nichtmal du selbst:
>    http://gcc.gnu.org/bugs/#need

Danke, kenne ich schon.

[Johann L. (gjlayde)]

ffonsel schrieb:
> Ich hatte nochmal etwas freie Zeit.
>
> Johann L. schrieb:
>> 1. Ist der Code mit dem Patch wie erwartet?
>
> Nein, genauso wie vorher. Allerdings habe ich gcc-4.6.0 kompiliert.

Klar da ist die Äanderung ja auch nicht drinne. In 4.6 müsstest du die 
paar Zeilen von Hand hinzufügen.

[ffonsel (Gast)]

Johann L. schrieb:
> Klar da ist die Äanderung ja auch nicht drinne. In 4.6 müsstest du die
> paar Zeilen von Hand hinzufügen.


Ich habe schon nicht vergessen zu patchen ;)

[Johann L. (gjlayde)]

ffonsel schrieb:
> Johann L. schrieb:
>> Klar da ist die Äanderung ja auch nicht drinne. In 4.6 müsstest du die
>> paar Zeilen von Hand hinzufügen.
>
> Ich habe schon nicht vergessen zu patchen ;)

Auch neu generiert und installiert? *wegduck*

hmmm. Dann würd ich mir das nochmal anschauen falls das ii-File am 
Bugreport hinge.

[ffonsel (Gast)]

Ich habe gerade den HEAD von heute kompiliert, mit und ohne Patch keine 
Änderung.

Ich fühle mich nicht danach, den bug zu filen, aber falls das jemand 
machen möchte sind hier alle nötigen Information

test.ii (intermediate)  http://pastebin.com/ZPV521si
test.s (ausgabe)   http://pastebin.com/DDFW0FCU

compiler-output (verbose) http://pastebin.com/XHNPB6WX

[Johann L. (gjlayde)]

ffonsel schrieb:

> Ich fühle mich nicht danach, den bug zu filen...

Da:

http://gcc.gnu.org/PR49446

[ffonsel (Gast)]

Oh, guter Detektiv, ich habe es nochmal rangehangen.

[Johann L. (gjlayde)]

Bedenke, daß ein vernünftiger Fehlerreport mit allen notwendigen 
Informationen im Report notwendig ist, so daß überhaupt eine Arbeit an 
dem Problem möglich ist.

GCC-Entwickler haben idR keine Lust und Zeit, jemand die notwendigen 
Infos aus der Nase zu ziehen.

[ffonsel (Gast)]

Johann L. schrieb:
> Bedenke, daß ein vernünftiger Fehlerreport mit allen notwendigen
> Informationen im Report notwendig ist, so daß überhaupt eine Arbeit an
> dem Problem möglich ist.
>
> GCC-Entwickler haben idR keine Lust und Zeit, jemand die notwendigen
> Infos aus der Nase zu ziehen.

Eigentlich hatte ich beim ersten Eintrag gedacht ich hätte alles 
rangehangen, aber irgendwo hat der File upload nicht geklappt, war nicht 
meine Absicht. Aber dann bei mir ist die frustgrenze dann meistens auch 
schnell erreicht, weil ich gar nicht im GCC irgendwo rumwühlen will und 
zwanzig Varianten ausprobieren, sondern ich will einfach nur "fertig 
werden".. anyway jetzt ist es da.

[Johann L. (gjlayde)]

http://gcc.gnu.org/PR33049 ist gefixt in 4.7.0.

Für einen Backport brauchst du allerdings nicht nur das Patch am PR (SVN 
175269), sondern auch die Prädikate "const1_operand" und 
"const_0_to_7_operand" aus ./gcc/config/avr/predicated.md. Die gibt's in 
älteren Versionen noch nicht:

http://gcc.gnu.org/viewcvs/trunk/gcc/config/avr/predicates.md?content-type=text%2Fplain&view=co

Viel Spaß.