Microwatt emits lots of metavalue assertions when simulating in GHDL

[antonblanchard]

After getting GHDL to emit some more information about metavalue assertions, the worst offenders are (the first column is the number of times I saw the assertion):

   3431 in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).mmu_0@mmu(behave).finalmaskgen
    847 in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).mmu_0@mmu(behave).addrmaskgen
    457 in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).icache_0@icache(rtl).icache_comb
    186 in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).dcache_0@dcache(rtl).dcache_request
     78 in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).icache_0@icache(rtl).itlb_update

[antonblanchard]

I started looking at these assertions. Ideally we'd like to get to a point where we could enable ghdls --assert-level=warning and avoid new regressions.

One thing I am struggling with is how to avoid assertions during reset. Take for example icache_tb.vhdl. We initialise i_out.nia at the start of the test, but we still get an assert.

I can fix this by initialising the signal:

-    signal r : reg_internal_t;
+    signal r : reg_internal_t := (hit_way => 0,
+                   hit_nia => (others => '0'),
+                   state => IDLE,
+                   wb => wishbone_master_out_init,
+                   store_way => 0,
+                   store_index => 0,
+                   store_row => 0,
+                   store_tag => (others => ('0')),
+                   end_row_ix => (others => '0'),
+                   rows_valid => (others => ('0')),
+                   others => '0');

and:

-    signal i_out        : Fetch1ToIcacheType;
+    signal i_out        : Fetch1ToIcacheType := (nia => (others => '0'), others => '0');

But that's a lot of boilerplate for something that is reset in the first cycle. I'm also loath to add more signal initialisation to microwatt because it was a source of issues in our sky130 ASIC work.

@umarcor any thoughts?

(FYI the testbench is currently failing, but that is a separate issue I need to chase)

[umarcor]

With regard to initialising the signals, I do agree that'd be more cumbersome than helpful, particularly given that microwatt is being used for ASICs, and not only for FPGA targets. Hence, I believe this should be fixed/handled on the GHDL side. However, I've not really gone into the details of the verbosity during the first cycles of the simulations. During the last year, there have been discussions about it in the VHDL Analysis and Standardisation group because there are mixed opinions about what the default verbosity should be. Traditionally, the motto is "better print too much and let the user filter, rather than risk not printing something meaningful". You might want to bring the issue in https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues.

With regard to the specific case of i_out.nia, it seems that you want to use --asserts=disable-at-0 (https://ghdl.github.io/ghdl/using/Simulation.html#cmdoption-ghdl-ieee-asserts)? That should get rid of all the assertions at the begining.

[antonblanchard]

@umarcor I didn't know about --asserts=disable-at-0! That makes this issue go away. Thanks!

I'm still seeing some issues where the signal is resolved on the first positive edge of the clock, eg:

# ./core_tb --backtrace-severity=warning --assert-level=warning --asserts=disable-at-0

../../src/ieee2008/numeric_std-body.vhdl:3036:7:@5ns:(assertion warning): NUMERIC_STD.TO_INTEGER: metavalue detected, returning 0
./core_tb:error: assertion failed
in process .core_tb(behave).soc0@soc(behaviour).processor@core(behave).writeback_0@writeback(behaviour).writeback_0
./core_tb:error: simulation failed

The assert is 5ns in, at negative clock edge.

[umarcor]

I guess that's because the reset is synchronous, not asynchronous:

microwatt/writeback.vhdl

Lines 47 to 51 in 05fe709

	if rising_edge(clk) then
	if rst = '1' then
	r.state <= WRITE_SRR0;
	r.srr1 <= (others => '0');
	else

. I think it's not possible to "disable until/for". However, you might want to combine --asserts and --ieee-asserts to disable all IEEE asserts (always, not just at 0).

[antonblanchard]

I was looking at this again this morning. We could add an option to ghdl to ignore asserts until a specific time, something like --ieee-asserts=disable-until-100

However that wont resolve some of the things we are doing in microwatt:

        -- load data formatting
        -- shift and byte-reverse data bytes
        for i in 0 to 7 loop
            j := to_integer(r2.byte_index(i)) * 8;
            data_permuted(i * 8 + 7 downto i * 8) := d_in.data(j + 7 downto j);
        end loop;

This is in the data path, and sometimes is fed with U/X state. Ideally we'd want to propagate the U state through to_integer(), instead of raising an assertion (which if we ignore the assertion it gets sanitized to 0 and if we dont ignore we have to add extra logic to the data path to avoid).

While fighting all of this, it does make me think we should just async reset every FF vs playing the U/X state whack a mole game.

[umarcor]

@antonblanchard I suggest to open an issue in https://gitlab.com/IEEE-P1076/VHDL-Issues/-/issues, in order to discuss this topic in the VHDL community. I recall it being discussed in the mailing list a couple of years ago. Overall, there is some friction between "I/we prefer excessive verbosity so that no warning/error goes unreported" and "I/we all know that doing 'math' with U/X does not work". The workaround might be done at the simulator level (GHDL); however, since these warnings are coming from the (standard) IEEE libs, we might want to have it "fixed" there (i.e. have a switch to control reporting metavalue related issues).

/cc @JimLewis @Paebbels

[umarcor]

FTR, https://grouper.ieee.org/groups/1076/email/msg01180.html

[Paebbels]

Using U and X propagation is the correct way to simulate.

From a mathematical point of view, an invalid input can't be converted to an integer. An invalid integer can't be expressed by VHDL. Moreover, an array access with an invalid pointer should lead to an invalid data word.

for i in 0 to 7 loop
  if is_X(r2.byte_index(i)) then
    data_permuted(i * 8 + 7 downto i * 8) := (others => 'X');
  else
    j := to_integer(r2.byte_index(i)) * 8;
    data_permuted(i * 8 + 7 downto i * 8) := d_in.data(j + 7 downto j);
  end if;
end loop;

This could be improved by a handwritten has_U and has_X function, so you can make 2 cases with if has_U(...) and elsif has_X(...) to propagate U and X accordingly.

If your tool might create logic for the if clause, then add a and SIMULATION condition to the if. This SIMULATION can be computed as follows:

package utils is
  -- Distinguishes simulation from synthesis
  constant SIMULATION : boolean;  -- deferred constant declaration
end package;

package body utils is
  function is_simulation return boolean is
    variable ret : boolean := false;
  begin
    --synthesis translate_off
    ret := true;
    --synthesis translate_on
    return ret;
  end function;

  -- deferred constant assignment
  constant SIMULATION : boolean := is_simulation;
end package body;

[antonblanchard]

Thanks @Paebbels and @umarcor. I can see why we can't propagate U/X state through integers, and we either need to modify the code to return U/X state as @Paebbels highlights, or avoid feeding U/X state to conversion functions in the first place. I'm working on that now.

While I have you both here - I hacked up some python to parse our records and ports, and spit out VHDL assertions such that we can catch places we propagate U/X state across module boundaries. In general we've been too lax and I'm finding it difficult to root cause gate level issues after the code goes through ghdl -> yosys -> verilog -> yosys (in openroad) with so much U/X state floating around.

My script parses records, eg:

type Execute1ToWritebackType is record
        valid: std_ulogic;
        instr_tag : instr_tag_t;
        rc : std_ulogic;
        mode_32bit : std_ulogic;
        write_enable : std_ulogic;
...

and ports, eg

entity writeback is
    port (
        clk          : in std_ulogic;
        rst          : in std_ulogic;

        e_in         : in Execute1ToWritebackType;

To create:

--pragma synthesis_off
    x_state_checks: process(clk)
    begin
        if rising_edge(clk) then
            if not rst then
                assert not(is_x(clk)) severity failure;
                assert not(is_x(rst)) severity failure;
                assert not(is_x(e_in.valid)) severity failure;
                assert not(is_x(e_in.instr_tag.valid)) severity failure;
                assert not(is_x(e_in.rc)) severity failure;
                assert not(is_x(e_in.mode_32bit)) severity failure;
...

I wanted to use pyGHDL to do the parsing instead my hand coded stuff. I found the example at https://libraries.io/pypi/pyVHDLModel but it fails on a lot of our files (eg https://github.com/antonblanchard/microwatt/blob/master/icache.vhdl):

[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Array_Subtype_Definition
[NOT IMPLEMENTED] Variable assignment (label: 'None') at line 460
[NOT IMPLEMENTED] Variable assignment (label: 'None') at line 461
[NOT IMPLEMENTED] Variable assignment (label: 'None') at line 487
Traceback (most recent call last):
  File "/tmp/blah.py", line 12, in <module>
    document = Document(sourceFile)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/NonStandard.py", line 149, in __init__
    self.translate()
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/NonStandard.py", line 204, in translate
    architecture = Architecture.parse(libraryUnit, contextItems)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/DesignUnit.py", line 183, in parse
    return cls(architectureNode, name, entity, contextItems, declaredItems, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/DesignUnit.py", line 165, in __init__
    super().__init__(identifier, entity, contextItems, declaredItems, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2058, in __init__
    self._statements    = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2058, in <listcomp>
    self._statements    = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/_Translate.py", line 877, in GetConcurrentStatementsFromChainedNodes
    yield ProcessStatement.parse(statement, label, True)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Concurrent.py", line 271, in parse
    return cls(processNode, label, declaredItems, statements, sensitivityList)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Concurrent.py", line 250, in __init__
    super().__init__(label, declaredItems, statements, sensitivityList)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2379, in __init__
    SequentialStatements.__init__(self, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in __init__
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in <listcomp>
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/_Translate.py", line 950, in GetSequentialStatementsFromChainedNodes
    yield IfStatement.parse(statement, label)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 160, in parse
    ifBranch = IfBranch.parse(ifNode, label)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 95, in parse
    return cls(branchNode, condition, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 81, in __init__
    super().__init__(condition, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2935, in __init__
    super().__init__(statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2929, in __init__
    SequentialStatements.__init__(self, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in __init__
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in <listcomp>
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/_Translate.py", line 950, in GetSequentialStatementsFromChainedNodes
    yield IfStatement.parse(statement, label)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 160, in parse
    ifBranch = IfBranch.parse(ifNode, label)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 95, in parse
    return cls(branchNode, condition, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 81, in __init__
    super().__init__(condition, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2935, in __init__
    super().__init__(statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2929, in __init__
    SequentialStatements.__init__(self, statements)
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in __init__
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyVHDLModel/SyntaxModel.py", line 2297, in <listcomp>
    self._statements = [] if statements is None else [s for s in statements]
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/_Translate.py", line 952, in GetSequentialStatementsFromChainedNodes
    yield ForLoopStatement.parse(statement, label)
  File "/home/anton/.local/lib/python3.10/site-packages/pyGHDL-3.0.0.dev0-py3.10.egg/pyGHDL/dom/Sequential.py", line 373, in parse
    raise DOMException(
pyGHDL.dom.DOMException: Unknown discete range kind 'Simple_Name' in for...loop statement at line 490.

Are we using newer features not implemented yet, or am I doing something wrong?

[Paebbels]

Translating all of GHDLs internal AST into the pyVHDLModel is a huge task. I would say like 50..60% is done, but some parts like for and if statements need translation code. As I did the first round of implementation, I focused on hierarchy like entity, architecture, types, functions, instantiation, generate statements, but not what's inside of processes or functions. That's why some parts print a [NOT IMPLEMENTED]. We used some existing code in unit testing and checked if no exception is raised, so all code is somehow accepted, but maybe not fully translated.

If you can provide your code, I can do 2 things in my vacation:

• fix the exception
• implement the missing parts inside of processes, so you can extract the information you need

General note:
pyGHDL.DOM waits for 2 features from GHDL itself:

• keeping comments so documentation can be extracted
• resolve symbols so e.g. names can be linked to type declarations

---

@umarcor microwatt is not part of our pyGHDL.DOM testing environment, right?

[Paebbels]

We currently test against OSVVM and UVVM. See: https://github.com/ghdl/extended-tests but not microwatt or neorv32

[antonblanchard]

Thanks @Paebbels. I'm not sure it warrants a bunch of your vacation time since I have a pretty hacky solution. The first argument is the file to get the port information from, and all subsequent ones are files to get record information from.

#!/usr/bin/python3

import sys
import re

record_re = re.compile(r'type\s+(\S+)\s+is\s+record(.*?)\s+end\s+record;', re.MULTILINE|re.DOTALL)
record_elem_re = re.compile(r'(\S+)\s*:\s*(\S+)')

record_dict = dict()

# These types can never have x state, so we can't use is_x() on them
record_dict["insn_type_t"] = [ ]
record_dict["tag_number_t"] = [ ]
record_dict["intr_vector_t"] = [ ]
record_dict["unit_t"] = [ ]
record_dict["facility_t"] = [ ]
record_dict["length_t"] = [ ]
record_dict["repeat_t"] = [ ]
record_dict["input_reg_a_t"] = [ ]
record_dict["input_reg_b_t"] = [ ]
record_dict["input_reg_c_t"] = [ ]
record_dict["output_reg_a_t"] = [ ]
record_dict["rc_t"] = [ ]
record_dict["carry_in_t"] = [ ]
record_dict["state_t"] = [ ]
record_dict["natural"] = [ ]
record_dict["boolean"] = [ ]
record_dict["index_t"] = [ ]

# Ignore this
record_dict["memory_t"] = [ ]

# These are just wrappers around std_ulogic_vectors
record_dict["gspr_index_t"] = [ "" ]
record_dict["wishbone_addr_type"] = [ "" ]
record_dict["wishbone_data_type"] = [ "" ]
record_dict["wishbone_sel_type"] = [ "" ]

for filename in sys.argv[2:]:
    with open(filename) as f:
        text = f.read()

        for match in record_re.finditer(text):
            arr = list()
            name = match.group(1)
            elements = match.group(2)
            for m2 in record_elem_re.finditer(elements):
                elem = m2.group(1)
                typ = m2.group(2).rstrip(';')

                if 'std_ulogic' in typ or 'std_logic' in typ:
                    arr.append(f"{elem}")
                else:
                    # Add the type for further expansion
                    arr.append(f"{elem}:{typ}")

            record_dict[name] = arr


def create_assert(s):
    print(f"                assert not(is_x({s})) severity failure;")


def expand(record, basename):
    for entry in record:
        if ":" in entry:
            (name, subrecord_str) = entry.split(":")
            n = f"{basename}.{name}"
            subrecord = record_dict[subrecord_str]
            expand(subrecord, n)
        else:
            if entry == "":
                n = f"{basename}"
            else:
                n = f"{basename}.{entry}"
            create_assert(n)

header = f"""
--pragma synthesis_off
    x_state_checks: process(clk)
    begin
        if rising_edge(clk) then
            if not rst then"""

footer = f"""            end if;
        end if;
    end process;
--pragma synthesis_off"""

print(header)

in_port = False
paren_depth = 0
port_str = ''
found = False

with open(sys.argv[1]) as f:
    for line in f.readlines():
        # Strip comments
        line = line.split('--')[0]

        if not in_port:
            if 'port' in line:
                in_port = True
                paren_depth = 1
                port_str = ''
        else:
            for e in line:
                if e == '(':
                    paren_depth = paren_depth + 1
                elif e == ')':
                    paren_depth = paren_depth - 1
                    if paren_depth == 0:
                        found = True
            else:
                port_str = port_str + line

        if found:
            break


for line in port_str.splitlines():
    # Strip comments
    if '--' in line:
        (line, ignore) = line.split('--')
    # Strip initialisation
    if ':=' in line:
        (line, ignore) = line.split(':=')
    if ':' in line:
        (name, typ) = line.split(":")
        name = name.strip()
        typ = typ.replace(' out ', '').replace(' in ', '').strip(';')
        typ = typ.strip()
        # Remove anything after a space
        if ' ' in typ:
            typ = typ.split(' ')[0]
        if 'std_ulogic' in typ or 'std_logic' in typ:
            create_assert(name)
        else:
            expand(record_dict[typ], name)

print(footer)