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

build: add ConstraintManager. #64

Closed
wants to merge 1 commit into from
Closed

build: add ConstraintManager. #64

wants to merge 1 commit into from

Conversation

jfng
Copy link
Contributor

@jfng jfng commented Apr 26, 2019

No description provided.

@codecov
Copy link

codecov bot commented Apr 26, 2019

Codecov Report

Merging #64 into master will increase coverage by 0.18%.
The diff coverage is 95.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #64      +/-   ##
==========================================
+ Coverage    86.2%   86.38%   +0.18%     
==========================================
  Files          28       30       +2     
  Lines        4174     4297     +123     
  Branches      867      897      +30     
==========================================
+ Hits         3598     3712     +114     
- Misses        481      486       +5     
- Partials       95       99       +4
Impacted Files Coverage Δ
nmigen/build/tools.py 50% <50%> (ø)
nmigen/build/constraint.py 98.26% <98.26%> (ø)
nmigen/hdl/ir.py 95.1% <0%> (-0.87%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 699fe5a...6bcb17e. Read the comment docs.


# Implementation of the Yosys RTLIL::IdString escaping function, as defined in
# https://github.com/YosysHQ/yosys/blob/master/backends/verilog/verilog_backend.cc#L101-L159
def yosys_id_escape(s):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This list doesn't have anything to do with Yosys itself, it is a list of (System)Verilog keywords.

@@ -0,0 +1,161 @@
from collections import defaultdict, OrderedDict
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file (and its corresponding test file) should really be called something like nmigen.build.constraint.

if not isinstance(resource, Resource):
raise TypeError("Object {!r} is not a Resource".format(resource))
if isinstance(resource.io[0], Subsignal):
raise ConstraintError("Cannot constrain Resource ('{}', {}) to a period of {} ns "
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It would be more clear to say "Cannot constrain period of resource … because it has subsignals", since there is no period that would be valid here at all.


class ConstraintManager:
def __init__(self, resources):
self.available = defaultdict(dict)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First, this should be an OrderedDict too.

Second, a much nicer logic would be to have two dicts, resources and requested. In the first dict would be contained all resources ever registered with this constraint manager. In the second dict, there would be contained resources requested from this constraint manager.

The reasons are twofold. First, oMigen has a problem that if a resource is requested twice, it acts the second time as if the resource has never existed in the first place. This is very confusing. Second, with the current implementation, you can request, add, request, add, ... the same resource any amount of times.

self.tristates = []
self.diffpairs = []
self.ports = []
self.inouts = []
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, since none of the fields above appear to be a part of the public API, they all should be named starting with an underscore.

@whitequark
Copy link
Contributor

whitequark commented May 26, 2019

Thanks for this great PR. I've merged it manually with some changes:

  • Added XDR support.
  • Changed period constraints to frequency constraints, in Hz. There are two reasons. First, frequencies are almost always referred to in Hz; for example most clock signals in nmigen have names like clk100. Second, being in Hz and not MHz simplifies calculations when the same frequency value is used elsewhere, like when setting up counters.
  • Changed record creation code to use the proper interface for filling in record fields.
  • Removed escaping code from ConstraintManager because that's not necessarily required or done the same way as Yosys by the target toolchain.

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

2 participants