Combinatorical Loop Found

[Fred (Gast)]

Hallo zusammen,

in einem aktuellen Lattice Diamond Projekt habe ich eine Fehlermeldung, 
die ich nicht ganz zuordnen kann:

  2019991  WARNING - CL137 :"<Pfad>AddressDecoder.vhd":878:15:878:45|Combinational loop found at sAccessViolationWr_1_sqmuxa

Da ich das Signal sAccessViolationWr Signal nur in einem getakteten 
Prozess verwende, ist mir nicht ganz klar, wie diese zustande kommt. 
Nachfolgend habe ich mal den zugehörigen Code vereinfacht dargestellt:

  

P_DspWrite : process (iReset, iSysClk)
begin
    if iReset = '1' then
        sAccessViolationWr  <= '0';
        oRegister_1         <= (others => '0')
        oRegister_2         <= (others => '0')
        oBlock_1            <= (others => '0')
    elsif rising_edge(iSysClk) then
    if sDspCsN = '0' and sDspRwN = '0' then             -- write access
        
            -- access single registers
            case sDspAddrA is
                
                when addr_Register_1    => oRegister_1   <= sDspDataInD;
                when addr_Register_2    => oRegister_2   <= sDspDataInD;
                [...]

                when others =>
                    -- adress not  found
                    sAccessViolationWr  <= '1';
            end case;

            -- access block 
            for i in 0 to BLOCK_SIZE loop
                if sDspAddrA = addr_Block1Start +  i then
                    oBlock_1(i)         <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;

            assert sAccessViolationWr = '0' report "case defaulted! AddressDecoder Write Access" severity failure;

        end if; -- if sDspCsN = '0' and sDspRwN = '0' then
    end if;     -- if rising_edge(iSysClkC)
end process;

Die Case Anweisung weist die Daten einzelnen Registern zu. Hat sie 
keinen Treffer, wird das sAccessViolationWr Flag gesetzt.

Anschließend kommt eine For Loop, die weitere Adressen überprüft. Wenn 
diese einen Treffer landet, soll der gesetzte Fehler aus der Case 
Anweisung überschrieben werden und der bisherige Fehlerstatus 
beibehalten werden.

Ich freue mich, wenn jemand Licht ins Dunkel bringen kann, damit ich die 
Ursache der Warning verstehen kann.

Viele Grüße
  Martin Fette

[Sigi (Gast)]

Fred schrieb:
> Da ich das Signal sAccessViolationWr Signal nur in einem getakteten
> Prozess verwende, ist mir nicht ganz klar, wie diese zustande kommt.

Nein, Du löscht das Signal beim async. Resetten.
Und btw., das Signal wird im getakteten Teil
nur gesetzt, aber niergends gelöscht.

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Fred schrieb:
> Nachfolgend habe ich mal den zugehörigen Code vereinfacht dargestellt: ...
So wie es aussieht, ist der Fehler wieder mal nicht im geposteten Code. 
Oder bringt genau dieser "vereinfachte" Code ebenfalls den Fehler(*)?


(*)Fehler deshalb, weil eine kombinatorische Schleife garantiert Unsinn 
ist und Probleme macht..

[Strubi (Gast)]

Moin,

nur so am Rande, die asynchronen Resets sollte man eher sparsam 
einsetzen, sofern du da nicht den GSR (oder einen andern dedizierten 
Reset) dranhängst. Bei Bus-Decodern würde ich async Rests komplett 
vermeiden. Das Keeper-Konstrukt im getakteten Prozess für das *Wr-Signal 
ist eigentlich auch nicht nötig.
Ansonsten sehe ich da aber die Schleife nicht. Welche Diamond-Version 
hast Du und für welchen Baustein gilt die Synthese?

Gruss,

- Strubi

[Schlumpf (Gast)]

Lothar M. schrieb:
> So wie es aussieht, ist der Fehler wieder mal nicht im geposteten Code.

Sehe ich genauso.

Auch wenn der gepostete Code an sich etwas eigenartig ist, kann ich 
darin keinen Grund für eine kombinatorische Schleife entdecken

[Fred (Gast)]

Hallo zusammen,

erst einmal vielen dank für das Feedback.

Strubi schrieb:
> nur so am Rande, die asynchronen Resets sollte man eher sparsam
einsetzen, sofern du da nicht den GSR (oder einen andern dedizierten
Reset) dranhängst.

Der Reset hängt an einem GSR, wird asynchron aktiviert, aber synchon 
deaktiviert.
Was meinst Du mit Keeper Konstrukt?

>Welche Diamond-Version hast Du und für welchen Baustein gilt die Synthese?
Ich verwende Lattice Diamond 3.5.0 und baue für eine XP2-17.

Lothar M. schrieb:
> (*)Fehler deshalb, weil eine kombinatorische Schleife garantiert Unsinn
ist und Probleme macht..
Deswegen wende ich mich an Euch, das ist mir doch etwas ungeheuer.
Lothar M. schrieb:
> So wie es aussieht, ist der Fehler wieder mal nicht im geposteten Code.

Ich habe den Code noch einmal gebaut und dabei festgestellt, dass mit 
exakt diesem Beispiel die Warning nicht auftritt. Daraufhin habe ich 
meinen Code immer weiter reduziert, um den Fehler einzugrenzen. Dabei 
hat sich herausgesetellt, dass ich die Warning bekomme, sobald ich 6 
oder mehr for loops einbaue:

P_DspWrite : process (iReset, iSysClk)
begin
    if iReset = '1' then
        sAccessViolationWr  <= '0';
        oRegister_1         <= (others => '0');
        oRegister_2         <= (others => '0');
        oBlock_1              <= (others => (others => '0'));
        oBlock_2              <= (others => (others => '0'));
        oBlock_3              <= (others => (others => '0'));
        oBlock_4              <= (others => (others => '0'));
        oBlock_5              <= (others => (others => '0'));
        oBlock_6              <= (others => (others => '0'));
        oBlock_7              <= (others => (others => '0'));

    elsif rising_edge(iSysClk) then
        if sDspCsN = '0' and sDspRwN = '0' then             -- write access
        
            sAccessViolationWr  <= '1';

            -- access blocks
            for i in 0 to BLOCK_SIZE-1 loop
                if sDspAddrA = addr_Block1Start +  i then
                    oBlock_1(i)           <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;

            for i in 0 to BLOCK_SIZE-1 loop
                if sDspAddrA = addr_Block2Start +  i then
                    oBlock_2(i)           <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;
            for i in 0 to BLOCK_SIZE-1 loop
                if sDspAddrA = addr_Block3Start +  i then
                    oBlock_3(i)           <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;
            for i in 0 to BLOCK_SIZE-1 loop
                if sDspAddrA = addr_Block4Start +  i then
                    oBlock_4(i)           <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;
            for i in 0 to BLOCK_SIZE-1 loop
                if sDspAddrA = addr_Block5Start +  i then
                    oBlock_5(i)           <= sDspDataInD;
                    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                end if;
            end loop;
            
-- UNCOMMENT THE FOLLOWING CODE TO GET A COMBINATIONAL LOOP
            -- for i in 0 to BLOCK_SIZE-1 loop
                -- if sDspAddrA = addr_Block6Start +  i then
                    -- oBlock_6(i)           <= sDspDataInD;
                    -- sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
                -- end if;
            -- end loop;

            assert sAccessViolationWr = '0' report "case defaulted! AddressDecoder Write Access" severity failure;

        end if; -- if sDspRwN = '0' then                            -- write access
    end if;         -- if rising_edge(iSysClkC)
end process;

[Fisch (Gast)]

Sieht irgendwie danach aus, dass du einem Signal im Reset-Zweig einen 
Wert zuweist im getakteten Zweig aber nicht. Kontrolliere das mal. Um 
mehr zu sagen bräuchte man mal die Signal- und Genericdeklaration.

Gruß,
Fisch

[Lattice User (Gast)]

Der Fehler ist immer noch nicht im gezeigten Code.

Mach ein vollständiges Beispiel, dass jemand einfach bei sich testen 
kann. CL137 ist in der Hilfe nicht dokumentiert, d.h. ein Fehler im Tool 
ist nicht auszuschliessen.


Lass mal die Loop für den 6. Block aus. Tritt der Fehler dann auch mit 
dem 7. auf? Tritt er auch auf wenn nur der 6. Block da ist?

[Fred (Gast)]

Ich habe den Adressdecoder mal auf ein minimum zusammengeschrumpft, 
sodass ich der Fehler weiterhin reproduziert werden kann.

Im Anhang findet Ihr den source Code, ein Diamond Projekt und je eine 
Implementierung mit und ohne kombinatorische Schleife. (jeweils nur 
synthese)

Die Combinational Loop Warning lässt sich verhindern, indem mindestens 
eine der for loops auskommentiert wird.

Ich würde mich freuen, wenn jemand Licht ins Dunkel bringen kann, was da 
passiert und ob diese Warning in meinem Fall als besorgniserregend zu 
betrachten ist.

Vielen Dank und beste Grüße
Fred

[Lothar M. (Firma: Titel) (lkmiller) (Moderator)]

Fred schrieb:
> ob diese Warning in meinem Fall als besorgniserregend zu betrachten ist.
Ich sehe diese etwas kuriosen "parallelen" RAMs sehr kritisch. Evtl. 
kommt der Synthesizer da ins Stolpern...
Wozu brauchst du diese eigenartigen RAMs? Ginge das nicht mit "normalen" 
RAMs?

Und das hier ist in einem getakteten Prozess unnötig wie ein Kropf:
> sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of
> sAccessViolationWr
Denn in einem getakteten Prozess wird der vorige Wert sowieso 
gespeichert. Das Signal übernimmt nur bei Änderung den "neuen" Wert.

[Lattice User (Gast)]

Der Fehler tritt auf wenn BLOCKSIZE * Anzahl Loops > 256 ist.

Wäre zu testen was passiert wenn man statt der loops einfach Vergelcihe 
verwendet.

PseudoCode (**) :

  if  sDspAddrA >= addr_BlockXStart && sDspAddrA < addr_BlockXStart BLOCKIZE ) then

     sAccessViolationWr <= sAccessViolationWr
  end if

Der Vergelich ist C-Style, als Veriloger habe ich zu wening Übung beim 
Schreiben in VHDL um das schnell mal zu testen, kann also jemand anders 
syntaktich hinbiegen.

Mit LSE statt Synplyfy Pro tritt der Fehler übrigens nicht auf.

Lothar M. schrieb:
> Und das hier ist in einem getakteten Prozess unnötig wie ein Kropf:
>> sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of
>> sAccessViolationWr
> Denn in einem getakteten Prozess wird der vorige Wert sowieso
> gespeichert. Das Signal übernimmt nur bei Änderung den "neuen" Wert.

Am Anfang des Processes wird es aus '1' gesetzt, und der alte Wert nur 
übernommen wenn keine Violation auftritt. Nimmt man die 
sAccessViolationWr  <= sAccessViolationWr raus, wird es bei jedem 
Zugriff auf '1' gesetzt.

[Fred (Gast)]

Lothar M. schrieb:
> Ich sehe diese etwas kuriosen "parallelen" RAMs sehr kritisch. Evtl.
> kommt der Synthesizer da ins Stolpern...
> Wozu brauchst du diese eigenartigen RAMs? Ginge das nicht mit "normalen"
> RAMs?

Es handelt sich dabei nicht um Block Rams, sondern um einen großen 
Adressmultiplexer. Mein Code hat original Code hat folgende Struktur:
 - Case, um einzelne Register abzufragen. Wenn mein Case keinen Treffer 
landet, wird sAccessViolationWr auf '1' gesetzt. In meinem 
Beispielprojekt habe ich die Case Anwendung durch sAccessViolationWr <= 
'1' ersetzt, da diese auf die Warning keinen Einfluss hat.
Anschließend kommen mehrere For loops, die aufeinanderfolgende Adressen 
abfragen, dies soll primär den Code Umfang reduzieren, jede for loop 
reduziert meinen Code um 48 "when" Einträge.

Lothar M. schrieb:
> Und das hier ist in einem getakteten Prozess unnötig wie ein Kropf:
>> sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of
>> sAccessViolationWr
> Denn in einem getakteten Prozess wird der vorige Wert sowieso
> gespeichert. Das Signal übernimmt nur bei Änderung den "neuen" Wert.
Da ich nicht mehr alle Register in meinem Case abfrage, bringt wird hier 
ein sAccessViolationWr Fehler generiert, sobald ich auf eine "Block 
Adresse" oder eine ungültige Adresse zugreife. Im Falle einer Block 
Adresse möchte ich den alten Fehlerstatus erhalten, d.h. den 
Fehlerstatus aus dem letzten Clk Cycle (sAccessViolationWr  <= 
sAccessViolationWr;) und somit den Case Fehler überschreiben.

Lattice User schrieb:
> Wäre zu testen was passiert wenn man statt der loops einfach Vergelcihe
> verwendet.
Da komme ich ursprünglich her, wollte mit den for loops eigentlich den 
Code Umfang verringern. Wenn ich alles in ein Case packe, tritt die 
Warnung nicht auf.

Lattice User schrieb:
> Mit LSE statt Synplyfy Pro tritt der Fehler übrigens nicht auf.
Das ist mir auch schon aufgefallen, bisher habe ich jedoch immer 
Synplify verwendet. Würdest Du mir einen Umstieg empfehlen? Welche Vor- 
oder Nachteile bringt mir der LSE?

Abschließen noch eine andere Frage:
Ich habe aktuell über 600 Register, auf die entweder lesend ODER 
schreibend zugegriffen wird. Beim Lesen musste ich die Register schon 
auf 2 Takte aufteilen, um das Timing noch zu schaffen.
Wie ist Euer Umgang mit so großen Adressdecodern? Habe ich den richtigen 
Ansatz gewählt?

[Duke Scarring (Gast)]

Fred schrieb:
>> Mit LSE statt Synplyfy Pro tritt der Fehler übrigens nicht auf.
> Das ist mir auch schon aufgefallen, bisher habe ich jedoch immer
> Synplify verwendet. Würdest Du mir einen Umstieg empfehlen? Welche Vor-
> oder Nachteile bringt mir der LSE?
Ich habe hier Code, bei dem ist es genau andersherum: Mit Synplyfy geht 
es und mit LSE nicht.
Außerdem habe ich den Eindruck, das LSE nicht so gut optimiert.
Vielleicht hängt das aber auch vom verwendeten Device ab. Ich verwende 
bisher nur den Mach XO2.

Duke

[Fred (Gast)]

Nachtrag

Lattice User schrieb:
> Wäre zu testen was passiert wenn man statt der loops einfach Vergelcihe
> verwendet.

Meine Antwort aus dem letzten beitrag war nicht ganz auf deine Idee 
zutreffend.
Hab jetzt mal die sAccessViolationWr Zuweisungen aus den Loops 
rausgenommen und durch folgende Abfrage ersetzt. Siehe da, die Warning 
erscheint nicht mehr.

if (sDspAddrA >= addr_Block1Start and sDspAddrA <= addr_Block1Start + BLOCK_SIZE-1) or
   (sDspAddrA >= addr_Block2Start and sDspAddrA <= addr_Block2Start + BLOCK_SIZE-1) or
   (sDspAddrA >= addr_Block3Start and sDspAddrA <= addr_Block3Start + BLOCK_SIZE-1) or
   (sDspAddrA >= addr_Block4Start and sDspAddrA <= addr_Block4Start + BLOCK_SIZE-1) or
   (sDspAddrA >= addr_Block5Start and sDspAddrA <= addr_Block5Start + BLOCK_SIZE-1) or
   (sDspAddrA >= addr_Block6Start and sDspAddrA <= addr_Block6Start + BLOCK_SIZE-1) 
then
    sAccessViolationWr  <= sAccessViolationWr;  -- keep previous state of sAccessViolationWr
end if;

[Lattice User (Gast)]

Duke Scarring schrieb:
> Fred schrieb:
>>> Mit LSE statt Synplyfy Pro tritt der Fehler übrigens nicht auf.
>> Das ist mir auch schon aufgefallen, bisher habe ich jedoch immer
>> Synplify verwendet. Würdest Du mir einen Umstieg empfehlen? Welche Vor-
>> oder Nachteile bringt mir der LSE?
> Ich habe hier Code, bei dem ist es genau andersherum: Mit Synplyfy geht
> es und mit LSE nicht.

Hier ist der Bug wohl die Warnung selbst, bzw der Text derselben. CL137 
ist
nicht dokumentiert. Sollte vermutlich heissen, du übertreibst es mit 
Zuweisenungen an ein Signal :-)

> Außerdem habe ich den Eindruck, das LSE nicht so gut optimiert.

LSE ist besser an das Device angepasst, bei Synplify muss ja noch ein 
Translationlayer nachgeschaltet werden. Aber Synplify hat die grössere 
Trickkiste. LSE ist noch am Anfang.

> Vielleicht hängt das aber auch vom verwendeten Device ab. Ich verwende
> bisher nur den Mach XO2.

[Fred (Gast)]

Hallo zusammen,

vielen Dank für Eure Antworten!

Ich verwende jetzt erst einmal die o.g. IF Abfrage.
Zu weitern Klärung, was mir die Warning sagen soll, habe ich trotzdem 
mal ein Ticket bei Lattice aufgemacht. Im Anhang findet Ihr den Source 
Code zum Ticket (noch mal vereinfacht, auf eine for Loop reduziert). Ich 
werde berichten, wenn ich da eine Antwort bekommen habe.

Für mich gilt der Fall erst einmal als gelöst.

Da aber an verschiedenen Stellen mein Code kritisiert wurde, freue ich 
mich über konstruktive Krtitik, wie ich die Fragestellung alternativ 
lösen kann.

Fred schrieb:
> Abschließen noch eine andere Frage:
> Ich habe aktuell über 600 Register, auf die entweder lesend ODER
> schreibend zugegriffen wird. Beim Lesen musste ich die Register schon
> auf 2 Takte aufteilen, um das Timing noch zu schaffen.
> Wie ist Euer Umgang mit so großen Adressdecodern? Habe ich den richtigen
> Ansatz gewählt?

[Sigi (Gast)]

Fred schrieb:
> Da aber an verschiedenen Stellen mein Code kritisiert wurde, freue ich
> mich über konstruktive Krtitik, wie ich die Fragestellung alternativ
> lösen kann.

Ja, teile den Zugriff auf mehrere Komponenten auf,
z.B. je Block eine Komponente.

Eine "angesprochene" Komponente gibt im Trefferfall
eine "1" aus. D.h. genau eine 1 ist von allen
Komponenten zu erwarten, sonst tritt ein Fehlerfall
ein. Evtl. muss jede dieser Block-Komponenten
ebenfalls in Subkomponenten ausgeteilt werden (32,
max 64 Register sind bestimmt gut handhabbar).

Der Test, ob genau eine 1 zurückgegeben wird, ist
ebenfalls recht einfach: Die Blöcke überschneiden
sich nicht => der Vektor aller Rückgaben muss auf
ungleich NULL getestet werden.

[Lattice User (Gast)]

Sigi schrieb:
> Fred schrieb:
>> Da aber an verschiedenen Stellen mein Code kritisiert wurde, freue ich
>> mich über konstruktive Krtitik, wie ich die Fragestellung alternativ
>> lösen kann.
>
> Ja, teile den Zugriff auf mehrere Komponenten auf,
> z.B. je Block eine Komponente.
>
> Eine "angesprochene" Komponente gibt im Trefferfall
> eine "1" aus. D.h. genau eine 1 ist von allen
> Komponenten zu erwarten, sonst tritt ein Fehlerfall
> ein. Evtl. muss jede dieser Block-Komponenten
> ebenfalls in Subkomponenten ausgeteilt werden (32,
> max 64 Register sind bestimmt gut handhabbar).
>
> Der Test, ob genau eine 1 zurückgegeben wird, ist
> ebenfalls recht einfach: Die Blöcke überschneiden
> sich nicht => der Vektor aller Rückgaben muss auf
> ungleich NULL getestet werden.

So ähnlich mache ich es auch.
Eine Case Anweisung für den Masterdecoder zur Generierung von 
Blockselects, dadurch bleibt dieser Übersichtlich und  man minimiert das 
Risiko dass man überlappende Blöcke hat.

Das Hauptproblem ist doch Implmentation, C Header und Dokumentation, 
konsistent zu halten. Irgdenwann kommt man da um Code-Generatoren nicht 
herum die VHDL/Verilog, C Header und Dokumentation auf Knopfdruck 
erzeugen. Ich habe sowas auch schon als Excelmacro gesehen.

[Sigi (Gast)]

Lattice User schrieb:
> Das Hauptproblem ist doch Implmentation, C Header und Dokumentation,
> konsistent zu halten. Irgdenwann kommt man da um Code-Generatoren nicht
> herum die VHDL/Verilog, C Header und Dokumentation auf Knopfdruck
> erzeugen. Ich habe sowas auch schon als Excelmacro gesehen.

Habe ich da etwas übersehen? Alle Adressen innerhalb
eines Blocks sind doch zusammenhängend, d.h. es ist
nur die Startadresse je Block anzugeben, der Rest
erfolgt per Generics etc.

[Lattice User (Gast)]

Sigi schrieb:
> Lattice User schrieb:
>> Das Hauptproblem ist doch Implmentation, C Header und Dokumentation,
>> konsistent zu halten. Irgdenwann kommt man da um Code-Generatoren nicht
>> herum die VHDL/Verilog, C Header und Dokumentation auf Knopfdruck
>> erzeugen. Ich habe sowas auch schon als Excelmacro gesehen.
>
> Habe ich da etwas übersehen?

Es ging bei den Excelmacro nicht nur um die Blöcke, sondern auch um die 
Details bis runter zu den Bits.

Die Systembuilder für Mico32 und Co leisten so etwas auch schon, 
zumindestens in Ansätzen.

[Fred (Gast)]

Hallo zusammen,

ich habe Euch eine Antwort versprochen, sobald das Lattice Ticket gelöst 
ist, und hier ist sie nun:

Bei der Warning CL137 handelt es sich tatsächlich um einen Bug in 
Synplify Pro und kann ignoriert werden.

Es hat sich leider etwas hingezogen, da mir der Lattice Support lange 
Zeit einreden wollte, dass es sich um eine kombinatorische Schleife 
handelt.

Erst widerholtes Nachfragen, ob es einen Beweis für eine kombinatorische 
Schleife gibt (z.B. im Technologie Viewer) hat die freundliche Dame dazu 
bewegt, sich doch einmal an Synopsis zu wenden, um diesen Fall nun 
endlich zu klären.

Noch einmal vielen Dank für Eure Hilfe.
Fred