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.

Sign up for GitHub

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

Jump to bottom

Support arrayed interfaces #879

Closed
veripoolbot opened this issue Feb 6, 2015 · 8 comments
Closed

Support arrayed interfaces #879

veripoolbot opened this issue Feb 6, 2015 · 8 comments
Labels
resolution: fixed Closed; fixed

Comments

@veripoolbot
Copy link
Contributor

Author Name: Varun Koyyalagunta
Original Redmine Issue: 879 from https://www.veripool.org
Original Date: 2015-02-06

Apologies if this is a duplicate.

Instantiating arrays of SystemVerilog interfaces would be a useful feature for my work. Is there any work being done on this? I am willing to work on it if it is tractable and someone can give me some pointers.

An example -

parameter N = 4;

interface a_if;
  logic a;
  modport source (output a);
  modport sink (input a);
endinterface

module intf_source(
  a_if.source i[N-1:0]
);
assign i[0].a = 0;
endmodule

module intf_sink(
  output [N-1:0] a_out,
  a_if.sink i[N-1:0]
);
assign a_out[0] = i[0].a;
endmodule

module intf_test
(
     output [N-1:0] a_out
);

     a_if i [N-1:0] ();
     intf_source source(i);
     intf_sink   sink(a_out, i);

endmodule

Currently errors with -

%Error: arrayed_interface.sv:10: Unsupported: Arrayed interfaces
%Error: arrayed_interface.sv:17: Unsupported: Arrayed interfaces
%Error: Exiting due to 2 error(s)

It also seems that supporting arrayed interfaces inside a generate block is a separate issue ([[#�]]), but I can live without that feature for now.
@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-02-13T01:49:05Z

If you are willing to have constant indices this should be doable, that is "iface[1].foo = bar", not "iface[signal].foo = bar". It would be excellent if you are willing to attack this:

FYI interfaces become both a module-like reference and a variable referencing that cell's variable. The module reference needs to be expanded into one for each array instance - see how V3Cell does this. Then the variables likewise need to be expanded into references into each instance.

  1. Make a test case in test_regress format (see docs)

  2. Look at how two examples expand now (see the .tree files with --debug), the first a normal arrayed module instantiation, and the other two non-array instances of an interface.

  3. See what breaks, then try to fix it!

Feel free to ask questions.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-04T01:32:37Z

Patch available: phb/verilator-dev@e5de9c8

It's not ready to be pulled yet -- fair bit of cleanup needed, but as far as I can tell at the moment, it seems to work -- any high-level feedback appreciated (I know there is lots of cleanup to be done).

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-11T04:52:14Z

Feedback from Wilson that ended up on a different ticket:

Great. Please fix the indentation - either 4 spaces or a 8 space tab; don't change the definition of tab-stops to 4.
Probably should test an interface that is numbered backwards "[0:N-1]", likewise "[N]".
Please add (anti-)copyright header to your new test.
--- a/src/V3Inst.cpp
+++ b/src/V3Inst.cpp
@@ -105,7 +105,7 @@ private:
-           } else if (nodep->modVarp()->isIfaceRef()) {
+           } else if (nodep->modVarp()->isIfaceRef() || nodep->modVarp()->subDTypep()->castUnpackArrayDType()->subDTypep()->castIfaceRefDTy$

Probably safer:

            } else if (nodep->modVarp()->isIfaceRef()
                       || (nodep->modVarp()->subDTypep()->castUnpackArrayDType()
                           && nodep->modVarp()->subDTypep()->castUnpackArrayDType()->subDTypep()->castIfaceRefDType())) {

@@ -268,6 +293,50 @@ private:
+                       // Now also clone the pin itself and update it's varref
+                       AstPin* newp = nodep->cloneTree(false);
+                       newp->modVarp(varNewp);
+                       newp->name(newp->name() + + "__BRA__" + cvtToStr(i) + "__KET__");

"+ +" not sure how that worked.

+                       // And replace exprp with a new varxref???

  // And replace exprp with a new varxref

index 3c88f99..bae801f 100644
--- a/src/V3LinkDot.cpp
+++ b/src/V3LinkDot.cpp
@@ -1666,7 +1691,7 @@ private:
                         refp->varp(varp);
                         m_ds.m_dotText = "";
                         if (m_ds.m_unresolved && m_ds.m_unlinkedScope) {
-                           newp = new AstUnlinkedVarXRef(nodep->fileline(), refp->castVarXRef(), refp->name(), m_ds.m_unlinkedScope->unlink$
+                           newp = new AstUnlinkedRef(nodep->fileline(), refp->castVarXRef(), refp->name(), m_ds.m_unlinkedScope->unlinkFrBa$

Please shorten the lines a bit

                         newp = new AstUnlinkedRef(nodep->fileline(), refp->castVarXRef(),
                                                    refp->name(), m_ds.m_unlinkedScope->unlinkFrBack());
@@ -1857,7 +1882,15 @@ private:
         } else if (m_ds.m_dotp && m_ds.m_dotPos == DP_FINAL) {
-           nodep->dotted(m_ds.m_dotText);  // Maybe "" 
+               if (m_ds.m_unresolved && m_ds.m_unlinkedScope) {
+                       AstNode *newp = new AstUnlinkedRef(nodep->fileline(), nodep->cloneTree(false), nodep->name(), m_ds.m_unlinkedScope->$
+                       m_ds.m_unlinkedScope = NULL;
+                       m_ds.m_unresolved = false;
+                       nodep->replaceWith(newp);

maybe missing a

             pushDeletep(nodep); VL_DANGLING(nodep);

@@ -2054,10 +2087,16 @@ private:
+       virtual void visit(AstConst* nodep, AstNUser*) {
+               UINFO(9,"  AstConst: "<<nodep<<" "<<m_ds.ascii()<<endl);
+               nodep->iterateChildren(*this);
+       }

Can remove, you had it for debug.

I'm going to keep holding off on this one until we get some other issues out of the way (994, 997, 996) -- I am seeing issues that I think stems from this patch when running large designs with heavy use of arrayed interfaces, but I haven't been able to isolate into a testcase yet.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-17T02:49:51Z

https://github.com/phb/verilator-dev/tree/arrayed_interfaces_879

Rebased on top of master and addressed comments. I fixed one issue that previously would cause parameterized arrayed interfaces to fail.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-17T22:23:49Z

Found some more issues with this patch in bigger designs. Will keep working on it.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-24T01:30:14Z

https://github.com/phb/verilator-dev/tree/issue879
From last version:

  • Rebased on master
  • Added support for [N] syntax.
  • Added test for [N] and [0:N-1] versions.

I'm feeling pretty confident that this patch does what it set out to do. We're still a little ways from being able to run our main designs, but this should be good to pull and we'll follow up with additional patches as we go along.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-12-06T00:47:48Z

This branch was provided which also fixes #�.

https://github.com/toddstrader/verilator-dev/commits/issue1001.3

This is fixed in git towards 3.880. Thanks to Todd Strader et al for this patch, I made some style changes and added a common function to find arrays to reduce some code duplication.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-12-19T15:32:56Z

In 3.880.

@veripoolbot veripoolbot added the resolution: fixed Closed; fixed label Dec 22, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
resolution: fixed Closed; fixed
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@veripoolbot