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

Enable CI for yosys synthesis for monitoring resource usages #87

Open
wants to merge 8 commits into
base: master
from

Conversation

@Nancy-Chauhan
Copy link
Contributor

commented Jul 4, 2019

Solves #85

mor1kx.core Outdated
@@ -87,3 +89,12 @@ targets:
- "tool_rivierapro? (monitor)"
- "tool_xsim? (monitor)"
parameters: [trace_enable, trace_to_screen]

tinyfpga_bx:

This comment has been minimized.

Copy link
@olofk

olofk Jul 30, 2019

Member

If we're only using this target to get the synthesis stats, then we never run the pnr tool and can simplify this a bit by dropping nextpnr_options and setting pnr : none directly in the core file. This will also allow us to get rid of the pcf file and the tinyfpga_bx fileset. When we do that we should also rename the target to something more descriptive, such as synth or something like that

There is even some further improvements to be done, but before that I would like to know if it is a mistake, or by design that the fpu fileset is not included

This comment has been minimized.

Copy link
@Nancy-Chauhan

Nancy-Chauhan Jul 30, 2019

Author Contributor

Thank you @olofk, I will do the changes. Other than this fpu was not included intentionally, because with it that it gave the error: https://paste.ubuntu.com/p/VB2CPkckhv/

This comment has been minimized.

Copy link
@stffrdhrn

stffrdhrn Jul 30, 2019

Member

Thanks @olofk for the comments this should have had a TODO comment, disabling FPU was just a temporary measure to get the design to synthesize without requiring investigation into the verilog fixes. It looks like some verilator annotations/comments are causing the failure. @bandvig Do you think we can just remove them?

This comment has been minimized.

Copy link
@olofk

olofk Jul 31, 2019

Member

We don't need to remove them. It's sufficient to just move it to after the case statement (case (generic_cmp_opc_i) // synthesis parallel_case). Seems like yosys is a bit stricter about that than many other tools.

With that said, if we include the fpu when doing the synthesis stats, then we can do further simplifications of the core file, but perhaps we should make the changes that we already talked about first

This comment has been minimized.

Copy link
@bandvig

bandvig Jul 31, 2019

Contributor

@olofk @stffrdhrn
I should note that 'synthesis parallel_case' is used in MAROCCHINO many times. If moving such meta-comments just after case statement (as Olof proposed) desn't hurt behavior of other tools, I agree.

This comment has been minimized.

Copy link
@stffrdhrn

stffrdhrn Aug 1, 2019

Member

I confirmed that @olofk 's fix for parallel case allows the build to work now. @Nancy-Chauhan you can add back fpu if you rebase to master.

Show resolved Hide resolved Jenkinsfile

@Nancy-Chauhan Nancy-Chauhan force-pushed the Nancy-Chauhan:yosys-ci branch from fada72c to 0bc5af2 Aug 1, 2019

yosys.sh Outdated
@@ -0,0 +1,7 @@
#! /bin/bash

cd /src

This comment has been minimized.

Copy link
@stffrdhrn

stffrdhrn Aug 1, 2019

Member

This script only will work if run in jenkins. Its probably better to move it somewhere else. Or move it into the docker image repo?

This comment has been minimized.

Copy link
@Nancy-Chauhan

Nancy-Chauhan Aug 4, 2019

Author Contributor

@stffrdhrn It will generate printing statistics and can work in Travis also, at least to show the results in travis.log. Do you think I should add this to Travis too?

Show resolved Hide resolved yosys.sh Outdated
Show resolved Hide resolved mor1kx.core Outdated
@@ -0,0 +1,38 @@
#!/usr/bin/env python3

This comment has been minimized.

Copy link
@stffrdhrn

stffrdhrn Aug 1, 2019

Member

I still think this should be in the Docker image, but I guess OK for now.

This comment has been minimized.

Copy link
@Nancy-Chauhan

Nancy-Chauhan Aug 8, 2019

Author Contributor

@stffrdhrn, I will use this in librecores/librecores-ci-jenkins-server#67, after this PR is merged and will do changes(will send another PR ) once its library is developed, I guess that will be ok?

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Aug 8, 2019

We might move it right away.
Ideally I would prefer to keep it in YOSYS repo so that we do not maintain it across versions.. But it is also fine to have it in the image directly for now

This comment has been minimized.

Copy link
@Nancy-Chauhan

Nancy-Chauhan Aug 12, 2019

Author Contributor

Raised librecores/docker-images#28 for this. Will make modifications once it is merged

@stffrdhrn

This comment has been minimized.

Copy link
Member

commented Aug 4, 2019

@Nancy-Chauhan Nancy-Chauhan marked this pull request as ready for review Aug 7, 2019

@oleg-nenashev
Copy link

left a comment

Fine as a temporary solution. Jenkins rendering is something I am going to fix

@stffrdhrn

This comment has been minimized.

Copy link
Member

commented Aug 11, 2019

The names like response times and throughput don't make much sense. There is probably something we can do about that. Maybe use a different graphing plugin?

@Nancy-Chauhan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 11, 2019

Hi @stffrdhrn I already discussed with @oleg-nenashev. But I think we can only work around this plugin. We need to do modifications in the plugin itself to support key-value type CSV

stffrdhrn and others added some commits Jul 12, 2019

tinyfpga_bx: Fixup synthesis core config
Changes to core:
 - remove fpu and monitor filesets
   * Monitor is not needed as its for simulation.
   * FPU synth is failing due to some flags yosys doesn't support, we can
     investigate later.
 - set top level verilog module to mor1kx

The synthesis runs now, but pnr fails with the below error:

  $ fusesoc run --target=tinyfpga_bx mor1kx
  ERROR: failed to place cell 'traceport_exec_insn_o[3]$sb_io' of type 'SB_IO'
  ERROR: Placing design failed.

I started adding the cells i.e. (traceport_exec_insn_o[3]) to the pcf
file at random pins just to see if we could get PNR to succeed, but I
think we will not have enough pins.

We don't really need the PNR to run so we have some options:
 1. Try to get olof's pnr: none option to work
 2. Keep PNR and add a simple wrapper module like data/toplevel.v to
    wrap mor1kx and wire most things to 1b'0.

@Nancy-Chauhan Nancy-Chauhan force-pushed the Nancy-Chauhan:yosys-ci branch from 2b9e6a5 to fd6f8e7 Aug 12, 2019

Nancy Chauhan Nancy Chauhan

@Nancy-Chauhan Nancy-Chauhan force-pushed the Nancy-Chauhan:yosys-ci branch from 33e0ade to dc27c41 Aug 13, 2019

@stffrdhrn
Copy link
Member

left a comment

It's looking better.

Show resolved Hide resolved .jenkins/yosys.sh Outdated

@Nancy-Chauhan Nancy-Chauhan force-pushed the Nancy-Chauhan:yosys-ci branch from 57ca0ef to ec5d733 Aug 15, 2019

@Nancy-Chauhan

This comment has been minimized.

Copy link
Contributor Author

commented Aug 17, 2019

@oleg-nenashev @stffrdhrn , I have added final changes to this PR. Please let me know for any changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.