Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

removing vhdl 2008 features to support vivado simulations #242

Closed
wants to merge 7 commits into from

Conversation

alaasal
Copy link

@alaasal alaasal commented Aug 28, 2020

Some vhdl 2008 features are not suported for simulation in vivado even recent version 2020. This batch fixes all the simulation errors.

@sharkcz
Copy link
Contributor

sharkcz commented Aug 28, 2020

I would probably split the commit into 3, one for the = '1' test, another for the when/else replacement by the if block and the to_string as last. I guess it would then create one commit per missing feature that would be easier to eg. revert in the future.

Signed-off-by: alaasal <alaamohsalman@gmail.com>
…ividing the changes to commit every removed feature alone

This reverts commit 48d681a.

Signed-off-by: alaasal <alaamohsalman@gmail.com>
…vivado simulation

Signed-off-by: alaasal <alaamohsalman@gmail.com>
…rs to support vivado simulation

Signed-off-by: alaasal <alaamohsalman@gmail.com>
…support vivado simulation

Signed-off-by: alaasal <alaamohsalman@gmail.com>
@alaasal
Copy link
Author

alaasal commented Aug 28, 2020

Done, I divided the commit to three different commits. The PR fails but I don't know why.
"Unable to find image 'ghdl/vunit:llvm' locally"

@mikey
Copy link
Collaborator

mikey commented Aug 31, 2020

CI is failing because the code isn't equivalent. CI starts passing if you add this:

index 32b08475be..196d95388a 100644
--- a/execute1.vhdl
+++ b/execute1.vhdl
@@ -512,7 +512,7 @@ begin
         right_shift <= '0';
     end if;
 
-    if ( e_in.insn_type = OP_RLC ) then
+    if ( ( e_in.insn_type = OP_RLC ) or (e_in.insn_type = OP_RLCL) ) then
         rot_clear_left <= '1';
         else
         rot_clear_left <= '0';

Also, please use 8 space tabs like the rest of the code.

execute1.vhdl Outdated Show resolved Hide resolved
Signed-off-by: alaasal <alaamohsalman@gmail.com>
Signed-off-by: alaasal <alaamohsalman@gmail.com>
@mikey
Copy link
Collaborator

mikey commented Sep 10, 2020

So I'm happy to take these fixes:

-	    if (rst) then
+	    if (rst = '1') then

And the to_string() -> to_hstring() fixes.

I don't want to take the right_shift <= '1' when e_in.insn_type = OP_SHR else '0'; -> if blah else endif as they make the code significantly more bloated and more importantly I don't believe this is new in VHDL 2008. I think your simulator is pretty broken if it doesn't support that. Also why have you only converted a few of these (even in execute.vhdl)?

Also, can you collapse this series into 1 or 2 commits. We don't need the reverts and the white space changes as separate fixes.

Microwatt is a mess of 8 spaces and tabs but please do not introduce 4 space indentation to this.

@alaasal
Copy link
Author

alaasal commented Sep 10, 2020 via email

@paulusmack
Copy link
Collaborator

The changes for the rotator control signals would look nicer as a case statement on e_in.insn_type.

@benjaminmordaunt
Copy link

As a note on this - I was stunned that even Vivado 2021 doesn't support when/else statement in simulation...
Even GHDL manages it and that's open source :/

@umarcor
Copy link
Contributor

umarcor commented Jun 8, 2021

As a note on this - I was stunned that even Vivado 2021 doesn't support when/else statement in simulation...
Even GHDL manages it and that's open source :/

Moreover, GHDL's synth command allows converting VHDL 2008 sources to a VHDL 1993 netlist that Vivado can accept. Hence, GHDL can be effectively used as a VHDL 2008 to 1993 "conversion" for synthesis purposes. See --out=vhdl-raw in https://ghdl.github.io/ghdl/using/Synthesis.html#cmdoption-ghdl-out.

microwatt can already be simulated with open source and several vendor tools. It can also be synthesised and implemented with open source or vendor tools. I don't think it's worth downgrading the codebase 30 years back just for supporting Vivado. The problem is on Xilinx's side, not on the rest of the community/industry.

@Paebbels
Copy link

Paebbels commented Jun 8, 2021

Caring VHDL support for synthesis is ok, but here Vivado has already a quite good support. Caring about simulation support for e tool less people use and no one should use (because of it's technical lacks, bus and missing language support) is a different question.

So please don't downgrade because one vendor is not capable of implementing 18 years old features (VHDL-2002) ...

@alaasal
Copy link
Author

alaasal commented Jun 13, 2021

the pr was originally to support vivado simulations because vivado was the only free simulator that runs verilog+vhdl simulations and back then we wanted to integrate microwatt(vhdl) + openpiton(verilog).

@umarcor
Copy link
Contributor

umarcor commented Jun 13, 2021

the pr was originally to support vivado simulations because vivado was the only free simulator that runs verilog+vhdl simulations and back then we wanted to integrate microwatt(vhdl) + openpiton(verilog).

I don't know which was the state back then, however, nowadays:

https://www.intel.es/content/www/es/es/software/programmable/quartus-prime/model-sim.html
Does ModelSim*-Intel® FPGA edition software support dual-language simulation?

Yes. Starting with Intel® Quartus® Prime Software v15.0, the ModelSim*-Intel® FPGA edition software supports dual-language simulation. This includes designs that are written in a combination of Verilog, System Verilog, and VHDL languages, also known as mixed HDL.

Furthermore, this repository does already use mixed-hdl cosimulation since May-June 2020, combining open source tools (GHDL and Verilator). I acknowledge it's not the cleanest solution, but it works. See https://github.com/antonblanchard/microwatt/blob/master/Makefile#L114-L138 and ghdl/ghdl#1335:

In order to be able to test microwatt (VHDL) with Litedram (Verilog) in sim together, I made a verilator model of litedram, which I wrapped with vhpidirect, and plugged it in-place of the verilog when running GHDL.

@alaasal
Copy link
Author

alaasal commented Jun 13, 2021

There are specific maximum lines of code that limit ModelSim free version support for mixed-language simulation (don't know if this changed), but I don't think there was such a thing for Vivado.

@paulusmack
Copy link
Collaborator

Consensus is that we don't want many of the things in this PR. If there are minor cleanups then please open separate PRs for them.

@paulusmack paulusmack closed this Aug 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants