Status of synthesis support for microwatt

antonblanchard commented 5 years ago

Description ghdl-gcc and ghdl-llvm both produce a binary when building the FPGA target of microwatt, but ghdl-mcode does nothing.

Expected behaviour I'm expecting an error, or a binary. I likely have a bug in my VHDL, but I'm not sure where to look.

Context

OS: Fedora 30
Origin:
- I've tried both the Fedora 30 version, and a recent build of ghdl upstream from today

How to reproduce? A branch with the source is here:

https://github.com/antonblanchard/microwatt/tree/ghdl-silent-fail

To reproduce the issue:

GHDL=ghdl-mcode

git clone https://github.com/antonblanchard/microwatt
cd microwatt
git checkout ghdl-silent-fail

mkdir build
cd build

$GHDL -a --std=08 ../decode_types.vhdl
$GHDL -a --std=08 ../common.vhdl
$GHDL -a --std=08 ../wishbone_types.vhdl
$GHDL -a --std=08 ../fetch1.vhdl
$GHDL -a --std=08 ../fetch2.vhdl
$GHDL -a --std=08 ../decode1.vhdl
$GHDL -a --std=08 ../helpers.vhdl
$GHDL -a --std=08 ../decode2.vhdl
$GHDL -a --std=08 ../register_file.vhdl
$GHDL -a --std=08 ../cr_file.vhdl
$GHDL -a --std=08 ../crhelpers.vhdl
$GHDL -a --std=08 ../ppc_fx_insns.vhdl
$GHDL -a --std=08 ../execute1.vhdl
$GHDL -a --std=08 ../execute2.vhdl
$GHDL -a --std=08 ../loadstore1.vhdl
$GHDL -a --std=08 ../loadstore2.vhdl
$GHDL -a --std=08 ../multiply.vhdl
$GHDL -a --std=08 ../writeback.vhdl
$GHDL -a --std=08 ../wishbone_arbiter.vhdl
$GHDL -a --std=08 ../core.vhdl
$GHDL -a --std=08 ../simple_ram_behavioural_helpers.vhdl
$GHDL -a --std=08 ../simple_ram_behavioural.vhdl
$GHDL -a --std=08 ../fpga/clk_gen_bypass.vhd
$GHDL -a --std=08 ../fpga/pp_utilities.vhd
$GHDL -a --std=08 ../fpga/pp_fifo.vhd
$GHDL -a --std=08 ../fpga/pp_soc_memory.vhd
$GHDL -a --std=08 ../fpga/pp_soc_reset.vhd
$GHDL -a --std=08 ../fpga/pp_soc_uart.vhd
$GHDL -a --std=08 ../fpga/toplevel.vhd

$GHDL -e --std=08 toplevel

eine commented 5 years ago

Hi @antonblanchard! This is the default behaviour of mcode backend. Hence, everything works as expected. Your are doing nothing wrong, and GHDL is not failing. It's just that, you should have read the manual... Nonetheless, I understand that we do not always have time to do so. Please, let me point you to the relevant sections:

First two 'Hint' boxes in https://ghdl.readthedocs.io/en/latest/using/QuickStartGuide.html
First 'Hint' box in https://ghdl.readthedocs.io/en/latest/building/Building.html

Further commets:

As you see, mcode backend is a built-in in-memory backend (for x86/amd64 only), while either LLVM or GCC are 'external' backends that are 'plugged into' GHDL. This is why analysis is much faster with mcode, but simulation is faster with GCC or LLVM.
I think that VHPIDIRECT is supported with any backend. So, even if a binary is not generated, ghdl -r with mcode will also pick your C modules.
See the 'Warning' in https://ghdl.readthedocs.io/en/latest/licenses.html, which might have influence on your project.

antonblanchard commented 5 years ago

@1138-4EB Thanks, I figured I was doing something silly. Sorrry about that. I actually got here because yosys synthesis is failing without much information. I'm using the ghdl/synth:beta container. After running the above:

yosys -m ghdl -p "ghdl --std=08 toplevel"

 /----------------------------------------------------------------------------\
 |                                                                            |
 |  yosys -- Yosys Open SYnthesis Suite                                       |
 |                                                                            |
 |  Copyright (C) 2012 - 2019  Clifford Wolf <clifford@clifford.at>           |
 |                                                                            |
 |  Permission to use, copy, modify, and/or distribute this software for any  |
 |  purpose with or without fee is hereby granted, provided that the above    |
 |  copyright notice and this permission notice appear in all copies.         |
 |                                                                            |
 |  THE SOFTWARE IS PROVIDED "AS IS" AND THE AUTHOR DISCLAIMS ALL WARRANTIES  |
 |  WITH REGARD TO THIS SOFTWARE INCLUDING ALL IMPLIED WARRANTIES OF          |
 |  MERCHANTABILITY AND FITNESS. IN NO EVENT SHALL THE AUTHOR BE LIABLE FOR   |
 |  ANY SPECIAL, DIRECT, INDIRECT, OR CONSEQUENTIAL DAMAGES OR ANY DAMAGES    |
 |  WHATSOEVER RESULTING FROM LOSS OF USE, DATA OR PROFITS, WHETHER IN AN     |
 |  ACTION OF CONTRACT, NEGLIGENCE OR OTHER TORTIOUS ACTION, ARISING OUT OF   |
 |  OR IN CONNECTION WITH THE USE OR PERFORMANCE OF THIS SOFTWARE.            |
 |                                                                            |
 \----------------------------------------------------------------------------/

 Yosys 0.8+ (git sha1 UNKNOWN, clang 7.0.1-8 -fPIC -Os)

-- Running command `ghdl --std=08 toplevel' --

1. Executing GHDL.
ERROR: vhdl import failed.

eine commented 5 years ago

It's absolutely ok. It is quite common for new users to need some time to understand the complexity of GHDL. I still need to frequently ask Tristan about what's the status of certain features with some specific backend.

The error you get with ghdlsynth seems legit. Unfortunately, debugging when the yosys plugin crashes is not documented yet. @pepijndevos commented recently (see August 14, 2019 9:38 AM) that he tries to run plain GHDL when he finds this error. However, I assume that the sources are properly analyzed and elaborated, since you use GHDL as the main simulator in your project. Hence, I'd suggest you to try ghdl --synth --std=08 toplevel. It should trigger the same internal error, but you should get a proper 'GHDL Bug' block.

antonblanchard commented 5 years ago

@1138-4EB That helps!

ghdl --synth --std=08 toplevel

******************** GHDL Bug occurred ***************************
Please report this bug on https://github.com/ghdl/ghdl/issues
GHDL release: 0.37-dev (tarball) [Dunoon edition]
Compiled with GNAT Version: 8.3.0
Target: x86_64-linux-gnu
/src/build/
Command line:
ghdl --synth --std=08 toplevel
Exception SYSTEM.ASSERTIONS.ASSERT_FAILURE raised
Exception information:
raised SYSTEM.ASSERTIONS.ASSERT_FAILURE : vhdl-annotations.adb:1382
Call stack traceback locations:
0x7f08163ac4f1 0x5589c94c7d71 0x5589c94c2f30 0x5589c94c3ca6 0x5589c94c301e 0x5589c94c41fd 0x5589c94c4545 0x5589c94c4dc0 0x5589c94c4f08 0x5589c94c4965 0x5589c94c657c 0x5589c95c3489 0x5589c95c39b2 0x5589c95330c1 0x5589c966dabb 0x5589c932076d 0x7f0815f94099 0x5589c931f128 0xfffffffffffffffe
******************************************************************

I'll see if I can make more sense of it.

eine commented 5 years ago

I'm glad it helped! I believe you can use gdb to get more info about the context. Unfortunately, I have never used it with ghdl, so I cannot help you with it. I know that you would need to rebuild ghdl with debug symbols, tho. Currently, we do not provide any docker image where ghdl debug symbols are included.

tgingold commented 5 years ago

Don't try to debug. There are many features used by microwatt that aren't yet supported by ghdlsynth (like records). But I am making progress.

antonblanchard commented 5 years ago

@tgingold No problem, I'm happy to hold off until it's a bit more ready. Thanks for working on this!

tgingold commented 5 years ago

Right now, ghdl can synthesize 6 microwatt files. That's still WIP.

antonblanchard commented 5 years ago

That's fantastic! Thanks @tgingold

eine commented 5 years ago

@antonblanchard, it seems that this issue turned into status of synthesis support for microwatt. Would you mind updating the title accordingly?

antonblanchard commented 5 years ago

@1138-4EB good idea, done

tgingold commented 5 years ago

ghdl can now synthesize all the files (but individually). So, I have to synthesize core.vhdl and then check the synthesis is correct. I also have to fetch the latest commits.

Please (and that's really a PLEASE), if you get any crash with ghdl, do not try to workaround, but report it. I don't need many instructions. Just push the current state on a branch, and create an issue with with the command line you used and the commit id.

antonblanchard commented 5 years ago

@tgingold fantastic! Do you want me to report synthesis issues, or hold off? eg:

library ieee;
use ieee.std_logic_1164.all;
use ieee.numeric_std.all;

entity test is
    port (
        v_in  : in std_ulogic_vector(63 downto 0);
        v_out : out std_ulogic_vector(63 downto 0)
        );
end test;

architecture rtl of test is
            function fls_64 (val: std_ulogic_vector(63 downto 0)) return integer is
        variable ret: integer;
    begin
        ret := 64;
        for i in val'range loop
            if val(i) = '1' then
                ret := 63 - i;
                exit;
            end if;
        end loop;

        return ret;
    end;
begin
    v_out <= std_ulogic_vector(to_unsigned(fls_64(v_in), 64));
end rtl;

gives:

synth_sequential_statements: cannot handle IIR_KIND_EXIT_STATEMENT (test.vhdl:20:17)

tgingold commented 5 years ago

After the latest push, ghdl cam handle next/exit statements.

tgingold commented 5 years ago

ghdl --synth is now able to synthesize the whole core and the result simulates ok. Next step: yosys import.

pepijndevos commented 5 years ago

This is incredibly exciting! I plan to work on Yosys things this weekend, so let's see tomorrow what has to be done.

On October 11, 2019 6:36:33 PM GMT+02:00, tgingold notifications@github.com wrote:

ghdl --synth is now able to synthesize the whole core and the result simulates ok. Next step: yosys import.

-- You are receiving this because you were mentioned. Reply to this email directly or view it on GitHub: https://github.com/ghdl/ghdl/issues/903#issuecomment-541136026

-- Sent from my Android device with K-9 Mail. Please excuse my brevity.

pepijndevos commented 5 years ago

Hmm, @antonblanchard your fifo seems invalid VHDL-2008 due to the shared variable. Could that be changed without breaking Xilinx memory inference?

The build script given at the top of this issue is no longer up to date.

Trying to synthesize just the core rather than the whole SoC, using the following script:

GHDL=ghdl

mkdir -p build
cd build

$GHDL -a --std=08 ../decode_types.vhdl
$GHDL -a --std=08 ../common.vhdl
$GHDL -a --std=08 ../wishbone_types.vhdl
$GHDL -a --std=08 ../fetch1.vhdl
$GHDL -a --std=08 ../fetch2.vhdl
$GHDL -a --std=08 ../decode1.vhdl
$GHDL -a --std=08 ../helpers.vhdl
$GHDL -a --std=08 ../insn_helpers.vhdl
$GHDL -a --std=08 ../decode2.vhdl
$GHDL -a --std=08 ../register_file.vhdl
$GHDL -a --std=08 ../cr_file.vhdl
$GHDL -a --std=08 ../crhelpers.vhdl
$GHDL -a --std=08 ../ppc_fx_insns.vhdl
$GHDL -a --std=08 ../rotator.vhdl
$GHDL -a --std=08 ../logical.vhdl
$GHDL -a --std=08 ../countzero.vhdl
$GHDL -a --std=08 ../execute1.vhdl
$GHDL -a --std=08 ../execute2.vhdl
$GHDL -a --std=08 ../loadstore1.vhdl
$GHDL -a --std=08 ../loadstore2.vhdl
$GHDL -a --std=08 ../multiply.vhdl
$GHDL -a --std=08 ../writeback.vhdl
$GHDL -a --std=08 ../wishbone_arbiter.vhdl
$GHDL -a --std=08 ../cache_ram.vhdl
$GHDL -a --std=08 ../plru.vhdl
$GHDL -a --std=08 ../icache.vhdl
$GHDL -a --std=08 ../divider.vhdl
$GHDL -a --std=08 ../core_debug.vhdl
$GHDL -a --std=08 ../core.vhdl

$GHDL --synth --std=08 core

On Microwatt bd73c17 and GHDL 966ffd5b I get the following error:

../fetch1.vhdl:41:17:error: synth_dyadic_operation: unhandled IIR_PREDEFINED_RECORD_INEQUALITY

******************** GHDL Bug occurred ***************************
Please report this bug on https://github.com/ghdl/ghdl/issues
GHDL release: 0.37-dev (v0.36-1003-g966ffd5b) [Dunoon edition]
Compiled with GNAT Version: 9.2.0
Target: x86_64-pc-linux-gnu
/home/pepijn/code/microwatt/build/
Command line:
ghdl --synth --std=08 core
Exception TYPES.INTERNAL_ERROR raised
Exception information:
raised TYPES.INTERNAL_ERROR : synth-oper.adb:816
Call stack traceback locations:
0x559e1a5c1a7a 0x559e1a5cc61c 0x559e1a5cd0ed 0x559e1a5b5cb5 0x559e1a5bc4cc 0x559e1a5b5e1e 0x559e1a5bc4cc 0x559e1a5bca2b 0x559e1a5be2e5 0x559e1a5b32f1 0x559e1a5b339f 0x559e1a5af5e5 0x559e1a6128d5 0x559e1a555c32 0x559e1a61ca69 0x559e1a41f0b4 0x7f1c14e33151 0x559e1a41e02c 0xfffffffffffffffe
******************************************************************

Is this a recent change in Microwatt that introduced new breakage, or a regression in GHDL? I'll see if I can fix this. Records might be a bit tricky maybe?

antonblanchard commented 5 years ago

@pepijndevos the UART is straight out of the potato core. That shared variable has been a problem elsewhere and I've been meaning to make it 2008 compatible. I'll do that now.

I did notice the same error when synthesizing microwatt yesterday. Perhaps we should stick to a particular SHA1 for the synthesis work since we are still making quite a lot of changes to microwatt. @tgingold are you working off a particular SHA1?

eine commented 5 years ago

@antonblanchard, this afternoon @tgingold commented:

I am using f1ac8894b19c57d5ebad25e14b4f9e5841bc5b41 Will update.

antonblanchard commented 5 years ago

@pepijndevos I took a look at the FIFO and can't see a reason why it needs to be a shared variable. I pushed a fix in antonblanchard/microwatt#93

antonblanchard commented 5 years ago

@pepijndevos IIR_PREDEFINED_RECORD_INEQUALITY is just some debug output and you can comment the two instances out until it is fixed, eg:

            --if (r /= rin) then
                --report "fetch2 rst:" & std_ulogic'image(rst) &
                    --" S:" & std_ulogic'image(stall_in) &
                    --" F:" & std_ulogic'image(flush_in) &
                    --" T:" & std_ulogic'image(rin.stop_mark) &
                    --" V:" & std_ulogic'image(rin.valid) &
                    --" nia:" & to_hstring(rin.nia);
            --end if;

At that point the icache fails ghdl --synth with an assert:

raised TYPES.INTERNAL_ERROR : synth-stmts.adb:2387

I also tried running yosys against everything individually, and found the following issues:

smul and neg are not implemented
Memidx is not implemented as we have previously discussed in tgingold/ghdlsynth-beta#56
I hit a few asserts that I don't see with ghdl --synth

eine commented 5 years ago

IIR_PREDEFINED_RECORD_INEQUALITY is just some debug output and you can comment the two instances out until it is fixed, eg:

Is this something that would be wrapped with --synthesis translate_off and --synthesis translate_on pragmas for some vendor tool? If so, it might be worth a separate issue.

BTW, regarding the new implementation of the FIFO, did you test if the synthesis is still correct with Vivado?

pepijndevos commented 5 years ago

I first get a crash in icache where it expects wait to be the first statement on line 260. Also some debugging code. Putting the wait as the first statement I get another error. It seems that GHDL wants to synth the wait condition like it were an if, but this one has none.

pepijndevos commented 5 years ago

There seems to be a problem with nested generate blocks. Might have a look, but might go over my head.

Deleting those blocks it gets to synth-stmts.adb:1319 where there is a comment FIXME: Arguments are passed by copy.

That instruction cache sure knows how to exercise ghdl.

pepijndevos commented 5 years ago

Ok I give up... I have an UB32 of 64 bits?? https://github.com/tgingold/ghdlsynth-beta/blob/master/src/ghdl.cc#L98

I've said this before, but I'll say it again: I've been contributing to GHDL throughout the summer, and I still find it impossible to debug any non-trivial issues. I think a big part of that is the impenetrable data representation in GHDL.

To give you an example of my current situation: I'm in GDB, in netlists.get_param_uns32 (inst=132, param=1), and I'm trying to figure out what this instance is, and where it came from. If I just print it it'll just print 132, so I source .gdbinit and try all of them. Most of them don't work in this particular context. pt seems to work, printing some stuff that makes very little sense to me, is this UB32 actually an enum? Must be an effing huge enum to require 64 bits.

enumeration_literal "enq" [132]
  location: *std_standard*:1:1
  identifier: "enq" [678]
  subprogram_hash:  134
  enum_pos:  5
  seen_flag: false
  visible_flag: true
  is_within_flag: false
  expr_staticness: local
  name_staticness: local
  parent: package_declaration "standard" [16]
  type: enumeration_type_definition [126] "character"
  literal_origin: *null*

What I'm really trying to figure out is where this thing came from. Because it seems like ghdl.cc is alright. But I'm completely incapable of walking the AST, finding the VHDL source this belongs to, or even finding which net/signal/type this is.

I think it would be really helpful if there was some documentation about how to debug things in GHDL, because I think at least for me it's a big problem that prevents me from doing anything non-trivial on GHDL.

tgingold commented 5 years ago

Pepijn,

many data structures use integers to designate nodes. Of course, gdb doesn't know how to display them better than a single number. Maybe some python extensions should be written.

So, for vhdl nodes, use: pt n (or ptf n) for netlists nets: use: call debug_net (n) for netlists instance, use: call dump_instance (n, true) for netlists module, use: call dump_module (n, 0)

For UB32 and SB32, the width can be larger than 32, in that case the value is unsigned and signed extended.

Gates are documented in netlists-gates, and dump procedures are declared in netlists-dump.

pepijndevos commented 5 years ago

Okay, that's useful, but still struggling a lot

(gdb) call dump_instance (inst, true)
  instance %71 {i132}: $const_UB32
    parameters $val=4
    outputs %71.$o{n163}:64
(gdb) call get_output(inst, 0)
$3 = 163
(gdb) call debug_net(163)
64'uh0000000000000004
(gdb) call dump_net_name(163, true)
%71.$o{n163}
(gdb) call get_first_sink(163)
$9 = 194
(gdb) call get_input_parent(194)
$10 = 133
(gdb) call disp_instance(166, true)
\i_in[32:1]
(gdb) call dump_instance(166, true)
  instance %101 {i166}: $extract
    parameters $offset=1
    inputs %101.$i{p236}
    outputs %101.$o{n203}:32

Then I decided to grep the raw ghdl dump and found this... thing

    \rin_int.$o[63:0]{n137} := $signal{i106} ({n164}$add{i133} ({n162}%69.$o[63:0], {n163}64'uh0000000000000004))

Which leads to this code in fetch1.vhd

    v_int.nia_next := std_logic_vector(unsigned(v.nia) + 4);

    -- Update registers
    rin <= v;
    rin_int <= v_int;

So now I'm super confused. See, in the source of Disp_Instance it actually only gets one 32 bit value, while in ghdlsynth's get_src it tries to read a second parameter for the next 32 bits.

For UB32 and SB32, the width can be larger than 32, in that case the value is unsigned and signed extended.

Can you clarify this? Do you mean that constants are only really 32 bits

tgingold commented 5 years ago

On 13 Oct 2019, at 20:56, Pepijn de Vos notifications@github.com wrote:

Okay, that's useful, but still struggling a lot

(gdb) call dump_instance (inst, true) instance %71 {i132}: $const_UB32 parameters $val=4 outputs %71.$o{n163}:64

In the output, {nXXX} means net XXX. Which is confirmed by:

(gdb) call get_output(inst, 0) $3 = 163 (gdb) call debug_net(163) 64'uh0000000000000004

And the net is displayed after outputs, as confirmed by:

(gdb) call dump_net_name(163, true) %71.$o{n163}

[...]

Which leads to this code in fetch1.vhd

If you know which net has an issue, just use ghdl --synth output (with --out=raw or --out=dump) to inspect the netlist. If you need to trace to sources, get the net name and look for it in the vhdl output. Maybe we should output the location for raw/dump outputs.

UB32 values should be zero extended when converted to yosys sigs.

pepijndevos commented 5 years ago

Ah ok, then the ghdlsynth code is clearly wrong, as it is trying to read more params. https://github.com/tgingold/ghdlsynth-beta/blob/9a8c8162b869479017ccb6f642fadce717685fa3/src/ghdl.cc#L98 Is this correct for the bit case? In that case I'll split off the ub32 case.

tgingold commented 5 years ago

Yes, you need to split the ub32 case. (and ul32 and sb32).

tgingold commented 4 years ago

Status update: it looks like ghdl is able to synthesize master of microwatt... But: it expands as dff the caches memory, which therefore generates a huge netlist. It reports that the memories are written asynchronously. I have to investigate.

antonblanchard commented 4 years ago

@tgingold excellent! I just worked through issues I saw in the soc build and submitted (hopefully) reduced test cases. Getting close :)

ghdl / ghdl

Status of synthesis support for microwatt #903