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

Jump to bottom

Verilator fails to handle constant functions that assign individual members of packed structures #997

Closed
veripoolbot opened this issue Nov 9, 2015 · 5 comments
Closed

Verilator fails to handle constant functions that assign individual members of packed structures #997

veripoolbot opened this issue Nov 9, 2015 · 5 comments
Labels
resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800

Comments

@veripoolbot
Copy link
Contributor

Author Name: Johan Bjork
Original Redmine Issue: 997 from https://www.veripool.org
Original Date: 2015-11-09
Original Assignee: Johan Bjork

Testcase + Patch ready to cherry pick here @ phb/verilator-dev@7057c54
@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-11-10T00:29:31Z

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.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-11T04:23:50Z

Ack, sorry for the confusion here. I kept piling patches on-top of my work-in-progress work and as this particular patch only touched V3Simulate so I left it in my dev branch.
I'll split the patch out onto it's own branch and add the additional tests. I'll fix the feedback ^ for the ticket where they belong.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Johan Bjork
Original Date: 2015-11-11T04:27:30Z

https://github.com/phb/verilator-dev/tree/#� contains only the commit intended for this ticket. It's the same changeset as mentioned originally in this ticket -- As far as I can tell all review feedback was directed towards another change that was in the same branch.

@veripoolbot
Copy link
Contributor Author

Original Redmine Comment
Author Name: Wilson Snyder (@wsnyder)
Original Date: 2015-11-12T01:50:02Z

More great stuff, thanks. Pushed to git towards 3.880.

@veripoolbot
Copy link
Contributor Author

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

In 3.880.

@veripoolbot veripoolbot added resolution: fixed Closed; fixed type: feature-IEEE Request to add new feature, described in IEEE 1800 labels 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 type: feature-IEEE Request to add new feature, described in IEEE 1800
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@veripoolbot